On Wed, May 24, 2023 at 7:12 AM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Wed, May 24, 2023 at 3:27 AM Maciej Fijalkowski > <maciej.fijalkowski@xxxxxxxxx> wrote: > > > > On Wed, May 24, 2023 at 10:56:21AM +0200, Sarkar, Tirthendu wrote: > > > > -----Original Message----- > > > > From: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> > > > > Sent: Friday, May 19, 2023 10:44 PM > > > > To: Stanislav Fomichev <sdf@xxxxxxxxxx> > > > > Cc: Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx>; bpf > > > > <bpf@xxxxxxxxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>; Daniel > > > > Borkmann <daniel@xxxxxxxxxxxxx>; Andrii Nakryiko <andrii@xxxxxxxxxx>; > > > > Network Development <netdev@xxxxxxxxxxxxxxx>; Karlsson, Magnus > > > > <magnus.karlsson@xxxxxxxxx>; Sarkar, Tirthendu > > > > <tirthendu.sarkar@xxxxxxxxx>; Björn Töpel <bjorn@xxxxxxxxxx> > > > > Subject: Re: [PATCH bpf-next 01/21] xsk: prepare 'options' in xdp_desc for > > > > multi-buffer use > > > > > > > > On Thu, May 18, 2023 at 12:22 PM Stanislav Fomichev <sdf@xxxxxxxxxx> > > > > wrote: > > > > > > > > > > On 05/18, Maciej Fijalkowski wrote: > > > > > > From: Tirthendu Sarkar <tirthendu.sarkar@xxxxxxxxx> > > > > > > > > > > > > Use the 'options' field in xdp_desc as a packet continuity marker. Since > > > > > > 'options' field was unused till now and was expected to be set to 0, the > > > > > > 'eop' descriptor will have it set to 0, while the non-eop descriptors > > > > > > will have to set it to 1. This ensures legacy applications continue to > > > > > > work without needing any change for single-buffer packets. > > > > > > > > > > > > Add helper functions and extend xskq_prod_reserve_desc() to use the > > > > > > 'options' field. > > > > > > > > > > > > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@xxxxxxxxx> > > > > > > --- > > > > > > include/uapi/linux/if_xdp.h | 16 ++++++++++++++++ > > > > > > net/xdp/xsk.c | 8 ++++---- > > > > > > net/xdp/xsk_queue.h | 12 +++++++++--- > > > > > > 3 files changed, 29 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h > > > > > > index a78a8096f4ce..4acc3a9430f3 100644 > > > > > > --- a/include/uapi/linux/if_xdp.h > > > > > > +++ b/include/uapi/linux/if_xdp.h > > > > > > @@ -108,4 +108,20 @@ struct xdp_desc { > > > > > > > > > > > > /* UMEM descriptor is __u64 */ > > > > > > > > > > > > +/* Flag indicating that the packet continues with the buffer pointed out > > > > by the > > > > > > + * next frame in the ring. The end of the packet is signalled by setting > > > > this > > > > > > + * bit to zero. For single buffer packets, every descriptor has 'options' > > > > set > > > > > > + * to 0 and this maintains backward compatibility. > > > > > > + */ > > > > > > +#define XDP_PKT_CONTD (1 << 0) > > > > > > + > > > > > > +/* Maximum number of descriptors supported as frags for a packet. So > > > > the total > > > > > > + * number of descriptors supported for a packet is > > > > XSK_DESC_MAX_FRAGS + 1. The > > > > > > + * max frags supported by skb is 16 for page sizes greater than 4K and 17 > > > > or > > > > > > > > > > This is now a config option CONFIG_MAX_SKB_FRAGS. Can we use it > > > > > directly? > > > > > > > > Also it doesn't look right to expose kernel internal config in uapi > > > > especially since XSK_DESC_MAX_FRAGS is not guaranteed to be 16. > > > > > > Ok, we have couple of options here: > > > > > > Option 1: We will define XSK_DESC_MAX_FRAGS to 17 now. This will ensure AF_XDP > > > applications will work on any system without any change since the MAX_SKB_FRAGS > > > is guaranteed to be at least 17. > > > > > > Option 2: Instead of defining a new macro, we say max frags supported is same as > > > MAX_SKB_FRAGS as configured in your system. So use 17 or less frags if you want > > > your app to work everywhere but you can go larger if you control the system. > > > > > > Any suggestions ? > > > > > > Also Alexei could you please clarify what you meant by ".. since XSK_DESC_MAX_FRAGS > > > is not guaranteed to be 16." ? > > > > Maybe it would be better to put this define onto patch 08 so people would > > see how it is used and get a feeling of it? Although it has a description > > nothing says about it in commit message. > > > > FWIW i'm voting for option 2, but also Alexei's comment is a bit unclear > > to me, would be nice to hear more about it. > > Meaning that uapi can only have fixed constants. > We cannot put *_MAX_FRAGS there, since it's config dependent. Same here, would prefer option 2. And don't put it in the uapi. That's something the users can try to probe maybe?