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

[Marcin Wojtas]

Hi,


wt., 9 lis 2021 o 11:31 Louis Amas <louis.amas@xxxxxxxx> 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@xxxxxxxx>
> Signed-off-by: Emmanuel Deloget <emmanuel.deloget@xxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxx>

Best regards,
Marcin