From: Marcin Wojtas <mw@semihalf.com>
To: Louis Amas <louis.amas@eho.link>
Cc: Russell King <linux@armlinux.org.uk>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Jesper Dangaard Brouer <hawk@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
Yonghong Song <yhs@fb.com>, KP Singh <kpsingh@kernel.org>,
Emmanuel Deloget <emmanuel.deloget@eho.link>,
netdev <netdev@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
bpf@vger.kernel.org
Subject: Re: [PATCH] net: mvpp2: fix XDP rx queues registering
Date: Tue, 9 Nov 2021 15:44:18 +0100 [thread overview]
Message-ID: <CAPv3WKeHN3c_C4Y+QrAR9=BnQ_+tdhyKKAk=-o_DpyQ_6KyzAQ@mail.gmail.com> (raw)
In-Reply-To: <20211109103101.92382-1-louis.amas@eho.link>
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
next prev parent reply other threads:[~2021-11-09 14:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-09 10:31 [PATCH] net: mvpp2: fix XDP rx queues registering Louis Amas
2021-11-09 14:44 ` Marcin Wojtas [this message]
2021-11-09 17:01 ` Emmanuel Deloget
2021-11-10 14:41 ` [PATCH 1/1] " Louis Amas
2021-12-06 15:37 ` Emmanuel Deloget
2021-12-06 15:42 ` Russell King (Oracle)
2021-12-06 16:03 ` Jakub Kicinski
2021-12-06 16:16 ` John Fastabend
2021-12-06 16:45 ` Emmanuel Deloget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPv3WKeHN3c_C4Y+QrAR9=BnQ_+tdhyKKAk=-o_DpyQ_6KyzAQ@mail.gmail.com' \
--to=mw@semihalf.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=emmanuel.deloget@eho.link \
--cc=hawk@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=louis.amas@eho.link \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).