[PATCH] net: mvpp2: fix XDP rx queues registering

[PATCH] net: mvpp2: fix XDP rx queues registering

[Louis Amas]

The registration of XDP queue information is incorrect because the RX queue id we use is invalid.
When port->id == 0 it appears to works as expected yet it's no longer the case when port->id != 0.

This is due to the fact that the value we use (rxq->id) is not the per-port queue index which
XDP expects -- it's a global index which should not be used in this case. Instead we shall use
rxq->logic_rxq which is the correct, per-port value.

Signed-off-by: Louis Amas <louis.amas@eho.link>
Signed-off-by: Emmanuel Deloget <emmanuel.deloget@eho.link>
---

As we were trying to capture packets using XDP on our mv8040-powered
MacchiatoBin, we experienced an issue related to rx queue numbering.

Before I get to the problem itself, a bit of setup:

* the Macchiato has several ports, all of them handled using the mvpp2
  ethernet driver. We are able to reproduce our issue on any device whose
  port->id != 0 (we used eth2 for our tests). When port->id == 0 (for
  example on eth0) everything works as expected ;

* we use xdp-tutorial for our tests ; more specifically, we used the
  advanced03-AF_XDP tutorial as it provides a simple testbed. We modified
  the kernel to simplify it:

        SEC("xdp_sock")
        int xdp_sock_prog(struct xdp_md *ctx)
        {
                int index = ctx->rx_queue_index;

                /* A set entry here means that the correspnding queue_id
                 * has an active AF_XDP socket bound to it. */
                if (bpf_map_lookup_elem(&xsks_map, &index))
                        return bpf_redirect_map(&xsks_map, index, 0);

                return XDP_PASS;
        }

* we tested kernel 5.10 (out target) and 5.15 (for reference) ; both kernels
  exhibits the same symptoms ; I expect kernel 5.9 (the first linux kernel
  with XDP support in the mvpp2 driver) to exhibit the same problem.

The normal invocation of this program would be:

        ./af_xdp_user -d ETHDEV

We should then capture packets on this interface. When ETHDEV is eth0
(port->id == 0) everything works as expcted ; when using ETHDEV == eth2
we fail to capture anything.

We investigated the issue and found that XDP rx queues (setup as
struct xdp_rxq_info by the mvpp2 driver) for this device were wrong. XDP
expected them to be numbered in [0..3] but we found numbers in [32..35].

The reason for this lies in mvpp2_main.c at lines 2962 and 2966 which are
of the form (symbolic notation, close to actual code, function
mvpp2_rxq_init()):

        err = xdp_rxq_info_reg(&rxq->some_xdp_rxqinfo, port->dev, rxq->id, 0);

The rxq->id value we pass to this function is incorrect - it's a virtual queue
id which is computed as (symbolic notation, not actual code):

        rxq->id = port->id * max_rxq_count + queue_id

In our case, max_rxq_count == 32 and port->id == 1 for eth2, meaning our
rxq->id are in the range [32..35] (for 4 queues).

We tried to force the rx queue id on the XDP side by using:

        ./af_xdp_user -d eth2 -Q 32

But that failed -- as expected, because we should not have more than 4
rx queues.

The computing of rxq->id is valid, but the use of rxq->id in this context is
not. What we really want here is the rx queue id for this port, and this value
is stored in rxq->logic_rxq -- as hinted by the code in mvpp2_rxq_init().
Replacing rxq->id by this value in the two xdp_rxq_info_reg() calls fixed the
issue and allowed us to use XDP on all the Macchiato ethernet ports.

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 587def69a6f7..f0ea377341c6 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -2959,11 +2959,11 @@ static int mvpp2_rxq_init(struct mvpp2_port *port,
        mvpp2_rxq_status_update(port, rxq->id, 0, rxq->size);

        if (priv->percpu_pools) {
-               err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->id, 0);
+               err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->logic_rxq, 0);
                if (err < 0)
                        goto err_free_dma;

-               err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->id, 0);
+               err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->logic_rxq, 0);
                if (err < 0)
                        goto err_unregister_rxq_short;

--
2.25.1


[eho.link event] <https://www.linkedin.com/company/eho.link/>

Re: [PATCH] net: mvpp2: fix XDP rx queues registering

[Marcin Wojtas]

Hi,


wt., 9 lis 2021 o 11:31 Louis Amas <louis.amas@eho.link> napisał(a):
>
> The registration of XDP queue information is incorrect because the RX queue id we use is invalid.
> When port->id == 0 it appears to works as expected yet it's no longer the case when port->id != 0.
>
> This is due to the fact that the value we use (rxq->id) is not the per-port queue index which
> XDP expects -- it's a global index which should not be used in this case. Instead we shall use
> rxq->logic_rxq which is the correct, per-port value.
>
> Signed-off-by: Louis Amas <louis.amas@eho.link>
> Signed-off-by: Emmanuel Deloget <emmanuel.deloget@eho.link>
> ---
>
> As we were trying to capture packets using XDP on our mv8040-powered
> MacchiatoBin, we experienced an issue related to rx queue numbering.
>
> Before I get to the problem itself, a bit of setup:
>
> * the Macchiato has several ports, all of them handled using the mvpp2
>   ethernet driver. We are able to reproduce our issue on any device whose
>   port->id != 0 (we used eth2 for our tests). When port->id == 0 (for
>   example on eth0) everything works as expected ;
>
> * we use xdp-tutorial for our tests ; more specifically, we used the
>   advanced03-AF_XDP tutorial as it provides a simple testbed. We modified
>   the kernel to simplify it:
>
>         SEC("xdp_sock")
>         int xdp_sock_prog(struct xdp_md *ctx)
>         {
>                 int index = ctx->rx_queue_index;
>
>                 /* A set entry here means that the correspnding queue_id
>                  * has an active AF_XDP socket bound to it. */
>                 if (bpf_map_lookup_elem(&xsks_map, &index))
>                         return bpf_redirect_map(&xsks_map, index, 0);
>
>                 return XDP_PASS;
>         }
>
> * we tested kernel 5.10 (out target) and 5.15 (for reference) ; both kernels
>   exhibits the same symptoms ; I expect kernel 5.9 (the first linux kernel
>   with XDP support in the mvpp2 driver) to exhibit the same problem.
>
> The normal invocation of this program would be:
>
>         ./af_xdp_user -d ETHDEV
>
> We should then capture packets on this interface. When ETHDEV is eth0
> (port->id == 0) everything works as expcted ; when using ETHDEV == eth2
> we fail to capture anything.
>
> We investigated the issue and found that XDP rx queues (setup as
> struct xdp_rxq_info by the mvpp2 driver) for this device were wrong. XDP
> expected them to be numbered in [0..3] but we found numbers in [32..35].
>
> The reason for this lies in mvpp2_main.c at lines 2962 and 2966 which are
> of the form (symbolic notation, close to actual code, function
> mvpp2_rxq_init()):
>
>         err = xdp_rxq_info_reg(&rxq->some_xdp_rxqinfo, port->dev, rxq->id, 0);
>
> The rxq->id value we pass to this function is incorrect - it's a virtual queue
> id which is computed as (symbolic notation, not actual code):
>
>         rxq->id = port->id * max_rxq_count + queue_id
>
> In our case, max_rxq_count == 32 and port->id == 1 for eth2, meaning our
> rxq->id are in the range [32..35] (for 4 queues).
>
> We tried to force the rx queue id on the XDP side by using:
>
>         ./af_xdp_user -d eth2 -Q 32
>
> But that failed -- as expected, because we should not have more than 4
> rx queues.
>
> The computing of rxq->id is valid, but the use of rxq->id in this context is
> not. What we really want here is the rx queue id for this port, and this value
> is stored in rxq->logic_rxq -- as hinted by the code in mvpp2_rxq_init().
> Replacing rxq->id by this value in the two xdp_rxq_info_reg() calls fixed the
> issue and allowed us to use XDP on all the Macchiato ethernet ports.
>
> drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 587def69a6f7..f0ea377341c6 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -2959,11 +2959,11 @@ static int mvpp2_rxq_init(struct mvpp2_port *port,
>         mvpp2_rxq_status_update(port, rxq->id, 0, rxq->size);
>
>         if (priv->percpu_pools) {
> -               err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->id, 0);
> +               err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->logic_rxq, 0);
>                 if (err < 0)
>                         goto err_free_dma;
>
> -               err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->id, 0);
> +               err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->logic_rxq, 0);
>                 if (err < 0)
>                         goto err_unregister_rxq_short;
>

Thank you for the patch and the detailed explanation. I think "Fixes:"
tag should be added to the commit message:
Fixes: b27db2274ba8 ("mvpp2: use page_pool allocator")

Other than that - LGTM, so you can add my:
Reviewed-by: Marcin Wojtas <mw@semihalf.com>

Best regards,
Marcin

Re: [PATCH] net: mvpp2: fix XDP rx queues registering

[Emmanuel Deloget]

Hello, 

From: Marcin Wojtas <mw@semihalf.com>
> wt., 9 lis 2021 o 11:31 Louis Amas <louis.amas@eho.link> napisał(a):
> 
> Thank you for the patch and the detailed explanation. I think "Fixes:"
> tag should be added to the commit message:
> Fixes: b27db2274ba8 ("mvpp2: use page_pool allocator")
> 
> Other than that - LGTM, so you can add my:
> Reviewed-by: Marcin Wojtas <mw@semihalf.com>
> 
> Best regards,
> Marcin

Thank you :)

We'll send an augmented version tomorrow along with a small rework of the commit message. 

Best regards, 

-- Emmanuel Deloget

[PATCH 1/1] net: mvpp2: fix XDP rx queues registering

[Louis Amas]

The registration of XDP queue information is incorrect because the
RX queue id we use is invalid. When port->id == 0 it appears to works
as expected yet it's no longer the case when port->id != 0.

When we register the XDP rx queue information (using
xdp_rxq_info_reg() in function mvpp2_rxq_init()) we tell them to use
rxq->id as the queue id. This value iscomputed as:
rxq->id = port->id * max_rxq_count + queue_id

where max_rxq_count depends on the device version. In the MB case,
this value is 32, meaning that rx queues on eth2 are numbered from
32 to 35 - there are four of them.

Clearly, this is not the per-port queue id that XDP is expecting:
it wants a value in the range [0..3]. It shall directly use queue_id
which is stored in rxq->logic_rxq -- so let's use that value instead.

This is consistent with the remaining part of the code in
mvpp2_rxq_init().

Fixes: b27db2274ba8 ("mvpp2: use page_pool allocator")
Signed-off-by: Louis Amas <louis.amas@eho.link>
Signed-off-by: Emmanuel Deloget <emmanuel.deloget@eho.link>
Reviewed-by: Marcin Wojtas <mw@semihalf.com>
---
This is a repost of [1]. The patch itself is not changed, but the
commit message has been enhanced using part of the explaination in
order to make it clearer (hopefully) and to incorporate the
reviewed-by tag from Marcin.

v1: original patch
v2: revamped commit description (no change in the patch itself)

[1] https://lore.kernel.org/bpf/20211109103101.92382-1-louis.amas@eho.link/

 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 587def69a6f7..f0ea377341c6 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -2959,11 +2959,11 @@ static int mvpp2_rxq_init(struct mvpp2_port *port,
        mvpp2_rxq_status_update(port, rxq->id, 0, rxq->size);

        if (priv->percpu_pools) {
-               err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->id, 0);
+               err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->logic_rxq, 0);
                if (err < 0)
                        goto err_free_dma;

-               err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->id, 0);
+               err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->logic_rxq, 0);
                if (err < 0)
                        goto err_unregister_rxq_short;

--
2.25.1


[eho.link event] <https://www.linkedin.com/company/eho.link/>

Re: [PATCH 1/1] net: mvpp2: fix XDP rx queues registering

[Emmanuel Deloget]

Hello,

On 10/11/2021 15:41, Louis Amas wrote:
> The registration of XDP queue information is incorrect because the
> RX queue id we use is invalid. When port->id == 0 it appears to works
> as expected yet it's no longer the case when port->id != 0.
> 
> When we register the XDP rx queue information (using
> xdp_rxq_info_reg() in function mvpp2_rxq_init()) we tell them to use
> rxq->id as the queue id. This value iscomputed as:
> rxq->id = port->id * max_rxq_count + queue_id
> 
> where max_rxq_count depends on the device version. In the MB case,
> this value is 32, meaning that rx queues on eth2 are numbered from
> 32 to 35 - there are four of them.
> 
> Clearly, this is not the per-port queue id that XDP is expecting:
> it wants a value in the range [0..3]. It shall directly use queue_id
> which is stored in rxq->logic_rxq -- so let's use that value instead.
> 
> This is consistent with the remaining part of the code in
> mvpp2_rxq_init().
> 
> Fixes: b27db2274ba8 ("mvpp2: use page_pool allocator")
> Signed-off-by: Louis Amas <louis.amas@eho.link>
> Signed-off-by: Emmanuel Deloget <emmanuel.deloget@eho.link>
> Reviewed-by: Marcin Wojtas <mw@semihalf.com>
> ---
> This is a repost of [1]. The patch itself is not changed, but the
> commit message has been enhanced using part of the explaination in
> order to make it clearer (hopefully) and to incorporate the
> reviewed-by tag from Marcin.
> 
> v1: original patch
> v2: revamped commit description (no change in the patch itself)
> 
> [1] https://lore.kernel.org/bpf/20211109103101.92382-1-louis.amas@eho.link/
> 
>   drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 587def69a6f7..f0ea377341c6 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -2959,11 +2959,11 @@ static int mvpp2_rxq_init(struct mvpp2_port *port,
>   	mvpp2_rxq_status_update(port, rxq->id, 0, rxq->size);
>   
>   	if (priv->percpu_pools) {
> -		err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->id, 0);
> +		err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->logic_rxq, 0);
>   		if (err < 0)
>   			goto err_free_dma;
>   
> -		err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->id, 0);
> +		err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->logic_rxq, 0);
>   		if (err < 0)
>   			goto err_unregister_rxq_short;
>   
> 

Is there any update on this patch ? Without it, XDP only partially work 
on a MACCHIATOBin (read: it works on some ports, not on others, as 
described in our analysis sent together with the original patch).

Best regards,

-- Emmanuel Deloget

Re: [PATCH 1/1] net: mvpp2: fix XDP rx queues registering

[Russell King (Oracle)]

On Mon, Dec 06, 2021 at 04:37:20PM +0100, Emmanuel Deloget wrote:
> Hello,
> 
> On 10/11/2021 15:41, Louis Amas wrote:
> > The registration of XDP queue information is incorrect because the
> > RX queue id we use is invalid. When port->id == 0 it appears to works
> > as expected yet it's no longer the case when port->id != 0.
> > 
> > When we register the XDP rx queue information (using
> > xdp_rxq_info_reg() in function mvpp2_rxq_init()) we tell them to use
> > rxq->id as the queue id. This value iscomputed as:
> > rxq->id = port->id * max_rxq_count + queue_id
> > 
> > where max_rxq_count depends on the device version. In the MB case,
> > this value is 32, meaning that rx queues on eth2 are numbered from
> > 32 to 35 - there are four of them.
> > 
> > Clearly, this is not the per-port queue id that XDP is expecting:
> > it wants a value in the range [0..3]. It shall directly use queue_id
> > which is stored in rxq->logic_rxq -- so let's use that value instead.
> > 
> > This is consistent with the remaining part of the code in
> > mvpp2_rxq_init().
> > 
> > Fixes: b27db2274ba8 ("mvpp2: use page_pool allocator")
> > Signed-off-by: Louis Amas <louis.amas@eho.link>
> > Signed-off-by: Emmanuel Deloget <emmanuel.deloget@eho.link>
> > Reviewed-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> > This is a repost of [1]. The patch itself is not changed, but the
> > commit message has been enhanced using part of the explaination in
> > order to make it clearer (hopefully) and to incorporate the
> > reviewed-by tag from Marcin.
> > 
> > v1: original patch
> > v2: revamped commit description (no change in the patch itself)
> > 
> > [1] https://lore.kernel.org/bpf/20211109103101.92382-1-louis.amas@eho.link/
> > 
> >   drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > index 587def69a6f7..f0ea377341c6 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -2959,11 +2959,11 @@ static int mvpp2_rxq_init(struct mvpp2_port *port,
> >   	mvpp2_rxq_status_update(port, rxq->id, 0, rxq->size);
> >   	if (priv->percpu_pools) {
> > -		err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->id, 0);
> > +		err = xdp_rxq_info_reg(&rxq->xdp_rxq_short, port->dev, rxq->logic_rxq, 0);
> >   		if (err < 0)
> >   			goto err_free_dma;
> > -		err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->id, 0);
> > +		err = xdp_rxq_info_reg(&rxq->xdp_rxq_long, port->dev, rxq->logic_rxq, 0);
> >   		if (err < 0)
> >   			goto err_unregister_rxq_short;
> > 
> 
> Is there any update on this patch ? Without it, XDP only partially work on a
> MACCHIATOBin (read: it works on some ports, not on others, as described in
> our analysis sent together with the original patch).

Hi,

I suspect if you *didn't* thread your updated patch to your previous
submission, then it would end up with a separate entry in
patchwork.kernel.org, and the netdev maintainers will notice that the
patch is ready for inclusion, having been reviewed by Marcin.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/1] net: mvpp2: fix XDP rx queues registering

[Jakub Kicinski]

On Mon, 6 Dec 2021 15:42:47 +0000 Russell King (Oracle) wrote:
> On Mon, Dec 06, 2021 at 04:37:20PM +0100, Emmanuel Deloget wrote:
> > On 10/11/2021 15:41, Louis Amas wrote:  
> > > The registration of XDP queue information is incorrect because the
> > > RX queue id we use is invalid. When port->id == 0 it appears to works
> > > as expected yet it's no longer the case when port->id != 0.
> > > 
> > > When we register the XDP rx queue information (using
> > > xdp_rxq_info_reg() in function mvpp2_rxq_init()) we tell them to use
> > > rxq->id as the queue id. This value iscomputed as:
> > > rxq->id = port->id * max_rxq_count + queue_id
> > > 
> > > where max_rxq_count depends on the device version. In the MB case,
> > > this value is 32, meaning that rx queues on eth2 are numbered from
> > > 32 to 35 - there are four of them.
> > > 
> > > Clearly, this is not the per-port queue id that XDP is expecting:
> > > it wants a value in the range [0..3]. It shall directly use queue_id
> > > which is stored in rxq->logic_rxq -- so let's use that value instead.
> > > 
> > > This is consistent with the remaining part of the code in
> > > mvpp2_rxq_init().

> > Is there any update on this patch ? Without it, XDP only partially work on a
> > MACCHIATOBin (read: it works on some ports, not on others, as described in
> > our analysis sent together with the original patch).  
> 
> I suspect if you *didn't* thread your updated patch to your previous
> submission, then it would end up with a separate entry in
> patchwork.kernel.org,

Indeed, it's easier to keep track of patches which weren't posted 
as a reply in a thread, at least for me.

> and the netdev maintainers will notice that the
> patch is ready for inclusion, having been reviewed by Marcin.

In this case I _think_ it was dropped because it didn't apply.

Please rebase on top of net/master and repost if the changes is still
needed.

Re: [PATCH 1/1] net: mvpp2: fix XDP rx queues registering

[John Fastabend]

Jakub Kicinski wrote:
> On Mon, 6 Dec 2021 15:42:47 +0000 Russell King (Oracle) wrote:
> > On Mon, Dec 06, 2021 at 04:37:20PM +0100, Emmanuel Deloget wrote:
> > > On 10/11/2021 15:41, Louis Amas wrote:  
> > > > The registration of XDP queue information is incorrect because the
> > > > RX queue id we use is invalid. When port->id == 0 it appears to works
> > > > as expected yet it's no longer the case when port->id != 0.
> > > > 
> > > > When we register the XDP rx queue information (using
> > > > xdp_rxq_info_reg() in function mvpp2_rxq_init()) we tell them to use
> > > > rxq->id as the queue id. This value iscomputed as:
> > > > rxq->id = port->id * max_rxq_count + queue_id
> > > > 
> > > > where max_rxq_count depends on the device version. In the MB case,
> > > > this value is 32, meaning that rx queues on eth2 are numbered from
> > > > 32 to 35 - there are four of them.
> > > > 
> > > > Clearly, this is not the per-port queue id that XDP is expecting:
> > > > it wants a value in the range [0..3]. It shall directly use queue_id
> > > > which is stored in rxq->logic_rxq -- so let's use that value instead.
> > > > 
> > > > This is consistent with the remaining part of the code in
> > > > mvpp2_rxq_init().
> 
> > > Is there any update on this patch ? Without it, XDP only partially work on a
> > > MACCHIATOBin (read: it works on some ports, not on others, as described in
> > > our analysis sent together with the original patch).  
> > 
> > I suspect if you *didn't* thread your updated patch to your previous
> > submission, then it would end up with a separate entry in
> > patchwork.kernel.org,
> 
> Indeed, it's easier to keep track of patches which weren't posted 
> as a reply in a thread, at least for me.
> 
> > and the netdev maintainers will notice that the
> > patch is ready for inclusion, having been reviewed by Marcin.
> 
> In this case I _think_ it was dropped because it didn't apply.
> 
> Please rebase on top of net/master and repost if the changes is still
> needed.

Also I would add the detailed description to the actual commit not below
the "--" lines. Capturing that in the log will be useful for future
reference if we ever hit similar issue here or elsewhere.

Otherwise for patch,

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH 1/1] net: mvpp2: fix XDP rx queues registering

[Emmanuel Deloget]

John,

On 06/12/2021 17:16, John Fastabend wrote:
> Jakub Kicinski wrote:
>> On Mon, 6 Dec 2021 15:42:47 +0000 Russell King (Oracle) wrote:
>>> On Mon, Dec 06, 2021 at 04:37:20PM +0100, Emmanuel Deloget wrote:
>>>> On 10/11/2021 15:41, Louis Amas wrote:
>>>>> The registration of XDP queue information is incorrect because the
>>>>> RX queue id we use is invalid. When port->id == 0 it appears to works
>>>>> as expected yet it's no longer the case when port->id != 0.
>>>>>
>>>>> When we register the XDP rx queue information (using
>>>>> xdp_rxq_info_reg() in function mvpp2_rxq_init()) we tell them to use
>>>>> rxq->id as the queue id. This value iscomputed as:
>>>>> rxq->id = port->id * max_rxq_count + queue_id
>>>>>
>>>>> where max_rxq_count depends on the device version. In the MB case,
>>>>> this value is 32, meaning that rx queues on eth2 are numbered from
>>>>> 32 to 35 - there are four of them.
>>>>>
>>>>> Clearly, this is not the per-port queue id that XDP is expecting:
>>>>> it wants a value in the range [0..3]. It shall directly use queue_id
>>>>> which is stored in rxq->logic_rxq -- so let's use that value instead.
>>>>>
>>>>> This is consistent with the remaining part of the code in
>>>>> mvpp2_rxq_init().
>>
>>>> Is there any update on this patch ? Without it, XDP only partially work on a
>>>> MACCHIATOBin (read: it works on some ports, not on others, as described in
>>>> our analysis sent together with the original patch).
>>>
>>> I suspect if you *didn't* thread your updated patch to your previous
>>> submission, then it would end up with a separate entry in
>>> patchwork.kernel.org,
>>
>> Indeed, it's easier to keep track of patches which weren't posted
>> as a reply in a thread, at least for me.
>>
>>> and the netdev maintainers will notice that the
>>> patch is ready for inclusion, having been reviewed by Marcin.
>>
>> In this case I _think_ it was dropped because it didn't apply.
>>
>> Please rebase on top of net/master and repost if the changes is still
>> needed.
> 
> Also I would add the detailed description to the actual commit not below
> the "--" lines. Capturing that in the log will be useful for future
> reference if we ever hit similar issue here or elsewhere.
> 
> Otherwise for patch,
> 
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> 

Ouch. We failed to get this email before resending the patch.

My bad - and sorry for the inconvenience.

I'll see with Louis to change again the commit message and add your 
Acked-by.

Best regards,

-- Emmanuel Deloget