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
