Re: AF_XDP poll() / sendmsg() race + headroom changes

From: Magnus Karlsson <magnus.karlsson@gmail.com>
To: "Benoit Ganne (bganne)" <bganne@cisco.com>
Cc: "bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: Re: AF_XDP poll() / sendmsg() race + headroom changes
Date: Wed, 12 May 2021 07:35:14 +0200	[thread overview]
Message-ID: <CAJ8uoz16WLwqP+=dtphm7KWh=c9QYiU25k33hNrAg8ciaGe9vw@mail.gmail.com> (raw)
In-Reply-To: <BYAPR11MB365382C5DB1E5FCC53242609C1549@BYAPR11MB3653.namprd11.prod.outlook.com>

On Mon, May 10, 2021 at 7:53 PM Benoit Ganne (bganne) <bganne@cisco.com> wrote:
>
> Hi everyone,
>
> Please CC me as I am not subscribed to the list.
> I am the maintainer of the AF_XDP driver for VPP, an open-source userspace networking stack, and I ran into an issue recently with kernels < 5.6 (including LTS kernel 5.4 which is shipped in eg. Ubuntu 20.04 LTS): it seems like one cannot call poll() and sendmsg() concurrently on the same AF_XDP socket. Is this a supported usecase? I know the rings are single producer/single consumer and I use them like this, but can I have one thread doing RX (and poll) while another thread is doing TX (and sendmsg)?
> A typical usecase is when using more processing threads than AF_XDP queues for an interface, eg. because I use several interfaces: each thread can poll its own set of RX queues from different NICs, but depending of the packet processing decisions, I must be able to send through any other interface - hence TX queues can be shared. In this case they are protected with a lock, but rx and tx can still happen in parallel.
> The problem has been fixed with commit 11cc2d21499cabe7e7964389634ed1de3ee91d33 "xsk: Simplify detection of empty and full rings" [1] but it looks like pure luck.
> From what I can see the issue stems that prior to this patch, poll() will update the cached txq prod_tail while sendmsg() is running and doing the same and because of that the txq cons_head can moved back, causing AF_XDP to process the same descriptor twice.
>
> I hit a 2nd issue with kernel >= 5.9, where the headroom on rx for copy mode has grown from 0 to XDP_PACKET_HEADROOM (256-bytes). This change of behavior was introduced by commit 2b43470add8c8ff1e1ee28dffc5c5df97e955d09 "xsk: Introduce AF_XDP buffer allocation API" [2].
> Previously, the headroom in copy mode was set to "configured headroom + 0" whereas the headroom in 0-copy mode was set to "configured headroom + XDP_PACKET_HEADROOM". This patch changed copy mode headroom to "configured headroom + XDP_PACKET_HEADROOM", identical to 0-copy.
> I agree the previous behavior was a bit weird, but is there a way to detect old vs new behavior? Otherwise it is difficult to run the same code before/after this patch.
>
> Thanks in advance for your help,
> ben

Hi Benoit. Thank you for reporting, I will take a look at this and get
back to you. Next time, please add me and Björn on the to line so that
you get a quicker response.

> [1] https://lore.kernel.org/bpf/1576759171-28550-3-git-send-email-magnus.karlsson@intel.com/
> [2] https://lore.kernel.org/bpf/20200520192103.355233-6-bjorn.topel@gmail.com