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

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

[Benoit Ganne (bganne)]

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

[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

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

[Magnus Karlsson]

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

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

[Benoit Ganne (bganne)]

Hi Magnus,

>> 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 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).
[...]

> 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.

Thanks! Sure I will in the future.
If I may, an ideal scenario would be:
 1) the concurrent rx/tx usecase is officially supported, and 11cc2d21499cabe7e7964389634ed1de3ee91d33 should be backported to LTS
 2) had a way to detect different version of AF_XDP (through an ioctl() or getsockopt() or...) so that I can detect more easily what I should do at init time. Eg. managing both headroom (0 and 256) for copy mode is not very difficult if I can detect it easily

Best
ben

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

[Magnus Karlsson]

On Wed, May 12, 2021 at 10:26 AM Benoit Ganne (bganne) <bganne@cisco.com> wrote:
>
> Hi Magnus,
>
> >> 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 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).
> [...]
>
> > 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.
>
> Thanks! Sure I will in the future.
> If I may, an ideal scenario would be:
>  1) the concurrent rx/tx usecase is officially supported, and 11cc2d21499cabe7e7964389634ed1de3ee91d33 should be backported to LTS

I will fix this. The (unintentional) fix you refer to should apply
cleanly to 5.4 and 4.19 LTS kernels. I just have to verify this and
submit a request to Greg. Probably with a new commit message that
describes what problem it fixes, since it is not just a simplification
anymore.

>  2) had a way to detect different version of AF_XDP (through an ioctl() or getsockopt() or...) so that I can detect more easily what I should do at init time. Eg. managing both headroom (0 and 256) for copy mode is not very difficult if I can detect it easily

Björn and I have yet to come up with a good way to detect this that
does not involve receiving a packet. Another possible way would be to
backport a fix for this to 5.4 and 4.19 LTS too. But this seems to be
a less straightforward patch, so let us see what that would entail.
Björn is working on this and will get back to you.

>
> Best
> ben