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

[Maryam Tahhan <mtahhan@redhat.com>]

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 :)

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

> 
> * 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.
> 
>> 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;
>>   }
>>
>>
>