Re: [PATCH RFCv2 bpf-next 17/18] xsk: AF_XDP xdp-hints support in desc options

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

On 08/09/2022 12.10, Maryam Tahhan wrote:
> On 08/09/2022 09:06, Magnus Karlsson wrote:
>> On Wed, Sep 7, 2022 at 5:48 PM Jesper Dangaard Brouer 
>> <brouer@redhat.com> wrote:
>>>
>>> From: Maryam Tahhan <mtahhan@redhat.com>
>>>
>>> Simply set AF_XDP descriptor options to XDP flags.
>>>
>>> Jesper: Will this really be acceptable by AF_XDP maintainers?
>>
>> Maryam, you guessed correctly that dedicating all these options bits
>> for a single feature will not be ok :-). E.g., I want one bit for the
>> AF_XDP multi-buffer support and who knows what other uses there might
>> be for this options field in the future. Let us try to solve this in
>> some other way. Here are some suggestions, all with their pros and
>> cons.
>>
> 
> TBH it was Jespers question :)

True. I'm generally questioning this patch...
... and indirectly asking Magnus.  (If you noticed, I didn't add my SoB)

>> * Put this feature flag at a known place in the metadata area, for
>> example just before the BTF ID. No need to fill this in if you are not
>> redirecting to AF_XDP, but at a redirect to AF_XDP, the XDP flags are
>> copied into this u32 in the metadata area so that user-space can
>> consume it. Will cost 4 bytes of the metadata area though.
> 
> If Jesper agrees I think this approach would make sense. Trying to
> translate encodings into some other flags for AF_XDP I think will lead
> to a growing set of translations as more options come along.
> The other thing to be aware of is just making sure to clear/zero the 
> metadata space in the buffers at some point (ideally when the descriptor 
> is returned from the application) so when the buffers are used again
> they are already in a "reset" state.

I don't like this option ;-)

First of all because this can give false positives, if "XDP flags copied
into metadata area" is used for something else.  This can easily happen
as XDP BPF-progs are free to metadata for something else.

Second reason, because it would require AF_XDP to always read the
metadata cache-line (and write, if clearing on "return").  Not a good
optioon, given how performance sensitive AF_XDP workloads (at least
benchmarks).

>>
>> * Instead encode this information into each metadata entry in the
>> metadata area, in some way so that a flags field is not needed (-1
>> signifies not valid, or whatever happens to make sense). This has the
>> drawback that the user might have to look at a large number of entries
>> just to find out there is nothing valid to read. To alleviate this, it
>> could be combined with the next suggestion.
>>
>> * Dedicate one bit in the options field to indicate that there is at
>> least one valid metadata entry in the metadata area. This could be
>> combined with the two approaches above. However, depending on what
>> metadata you have enabled, this bit might be pointless. If some
>> metadata is always valid, then it serves no purpose. But it might if
>> all enabled metadata is rarely valid, e.g., if you get an Rx timestamp
>> on one packet out of one thousand.
>>

I like this option better! Except that I have hoped to get 2 bits ;-)

The performance advantage is that the AF_XDP descriptor bits will 
already be cache-hot, and if it indicates no-metadata-hints the AF_XDP 
application can avoid reading the metadata cache-line :-).

When metadata is valid and contains valid XDP-hints can change between 
two packets.  E.g. XDP-hints can be enabled/disabled via ethtool, and 
the content can be enabled/disabled by other ethtool commands, and even 
setsockopt calls (e.g timestamping).  An XDP prog can also choose to use 
the area for something else for a subset of the packets.

It is a design choice in this patchset to avoid locking down the NIC 
driver to a fixed XDP-hints layout, and avoid locking/disabling other 
ethtool config setting to keeping XDP-hints layout stable.  Originally I 
wanted this, but I realized that it would be impossible (and annoying 
for users) if we had to control every config interface to NIC hardware 
offload hints, to keep XDP-hints "always-valid".

--Jesper

>>> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
>>> ---
>>>   include/uapi/linux/if_xdp.h |    2 +-
>>>   net/xdp/xsk.c               |    2 +-
>>>   net/xdp/xsk_queue.h         |    3 ++-
>>>   3 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
>>> index a78a8096f4ce..9335b56474e7 100644
>>> --- a/include/uapi/linux/if_xdp.h
>>> +++ b/include/uapi/linux/if_xdp.h
>>> @@ -103,7 +103,7 @@ struct xdp_options {
>>>   struct xdp_desc {
>>>          __u64 addr;
>>>          __u32 len;
>>> -       __u32 options;
>>> +       __u32 options; /* set to the values of xdp_hints_flags*/
>>>   };
>>>
>>>   /* UMEM descriptor is __u64 */
>>> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
>>> index 5b4ce6ba1bc7..32095d78f06b 100644
>>> --- a/net/xdp/xsk.c
>>> +++ b/net/xdp/xsk.c
>>> @@ -141,7 +141,7 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, 
>>> struct xdp_buff *xdp, u32 len)
>>>          int err;
>>>
>>>          addr = xp_get_handle(xskb);
>>> -       err = xskq_prod_reserve_desc(xs->rx, addr, len);
>>> +       err = xskq_prod_reserve_desc(xs->rx, addr, len, xdp->flags);
>>>          if (err) {
>>>                  xs->rx_queue_full++;
>>>                  return err;
>>> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
>>> index fb20bf7207cf..7a66f082f97e 100644
>>> --- a/net/xdp/xsk_queue.h
>>> +++ b/net/xdp/xsk_queue.h
>>> @@ -368,7 +368,7 @@ static inline u32 
>>> xskq_prod_reserve_addr_batch(struct xsk_queue *q, struct xdp_d
>>>   }
>>>
>>>   static inline int xskq_prod_reserve_desc(struct xsk_queue *q,
>>> -                                        u64 addr, u32 len)
>>> +                                        u64 addr, u32 len, u32 flags)
>>>   {
>>>          struct xdp_rxtx_ring *ring = (struct xdp_rxtx_ring *)q->ring;
>>>          u32 idx;
>>> @@ -380,6 +380,7 @@ static inline int xskq_prod_reserve_desc(struct 
>>> xsk_queue *q,
>>>          idx = q->cached_prod++ & q->ring_mask;
>>>          ring->desc[idx].addr = addr;
>>>          ring->desc[idx].len = len;
>>> +       ring->desc[idx].options = flags;
>>>
>>>          return 0;
>>>   }
>>>
>>>
>>
>