| Message ID | 1e8e82f72e46264b7a7a1ac704d24e163ebed100.1599165031.git.lorenzo@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | mvneta: introduce XDP multi-buffer support | expand |
On Thu, Sep 03, 2020 at 10:58:45PM +0200, Lorenzo Bianconi wrote: > Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify > if shared_info area has been properly initialized for non-linear > xdp buffers > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > include/net/xdp.h | 8 ++++++-- > net/core/xdp.c | 1 + > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/net/xdp.h b/include/net/xdp.h > index 3814fb631d52..42f439f9fcda 100644 > --- a/include/net/xdp.h > +++ b/include/net/xdp.h > @@ -72,7 +72,8 @@ struct xdp_buff { > void *data_hard_start; > struct xdp_rxq_info *rxq; > struct xdp_txq_info *txq; > - u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/ > + u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/ > + u32 mb:1; /* xdp non-linear buffer */ > }; > > /* Reserve memory area at end-of data area. > @@ -96,7 +97,8 @@ struct xdp_frame { > u16 len; > u16 headroom; > u32 metasize:8; > - u32 frame_sz:24; > + u32 frame_sz:23; > + u32 mb:1; /* xdp non-linear frame */ Hmm. Last time I checked compilers were generating ugly code with bitfields. Not performant and not efficient. frame_sz is used in the fast path. I suspect the first hunk alone will cause performance degradation. Could you use normal u8 or u32 flag field?
On Thu, 3 Sep 2020 18:07:05 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Thu, Sep 03, 2020 at 10:58:45PM +0200, Lorenzo Bianconi wrote: > > Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify > > if shared_info area has been properly initialized for non-linear > > xdp buffers > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > include/net/xdp.h | 8 ++++++-- > > net/core/xdp.c | 1 + > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/include/net/xdp.h b/include/net/xdp.h > > index 3814fb631d52..42f439f9fcda 100644 > > --- a/include/net/xdp.h > > +++ b/include/net/xdp.h > > @@ -72,7 +72,8 @@ struct xdp_buff { > > void *data_hard_start; > > struct xdp_rxq_info *rxq; > > struct xdp_txq_info *txq; > > - u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/ > > + u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/ > > + u32 mb:1; /* xdp non-linear buffer */ > > }; > > > > /* Reserve memory area at end-of data area. > > @@ -96,7 +97,8 @@ struct xdp_frame { > > u16 len; > > u16 headroom; > > u32 metasize:8; > > - u32 frame_sz:24; > > + u32 frame_sz:23; > > + u32 mb:1; /* xdp non-linear frame */ > > Hmm. Last time I checked compilers were generating ugly code with bitfields. > Not performant and not efficient. > frame_sz is used in the fast path. > I suspect the first hunk alone will cause performance degradation. > Could you use normal u8 or u32 flag field? For struct xdp_buff sure we can do this. For struct xdp_frame, I'm not sure, as it is a state compressed version of xdp_buff + extra information. The xdp_frame have been called skb-light, and I know people (e.g Ahern) wants to add more info to this, vlan, RX-hash, csum, and we must keep this to 1-cache-line, for performance reasons. You do make a good point, that these bit-fields might hurt performance more. I guess, we need to test this. As I constantly worry that we will slowly kill XDP performance with a 1000 paper-cuts.
On 9/4/20 1:19 AM, Jesper Dangaard Brouer wrote: > On Thu, 3 Sep 2020 18:07:05 -0700 > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > >> On Thu, Sep 03, 2020 at 10:58:45PM +0200, Lorenzo Bianconi wrote: >>> Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify >>> if shared_info area has been properly initialized for non-linear >>> xdp buffers >>> >>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >>> --- >>> include/net/xdp.h | 8 ++++++-- >>> net/core/xdp.c | 1 + >>> 2 files changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/net/xdp.h b/include/net/xdp.h >>> index 3814fb631d52..42f439f9fcda 100644 >>> --- a/include/net/xdp.h >>> +++ b/include/net/xdp.h >>> @@ -72,7 +72,8 @@ struct xdp_buff { >>> void *data_hard_start; >>> struct xdp_rxq_info *rxq; >>> struct xdp_txq_info *txq; >>> - u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/ >>> + u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/ >>> + u32 mb:1; /* xdp non-linear buffer */ >>> }; >>> >>> /* Reserve memory area at end-of data area. >>> @@ -96,7 +97,8 @@ struct xdp_frame { >>> u16 len; >>> u16 headroom; >>> u32 metasize:8; >>> - u32 frame_sz:24; >>> + u32 frame_sz:23; >>> + u32 mb:1; /* xdp non-linear frame */ >> >> Hmm. Last time I checked compilers were generating ugly code with bitfields. >> Not performant and not efficient. >> frame_sz is used in the fast path. >> I suspect the first hunk alone will cause performance degradation. >> Could you use normal u8 or u32 flag field? > > For struct xdp_buff sure we can do this. For struct xdp_frame, I'm not > sure, as it is a state compressed version of xdp_buff + extra > information. The xdp_frame have been called skb-light, and I know > people (e.g Ahern) wants to add more info to this, vlan, RX-hash, csum, > and we must keep this to 1-cache-line, for performance reasons. > > You do make a good point, that these bit-fields might hurt performance > more. I guess, we need to test this. As I constantly worry that we > will slowly kill XDP performance with a 1000 paper-cuts. > That struct is tight on space, and we have to be very smart about additions. dev_rx for example seems like it could just be the netdev index rather than a pointer or perhaps can be removed completely. I believe it is only used for 1 use case (redirects to CPUMAP); maybe that code can be refactored to handle the dev outside of xdp_frame. xdp_mem_info is 2 u32's; the type in that struct really could be a u8. In this case it means removing struct in favor of 2 elements to reclaim the space, but as we reach the 64B limit this is a place to change. e.g., make it a single u32 with the id only 24 bits though the rhashtable key can stay u32 but now with the combined type + id. As for frame_sz, why does it need to be larger than a u16? If it really needs to be larger than u16, there are several examples of using a bit (or bits) in the data path. dst metrics for examples uses lowest 4 bits of the dst pointer as a bitfield. It does so using a mask with accessors vs a bitfield. Perhaps that is the way to go here.
On Fri, 4 Sep 2020 09:15:04 -0600 David Ahern <dsahern@gmail.com> wrote: > On 9/4/20 1:19 AM, Jesper Dangaard Brouer wrote: > > On Thu, 3 Sep 2020 18:07:05 -0700 > > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > >> On Thu, Sep 03, 2020 at 10:58:45PM +0200, Lorenzo Bianconi wrote: > >>> Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify > >>> if shared_info area has been properly initialized for non-linear > >>> xdp buffers > >>> > >>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > >>> --- > >>> include/net/xdp.h | 8 ++++++-- > >>> net/core/xdp.c | 1 + > >>> 2 files changed, 7 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/include/net/xdp.h b/include/net/xdp.h > >>> index 3814fb631d52..42f439f9fcda 100644 > >>> --- a/include/net/xdp.h > >>> +++ b/include/net/xdp.h > >>> @@ -72,7 +72,8 @@ struct xdp_buff { > >>> void *data_hard_start; > >>> struct xdp_rxq_info *rxq; > >>> struct xdp_txq_info *txq; > >>> - u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/ > >>> + u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/ > >>> + u32 mb:1; /* xdp non-linear buffer */ > >>> }; > >>> > >>> /* Reserve memory area at end-of data area. > >>> @@ -96,7 +97,8 @@ struct xdp_frame { > >>> u16 len; > >>> u16 headroom; > >>> u32 metasize:8; > >>> - u32 frame_sz:24; > >>> + u32 frame_sz:23; > >>> + u32 mb:1; /* xdp non-linear frame */ > >> > >> Hmm. Last time I checked compilers were generating ugly code with bitfields. > >> Not performant and not efficient. > >> frame_sz is used in the fast path. > >> I suspect the first hunk alone will cause performance degradation. > >> Could you use normal u8 or u32 flag field? > > > > For struct xdp_buff sure we can do this. For struct xdp_frame, I'm not > > sure, as it is a state compressed version of xdp_buff + extra > > information. The xdp_frame have been called skb-light, and I know > > people (e.g Ahern) wants to add more info to this, vlan, RX-hash, csum, > > and we must keep this to 1-cache-line, for performance reasons. > > > > You do make a good point, that these bit-fields might hurt performance > > more. I guess, we need to test this. As I constantly worry that we > > will slowly kill XDP performance with a 1000 paper-cuts. > > > > That struct is tight on space, and we have to be very smart about > additions. I fully agree. > dev_rx for example seems like it could just be the netdev > index rather than a pointer or perhaps can be removed completely. I > believe it is only used for 1 use case (redirects to CPUMAP); maybe that > code can be refactored to handle the dev outside of xdp_frame. The dev_rx is needed when creating an SKB from a xdp_frame (basically skb->dev = rx_dev). Yes, that is done in cpumap, but I want to generalize this. The veth also creates SKBs from xdp_frame, but use itself as skb->dev. And yes, we could save some space storing the index instead, and trade space for cycles in a lookup. > xdp_mem_info is 2 u32's; the type in that struct really could be a u8. Yes, I have floated a patch that did this earlier, but it was never merged, as it was part of storing the xdp_mem_info in the SKB to create a return path for page_pool pages. > In this case it means removing struct in favor of 2 elements to reclaim > the space, but as we reach the 64B limit this is a place to change. > e.g., make it a single u32 with the id only 24 bits though the > rhashtable key can stay u32 but now with the combined type + id. > > As for frame_sz, why does it need to be larger than a u16? Because PAGE_SIZE can be 64KiB on some archs. > If it really needs to be larger than u16, there are several examples of > using a bit (or bits) in the data path. dst metrics for examples uses > lowest 4 bits of the dst pointer as a bitfield. It does so using a mask > with accessors vs a bitfield. Perhaps that is the way to go here.
On 9/4/20 9:59 AM, Jesper Dangaard Brouer wrote: >> dev_rx for example seems like it could just be the netdev >> index rather than a pointer or perhaps can be removed completely. I >> believe it is only used for 1 use case (redirects to CPUMAP); maybe that >> code can be refactored to handle the dev outside of xdp_frame. > > The dev_rx is needed when creating an SKB from a xdp_frame (basically > skb->dev = rx_dev). Yes, that is done in cpumap, but I want to > generalize this. The veth also creates SKBs from xdp_frame, but use > itself as skb->dev. > > And yes, we could save some space storing the index instead, and trade > space for cycles in a lookup. I think this can be managed without adding a reference to the xdp_frame. I'll start a separate thread on that. >> >> As for frame_sz, why does it need to be larger than a u16? > > Because PAGE_SIZE can be 64KiB on some archs. > ok, is there any alignment requirement? can frame_sz be number of 32-bit words? I believe bit shifts are cheap.
On Fri, 4 Sep 2020 10:30:48 -0600 David Ahern <dsahern@gmail.com> wrote: > On 9/4/20 9:59 AM, Jesper Dangaard Brouer wrote: > >> dev_rx for example seems like it could just be the netdev > >> index rather than a pointer or perhaps can be removed completely. I > >> believe it is only used for 1 use case (redirects to CPUMAP); maybe that > >> code can be refactored to handle the dev outside of xdp_frame. > > > > The dev_rx is needed when creating an SKB from a xdp_frame (basically > > skb->dev = rx_dev). Yes, that is done in cpumap, but I want to > > generalize this. The veth also creates SKBs from xdp_frame, but use > > itself as skb->dev. > > > > And yes, we could save some space storing the index instead, and trade > > space for cycles in a lookup. > > I think this can be managed without adding a reference to the xdp_frame. > I'll start a separate thread on that. > > >> > >> As for frame_sz, why does it need to be larger than a u16? > > > > Because PAGE_SIZE can be 64KiB on some archs. > > I also believe syzbot managed to create packets for generic-XDP with frame_sz 128KiB, which was a bit weird (it's on my todo list to investigate and fix). > ok, is there any alignment requirement? can frame_sz be number of 32-bit > words? I believe bit shifts are cheap. No that is not possible, because some drivers and generic-XDP have a fully dynamic frame_sz.
On 9/7/20 12:02 PM, Jesper Dangaard Brouer wrote: > >> ok, is there any alignment requirement? can frame_sz be number of 32-bit >> words? I believe bit shifts are cheap. > > No that is not possible, because some drivers and generic-XDP have a > fully dynamic frame_sz. > frame_sz represents allocated memory right? What is the real range that needs to be supported for frame_sz? Surely there is some upper limit, and I thought it was 64kB. Allocated memory will not be on an odd number, so fair to assume at a minimum it is a multiple of 2. correct? At a minimum we should be able to shift frame_sz by 1 which now covers 64kB in a u16.
diff --git a/include/net/xdp.h b/include/net/xdp.h index 3814fb631d52..42f439f9fcda 100644 --- a/include/net/xdp.h +++ b/include/net/xdp.h @@ -72,7 +72,8 @@ struct xdp_buff { void *data_hard_start; struct xdp_rxq_info *rxq; struct xdp_txq_info *txq; - u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/ + u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/ + u32 mb:1; /* xdp non-linear buffer */ }; /* Reserve memory area at end-of data area. @@ -96,7 +97,8 @@ struct xdp_frame { u16 len; u16 headroom; u32 metasize:8; - u32 frame_sz:24; + u32 frame_sz:23; + u32 mb:1; /* xdp non-linear frame */ /* Lifetime of xdp_rxq_info is limited to NAPI/enqueue time, * while mem info is valid on remote CPU. */ @@ -141,6 +143,7 @@ void xdp_convert_frame_to_buff(struct xdp_frame *frame, struct xdp_buff *xdp) xdp->data_end = frame->data + frame->len; xdp->data_meta = frame->data - frame->metasize; xdp->frame_sz = frame->frame_sz; + xdp->mb = frame->mb; } static inline @@ -167,6 +170,7 @@ int xdp_update_frame_from_buff(struct xdp_buff *xdp, xdp_frame->headroom = headroom - sizeof(*xdp_frame); xdp_frame->metasize = metasize; xdp_frame->frame_sz = xdp->frame_sz; + xdp_frame->mb = xdp->mb; return 0; } diff --git a/net/core/xdp.c b/net/core/xdp.c index 48aba933a5a8..884f140fc3be 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -454,6 +454,7 @@ struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp) xdpf->headroom = 0; xdpf->metasize = metasize; xdpf->frame_sz = PAGE_SIZE; + xdpf->mb = xdp->mb; xdpf->mem.type = MEM_TYPE_PAGE_ORDER0; xsk_buff_free(xdp);
Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify if shared_info area has been properly initialized for non-linear xdp buffers Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- include/net/xdp.h | 8 ++++++-- net/core/xdp.c | 1 + 2 files changed, 7 insertions(+), 2 deletions(-)