[v5,4/5] lib/bpf: use wait event scheme for Rx/Tx iteration
Checks
Commit Message
Instead of polling for cbi->use to be updated, use wait event scheme.
Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is because of
a compilation error:
-----------------------------------------------------------------------
../lib/eal/include/rte_common.h:36:13: error: read-only variable ‘value’
used as ‘asm’ output
36 | #define asm __asm__
| ^~~~~~~
../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion of macro
‘asm’
66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
| ^~~
../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion of macro
‘__LOAD_EXC_32’
96 | __LOAD_EXC_32((src), dst, memorder) \
| ^~~~~~~~~~~~~
../lib/eal/arm/include/rte_pause_64.h:167:4: note: in expansion of macro
‘__LOAD_EXC’
167 | __LOAD_EXC((addr), value, memorder, size) \
| ^~~~~~~~~~
../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro ‘rte_wait_event’
125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
-----------------------------------------------------------------------
Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
lib/bpf/bpf_pkt.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
Comments
> -----邮件原件-----
> 发件人: Feifei Wang <feifei.wang2@arm.com>
> 发送时间: Tuesday, October 26, 2021 4:02 PM
> 收件人: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; Feifei Wang
> <Feifei.Wang2@arm.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>
> 主题: [PATCH v5 4/5] lib/bpf: use wait event scheme for Rx/Tx iteration
>
> Instead of polling for cbi->use to be updated, use wait event scheme.
>
> Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is because of a
> compilation error:
> -----------------------------------------------------------------------
> ../lib/eal/include/rte_common.h:36:13: error: read-only variable ‘value’
> used as ‘asm’ output
> 36 | #define asm __asm__
> | ^~~~~~~
>
> ../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion of macro ‘asm’
> 66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
> | ^~~
>
> ../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion of macro
> ‘__LOAD_EXC_32’
> 96 | __LOAD_EXC_32((src), dst, memorder) \
> | ^~~~~~~~~~~~~
>
> ../lib/eal/arm/include/rte_pause_64.h:167:4: note: in expansion of macro
> ‘__LOAD_EXC’
> 167 | __LOAD_EXC((addr), value, memorder, size) \
> | ^~~~~~~~~~
>
> ../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro ‘rte_wait_event’
> 125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> -----------------------------------------------------------------------
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> lib/bpf/bpf_pkt.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index
> 6e8248f0d6..213d44a75a 100644
> --- a/lib/bpf/bpf_pkt.c
> +++ b/lib/bpf/bpf_pkt.c
> @@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
> * Waits till datapath finished using given callback.
> */
> static void
> -bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> +bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
Hi, Konstantin
For this bpf patch, I delete 'const' through this is contrary to what we
discussed earlier. This is because if we keep 'constant' here and use 'rte_wait_event'
new macro, compiler will report error. And earlier the arm version cannot be compiled
due to I forgot enable "wfe" config in the meson file, so this issue can not happen before.
> {
> - uint32_t nuse, puse;
> + uint32_t puse;
>
> /* make sure all previous loads and stores are completed */
> rte_smp_mb();
> @@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
>
> /* in use, busy wait till current RX/TX iteration is finished */
> if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> - do {
> - rte_pause();
> - rte_compiler_barrier();
> - nuse = cbi->use;
> - } while (nuse == puse);
> + rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> + __ATOMIC_RELAXED);
> }
> }
>
> --
> 2.25.1
Hi Feifei,
> > Instead of polling for cbi->use to be updated, use wait event scheme.
> >
> > Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is because of a
> > compilation error:
> > -----------------------------------------------------------------------
> > ../lib/eal/include/rte_common.h:36:13: error: read-only variable ‘value’
> > used as ‘asm’ output
> > 36 | #define asm __asm__
> > | ^~~~~~~
> >
> > ../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion of macro ‘asm’
> > 66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
> > | ^~~
> >
> > ../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion of macro
> > ‘__LOAD_EXC_32’
> > 96 | __LOAD_EXC_32((src), dst, memorder) \
> > | ^~~~~~~~~~~~~
> >
> > ../lib/eal/arm/include/rte_pause_64.h:167:4: note: in expansion of macro
> > ‘__LOAD_EXC’
> > 167 | __LOAD_EXC((addr), value, memorder, size) \
> > | ^~~~~~~~~~
> >
> > ../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro ‘rte_wait_event’
> > 125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > -----------------------------------------------------------------------
> >
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > lib/bpf/bpf_pkt.c | 11 ++++-------
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index
> > 6e8248f0d6..213d44a75a 100644
> > --- a/lib/bpf/bpf_pkt.c
> > +++ b/lib/bpf/bpf_pkt.c
> > @@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
> > * Waits till datapath finished using given callback.
> > */
> > static void
> > -bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> > +bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
>
> Hi, Konstantin
>
> For this bpf patch, I delete 'const' through this is contrary to what we
> discussed earlier. This is because if we keep 'constant' here and use 'rte_wait_event'
> new macro, compiler will report error. And earlier the arm version cannot be compiled
> due to I forgot enable "wfe" config in the meson file, so this issue can not happen before.
Honestly, I don't understand why we have to remove perfectly valid 'const' qualifier here.
If this macro can't be used with pointers to const (still don't understand why),
then let's just not use this macro here.
Strictly speaking I don't see much benefit here from it.
>
> > {
> > - uint32_t nuse, puse;
> > + uint32_t puse;
> >
> > /* make sure all previous loads and stores are completed */
> > rte_smp_mb();
> > @@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> >
> > /* in use, busy wait till current RX/TX iteration is finished */
> > if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> > - do {
> > - rte_pause();
> > - rte_compiler_barrier();
> > - nuse = cbi->use;
> > - } while (nuse == puse);
> > + rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > + __ATOMIC_RELAXED);
> > }
> > }
> >
> > --
> > 2.25.1
> Hi Feifei,
>
> > > Instead of polling for cbi->use to be updated, use wait event scheme.
> > >
> > > Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is because of a
> > > compilation error:
> > > -----------------------------------------------------------------------
> > > ../lib/eal/include/rte_common.h:36:13: error: read-only variable ‘value’
> > > used as ‘asm’ output
> > > 36 | #define asm __asm__
> > > | ^~~~~~~
> > >
> > > ../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion of macro ‘asm’
> > > 66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
> > > | ^~~
> > >
> > > ../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion of macro
> > > ‘__LOAD_EXC_32’
> > > 96 | __LOAD_EXC_32((src), dst, memorder) \
> > > | ^~~~~~~~~~~~~
> > >
> > > ../lib/eal/arm/include/rte_pause_64.h:167:4: note: in expansion of macro
> > > ‘__LOAD_EXC’
> > > 167 | __LOAD_EXC((addr), value, memorder, size) \
> > > | ^~~~~~~~~~
> > >
> > > ../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro ‘rte_wait_event’
> > > 125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > -----------------------------------------------------------------------
> > >
> > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > > lib/bpf/bpf_pkt.c | 11 ++++-------
> > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index
> > > 6e8248f0d6..213d44a75a 100644
> > > --- a/lib/bpf/bpf_pkt.c
> > > +++ b/lib/bpf/bpf_pkt.c
> > > @@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
> > > * Waits till datapath finished using given callback.
> > > */
> > > static void
> > > -bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> > > +bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
> >
> > Hi, Konstantin
> >
> > For this bpf patch, I delete 'const' through this is contrary to what we
> > discussed earlier. This is because if we keep 'constant' here and use 'rte_wait_event'
> > new macro, compiler will report error. And earlier the arm version cannot be compiled
> > due to I forgot enable "wfe" config in the meson file, so this issue can not happen before.
>
>
> Honestly, I don't understand why we have to remove perfectly valid 'const' qualifier here.
> If this macro can't be used with pointers to const (still don't understand why),
> then let's just not use this macro here.
> Strictly speaking I don't see much benefit here from it.
>
> >
> > > {
> > > - uint32_t nuse, puse;
> > > + uint32_t puse;
> > >
> > > /* make sure all previous loads and stores are completed */
> > > rte_smp_mb();
> > > @@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> > >
> > > /* in use, busy wait till current RX/TX iteration is finished */
> > > if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> > > - do {
> > > - rte_pause();
> > > - rte_compiler_barrier();
> > > - nuse = cbi->use;
> > > - } while (nuse == puse);
> > > + rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > + __ATOMIC_RELAXED);
After another thought, if we do type conversion at macro invocation time:
bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
{
...
rte_wait_event((uint32_t *)&cbi->use, UINT32_MAX, ==, puse, __ATOMIC_RELAXED);
would that help?
> > > }
> > > }
> > >
> > > --
> > > 2.25.1
> -----邮件原件-----
> 发件人: dev <dev-bounces@dpdk.org> 代表 Ananyev, Konstantin
> 发送时间: Tuesday, October 26, 2021 8:57 PM
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> 主题: Re: [dpdk-dev] [PATCH v5 4/5] lib/bpf: use wait event scheme for Rx/Tx
> iteration
>
>
> > Hi Feifei,
> >
> > > > Instead of polling for cbi->use to be updated, use wait event scheme.
> > > >
> > > > Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is
> > > > because of a compilation error:
> > > > ------------------------------------------------------------------
> > > > -----
> > > > ../lib/eal/include/rte_common.h:36:13: error: read-only variable ‘value’
> > > > used as ‘asm’ output
> > > > 36 | #define asm __asm__
> > > > | ^~~~~~~
> > > >
> > > > ../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion of
> macro ‘asm’
> > > > 66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
> > > > | ^~~
> > > >
> > > > ../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion of
> > > > macro ‘__LOAD_EXC_32’
> > > > 96 | __LOAD_EXC_32((src), dst, memorder) \
> > > > | ^~~~~~~~~~~~~
> > > >
> > > > ../lib/eal/arm/include/rte_pause_64.h:167:4: note: in expansion of
> > > > macro ‘__LOAD_EXC’
> > > > 167 | __LOAD_EXC((addr), value, memorder, size) \
> > > > | ^~~~~~~~~~
> > > >
> > > > ../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro ‘rte_wait_event’
> > > > 125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > ------------------------------------------------------------------
> > > > -----
> > > >
> > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > ---
> > > > lib/bpf/bpf_pkt.c | 11 ++++-------
> > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index
> > > > 6e8248f0d6..213d44a75a 100644
> > > > --- a/lib/bpf/bpf_pkt.c
> > > > +++ b/lib/bpf/bpf_pkt.c
> > > > @@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
> > > > * Waits till datapath finished using given callback.
> > > > */
> > > > static void
> > > > -bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> > > > +bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
> > >
> > > Hi, Konstantin
> > >
> > > For this bpf patch, I delete 'const' through this is contrary to
> > > what we discussed earlier. This is because if we keep 'constant' here and
> use 'rte_wait_event'
> > > new macro, compiler will report error. And earlier the arm version
> > > cannot be compiled due to I forgot enable "wfe" config in the meson file,
> so this issue can not happen before.
> >
> >
> > Honestly, I don't understand why we have to remove perfectly valid 'const'
> qualifier here.
> > If this macro can't be used with pointers to const (still don't
> > understand why), then let's just not use this macro here.
> > Strictly speaking I don't see much benefit here from it.
> >
> > >
> > > > {
> > > > - uint32_t nuse, puse;
> > > > + uint32_t puse;
> > > >
> > > > /* make sure all previous loads and stores are completed */
> > > > rte_smp_mb();
> > > > @@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi
> > > > *cbi)
> > > >
> > > > /* in use, busy wait till current RX/TX iteration is finished */
> > > > if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> > > > - do {
> > > > - rte_pause();
> > > > - rte_compiler_barrier();
> > > > - nuse = cbi->use;
> > > > - } while (nuse == puse);
> > > > + rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > + __ATOMIC_RELAXED);
>
> After another thought, if we do type conversion at macro invocation time:
>
> bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi) {
> ...
> rte_wait_event((uint32_t *)&cbi->use, UINT32_MAX, ==, puse,
> __ATOMIC_RELAXED);
>
> would that help?
I try to with this and it will report compiler warning:
' cast discards ‘const’ qualifier'.
I think this is due to that in rte_wait_event macro, we use
typeof(*(addr)) value = 0;
and value is defined as "const uint32_t",
but it should be able to be updated.
Furthermore, this reflects the limitations of the new macro, it cannot be applied
when 'addr' is type of 'const'. Finally, I think I should give up the change for "bpf".
>
>
> > > > }
> > > > }
> > > >
> > > > --
> > > > 2.25.1
> -----邮件原件-----
> 发件人: Feifei Wang
> 发送时间: Wednesday, October 27, 2021 3:04 PM
> 收件人: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> 主题: 回复: [PATCH v5 4/5] lib/bpf: use wait event scheme for Rx/Tx iteration
>
>
>
> > -----邮件原件-----
> > 发件人: dev <dev-bounces@dpdk.org> 代表 Ananyev, Konstantin
> > 发送时间: Tuesday, October 26, 2021 8:57 PM
> > 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> > 抄送: dev@dpdk.org; nd <nd@arm.com>; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> > 主题: Re: [dpdk-dev] [PATCH v5 4/5] lib/bpf: use wait event scheme for
> > Rx/Tx iteration
> >
> >
> > > Hi Feifei,
> > >
> > > > > Instead of polling for cbi->use to be updated, use wait event scheme.
> > > > >
> > > > > Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is
> > > > > because of a compilation error:
> > > > > ----------------------------------------------------------------
> > > > > --
> > > > > -----
> > > > > ../lib/eal/include/rte_common.h:36:13: error: read-only variable
> ‘value’
> > > > > used as ‘asm’ output
> > > > > 36 | #define asm __asm__
> > > > > | ^~~~~~~
> > > > >
> > > > > ../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion
> > > > > of
> > macro ‘asm’
> > > > > 66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
> > > > > | ^~~
> > > > >
> > > > > ../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion
> > > > > of macro ‘__LOAD_EXC_32’
> > > > > 96 | __LOAD_EXC_32((src), dst, memorder) \
> > > > > | ^~~~~~~~~~~~~
> > > > >
> > > > > ../lib/eal/arm/include/rte_pause_64.h:167:4: note: in expansion
> > > > > of macro ‘__LOAD_EXC’
> > > > > 167 | __LOAD_EXC((addr), value, memorder, size) \
> > > > > | ^~~~~~~~~~
> > > > >
> > > > > ../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro
> ‘rte_wait_event’
> > > > > 125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > > ----------------------------------------------------------------
> > > > > --
> > > > > -----
> > > > >
> > > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > ---
> > > > > lib/bpf/bpf_pkt.c | 11 ++++-------
> > > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index
> > > > > 6e8248f0d6..213d44a75a 100644
> > > > > --- a/lib/bpf/bpf_pkt.c
> > > > > +++ b/lib/bpf/bpf_pkt.c
> > > > > @@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
> > > > > * Waits till datapath finished using given callback.
> > > > > */
> > > > > static void
> > > > > -bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> > > > > +bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
> > > >
> > > > Hi, Konstantin
> > > >
> > > > For this bpf patch, I delete 'const' through this is contrary to
> > > > what we discussed earlier. This is because if we keep 'constant'
> > > > here and
> > use 'rte_wait_event'
> > > > new macro, compiler will report error. And earlier the arm version
> > > > cannot be compiled due to I forgot enable "wfe" config in the
> > > > meson file,
> > so this issue can not happen before.
> > >
> > >
> > > Honestly, I don't understand why we have to remove perfectly valid
> 'const'
> > qualifier here.
> > > If this macro can't be used with pointers to const (still don't
> > > understand why), then let's just not use this macro here.
> > > Strictly speaking I don't see much benefit here from it.
> > >
> > > >
> > > > > {
> > > > > - uint32_t nuse, puse;
> > > > > + uint32_t puse;
> > > > >
> > > > > /* make sure all previous loads and stores are completed */
> > > > > rte_smp_mb();
> > > > > @@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi
> > > > > *cbi)
> > > > >
> > > > > /* in use, busy wait till current RX/TX iteration is finished */
> > > > > if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> > > > > - do {
> > > > > - rte_pause();
> > > > > - rte_compiler_barrier();
> > > > > - nuse = cbi->use;
> > > > > - } while (nuse == puse);
> > > > > + rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > > + __ATOMIC_RELAXED);
> >
> > After another thought, if we do type conversion at macro invocation time:
> >
> > bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi) {
> > ...
> > rte_wait_event((uint32_t *)&cbi->use, UINT32_MAX, ==, puse,
> > __ATOMIC_RELAXED);
> >
> > would that help?
>
> I try to with this and it will report compiler warning:
> ' cast discards ‘const’ qualifier'.
> I think this is due to that in rte_wait_event macro, we use
> typeof(*(addr)) value = 0;
> and value is defined as "const uint32_t", but it should be able to be updated.
>
Correct a little.
The explain is for 'asm error' in the commit message.
> Furthermore, this reflects the limitations of the new macro, it cannot be
> applied when 'addr' is type of 'const'. Finally, I think I should give up the
> change for "bpf".
> >
> >
> > > > > }
> > > > > }
> > > > >
> > > > > --
> > > > > 2.25.1
>
> > -----邮件原件-----
> > 发件人: dev <dev-bounces@dpdk.org> 代表 Ananyev, Konstantin
> > 发送时间: Tuesday, October 26, 2021 8:57 PM
> > 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> > 抄送: dev@dpdk.org; nd <nd@arm.com>; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> > 主题: Re: [dpdk-dev] [PATCH v5 4/5] lib/bpf: use wait event scheme for Rx/Tx
> > iteration
> >
> >
> > > Hi Feifei,
> > >
> > > > > Instead of polling for cbi->use to be updated, use wait event scheme.
> > > > >
> > > > > Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is
> > > > > because of a compilation error:
> > > > > ------------------------------------------------------------------
> > > > > -----
> > > > > ../lib/eal/include/rte_common.h:36:13: error: read-only variable ‘value’
> > > > > used as ‘asm’ output
> > > > > 36 | #define asm __asm__
> > > > > | ^~~~~~~
> > > > >
> > > > > ../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion of
> > macro ‘asm’
> > > > > 66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
> > > > > | ^~~
> > > > >
> > > > > ../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion of
> > > > > macro ‘__LOAD_EXC_32’
> > > > > 96 | __LOAD_EXC_32((src), dst, memorder) \
> > > > > | ^~~~~~~~~~~~~
> > > > >
> > > > > ../lib/eal/arm/include/rte_pause_64.h:167:4: note: in expansion of
> > > > > macro ‘__LOAD_EXC’
> > > > > 167 | __LOAD_EXC((addr), value, memorder, size) \
> > > > > | ^~~~~~~~~~
> > > > >
> > > > > ../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro ‘rte_wait_event’
> > > > > 125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > > ------------------------------------------------------------------
> > > > > -----
> > > > >
> > > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > ---
> > > > > lib/bpf/bpf_pkt.c | 11 ++++-------
> > > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index
> > > > > 6e8248f0d6..213d44a75a 100644
> > > > > --- a/lib/bpf/bpf_pkt.c
> > > > > +++ b/lib/bpf/bpf_pkt.c
> > > > > @@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
> > > > > * Waits till datapath finished using given callback.
> > > > > */
> > > > > static void
> > > > > -bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> > > > > +bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
> > > >
> > > > Hi, Konstantin
> > > >
> > > > For this bpf patch, I delete 'const' through this is contrary to
> > > > what we discussed earlier. This is because if we keep 'constant' here and
> > use 'rte_wait_event'
> > > > new macro, compiler will report error. And earlier the arm version
> > > > cannot be compiled due to I forgot enable "wfe" config in the meson file,
> > so this issue can not happen before.
> > >
> > >
> > > Honestly, I don't understand why we have to remove perfectly valid 'const'
> > qualifier here.
> > > If this macro can't be used with pointers to const (still don't
> > > understand why), then let's just not use this macro here.
> > > Strictly speaking I don't see much benefit here from it.
> > >
> > > >
> > > > > {
> > > > > - uint32_t nuse, puse;
> > > > > + uint32_t puse;
> > > > >
> > > > > /* make sure all previous loads and stores are completed */
> > > > > rte_smp_mb();
> > > > > @@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi
> > > > > *cbi)
> > > > >
> > > > > /* in use, busy wait till current RX/TX iteration is finished */
> > > > > if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> > > > > - do {
> > > > > - rte_pause();
> > > > > - rte_compiler_barrier();
> > > > > - nuse = cbi->use;
> > > > > - } while (nuse == puse);
> > > > > + rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > > + __ATOMIC_RELAXED);
> >
> > After another thought, if we do type conversion at macro invocation time:
> >
> > bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi) {
> > ...
> > rte_wait_event((uint32_t *)&cbi->use, UINT32_MAX, ==, puse,
> > __ATOMIC_RELAXED);
> >
> > would that help?
>
> I try to with this and it will report compiler warning:
> ' cast discards ‘const’ qualifier'.
Something like:
(uint32_t *)(uintptr_t)&cbi->use
?
> I think this is due to that in rte_wait_event macro, we use
> typeof(*(addr)) value = 0;
> and value is defined as "const uint32_t",
> but it should be able to be updated.
> Furthermore, this reflects the limitations of the new macro, it cannot be applied
> when 'addr' is type of 'const'. Finally, I think I should give up the change for "bpf".
Ah yes, I see.
One trick to avoid it:
typeof (*(addr) + 0) value;
...
But it would cause integer promotion for uint16_t.
So probably wouldn't suit you here.
> -----邮件原件-----
> 发件人: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> 发送时间: Wednesday, October 27, 2021 10:48 PM
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> 主题: RE: [PATCH v5 4/5] lib/bpf: use wait event scheme for Rx/Tx iteration
>
>
>
> >
> > > -----邮件原件-----
> > > 发件人: dev <dev-bounces@dpdk.org> 代表 Ananyev, Konstantin
> > > 发送时间: Tuesday, October 26, 2021 8:57 PM
> > > 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> > > 抄送: dev@dpdk.org; nd <nd@arm.com>; Ruifeng Wang
> > > <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> > > 主题: Re: [dpdk-dev] [PATCH v5 4/5] lib/bpf: use wait event scheme for
> > > Rx/Tx iteration
> > >
> > >
> > > > Hi Feifei,
> > > >
> > > > > > Instead of polling for cbi->use to be updated, use wait event scheme.
> > > > > >
> > > > > > Furthermore, delete 'const' for 'bpf_eth_cbi_wait'. This is
> > > > > > because of a compilation error:
> > > > > > --------------------------------------------------------------
> > > > > > ----
> > > > > > -----
> > > > > > ../lib/eal/include/rte_common.h:36:13: error: read-only variable
> ‘value’
> > > > > > used as ‘asm’ output
> > > > > > 36 | #define asm __asm__
> > > > > > | ^~~~~~~
> > > > > >
> > > > > > ../lib/eal/arm/include/rte_pause_64.h:66:3: note: in expansion
> > > > > > of
> > > macro ‘asm’
> > > > > > 66 | asm volatile("ldaxr %w[tmp], [%x[addr]]" \
> > > > > > | ^~~
> > > > > >
> > > > > > ../lib/eal/arm/include/rte_pause_64.h:96:3: note: in expansion
> > > > > > of macro ‘__LOAD_EXC_32’
> > > > > > 96 | __LOAD_EXC_32((src), dst, memorder) \
> > > > > > | ^~~~~~~~~~~~~
> > > > > >
> > > > > > ../lib/eal/arm/include/rte_pause_64.h:167:4: note: in
> > > > > > expansion of macro ‘__LOAD_EXC’
> > > > > > 167 | __LOAD_EXC((addr), value, memorder, size) \
> > > > > > | ^~~~~~~~~~
> > > > > >
> > > > > > ../lib/bpf/bpf_pkt.c:125:3: note: in expansion of macro
> ‘rte_wait_event’
> > > > > > 125 | rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > > > --------------------------------------------------------------
> > > > > > ----
> > > > > > -----
> > > > > >
> > > > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > ---
> > > > > > lib/bpf/bpf_pkt.c | 11 ++++-------
> > > > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index
> > > > > > 6e8248f0d6..213d44a75a 100644
> > > > > > --- a/lib/bpf/bpf_pkt.c
> > > > > > +++ b/lib/bpf/bpf_pkt.c
> > > > > > @@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
> > > > > > * Waits till datapath finished using given callback.
> > > > > > */
> > > > > > static void
> > > > > > -bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> > > > > > +bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
> > > > >
> > > > > Hi, Konstantin
> > > > >
> > > > > For this bpf patch, I delete 'const' through this is contrary to
> > > > > what we discussed earlier. This is because if we keep
> > > > > 'constant' here and
> > > use 'rte_wait_event'
> > > > > new macro, compiler will report error. And earlier the arm
> > > > > version cannot be compiled due to I forgot enable "wfe" config
> > > > > in the meson file,
> > > so this issue can not happen before.
> > > >
> > > >
> > > > Honestly, I don't understand why we have to remove perfectly valid
> 'const'
> > > qualifier here.
> > > > If this macro can't be used with pointers to const (still don't
> > > > understand why), then let's just not use this macro here.
> > > > Strictly speaking I don't see much benefit here from it.
> > > >
> > > > >
> > > > > > {
> > > > > > - uint32_t nuse, puse;
> > > > > > + uint32_t puse;
> > > > > >
> > > > > > /* make sure all previous loads and stores are completed */
> > > > > > rte_smp_mb();
> > > > > > @@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi
> > > > > > *cbi)
> > > > > >
> > > > > > /* in use, busy wait till current RX/TX iteration is finished */
> > > > > > if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> > > > > > - do {
> > > > > > - rte_pause();
> > > > > > - rte_compiler_barrier();
> > > > > > - nuse = cbi->use;
> > > > > > - } while (nuse == puse);
> > > > > > + rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
> > > > > > + __ATOMIC_RELAXED);
> > >
> > > After another thought, if we do type conversion at macro invocation time:
> > >
> > > bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi) {
> > > ...
> > > rte_wait_event((uint32_t *)&cbi->use, UINT32_MAX, ==, puse,
> > > __ATOMIC_RELAXED);
> > >
> > > would that help?
> >
> > I try to with this and it will report compiler warning:
> > ' cast discards ‘const’ qualifier'.
>
> Something like:
> (uint32_t *)(uintptr_t)&cbi->use
> ?
I try to apply this and it is OK to fix complier warning.
Good comments and with this change I think wfe new macro
can be applied in this bpf API. Thanks.
>
> > I think this is due to that in rte_wait_event macro, we use
> > typeof(*(addr)) value = 0;
> > and value is defined as "const uint32_t", but it should be able to be
> > updated.
> > Furthermore, this reflects the limitations of the new macro, it cannot
> > be applied when 'addr' is type of 'const'. Finally, I think I should give up the
> change for "bpf".
>
> Ah yes, I see.
> One trick to avoid it:
> typeof (*(addr) + 0) value;
> ...
> But it would cause integer promotion for uint16_t.
> So probably wouldn't suit you here.
I also try with this change, it can also fix our issues. But as you say,
If *addr is uint16_t, it will large its size. It is a really good suggestion since
I'm willing to apply the last strategy.
@@ -111,9 +111,9 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
* Waits till datapath finished using given callback.
*/
static void
-bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
+bpf_eth_cbi_wait(struct bpf_eth_cbi *cbi)
{
- uint32_t nuse, puse;
+ uint32_t puse;
/* make sure all previous loads and stores are completed */
rte_smp_mb();
@@ -122,11 +122,8 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
/* in use, busy wait till current RX/TX iteration is finished */
if ((puse & BPF_ETH_CBI_INUSE) != 0) {
- do {
- rte_pause();
- rte_compiler_barrier();
- nuse = cbi->use;
- } while (nuse == puse);
+ rte_wait_event(&cbi->use, UINT32_MAX, ==, puse,
+ __ATOMIC_RELAXED);
}
}