| Message ID | eef58418ab78408f4a5fbd3d3b0071f30ece2ccd.1617885385.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | mvneta: introduce XDP multi-buffer support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 6 maintainers not CCed: yhs@fb.com kpsingh@kernel.org hawk@kernel.org andrii@kernel.org kafai@fb.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 6680 this patch: 6680 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 41 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6893 this patch: 6893 |
| netdev/header_inline | success | Link |
On Thu, Apr 08, 2021 at 02:50:53PM +0200, Lorenzo Bianconi wrote: > Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure > in order to specify if this is a linear buffer (mb = 0) or a multi-buffer > frame (mb = 1). In the latter case the shared_info area at the end of the > first buffer will be properly initialized to link together subsequent > buffers. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > include/net/xdp.h | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/include/net/xdp.h b/include/net/xdp.h > index a5bc214a49d9..842580a61563 100644 > --- a/include/net/xdp.h > +++ b/include/net/xdp.h > @@ -73,7 +73,10 @@ 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 > + */ This comment would have fit just fine on one line: /* frame size to deduce data_hard_end/reserved tailroom */ > + u32 mb:1; /* xdp non-linear buffer */ > }; > > static __always_inline void > @@ -81,6 +84,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq) > { > xdp->frame_sz = frame_sz; > xdp->rxq = rxq; > + xdp->mb = 0; > } > > static __always_inline void > @@ -116,7 +120,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. > */ > @@ -179,6 +184,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 > @@ -205,6 +211,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; > } > -- > 2.30.2 >
> On Thu, Apr 08, 2021 at 02:50:53PM +0200, Lorenzo Bianconi wrote: > > Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure > > in order to specify if this is a linear buffer (mb = 0) or a multi-buffer > > frame (mb = 1). In the latter case the shared_info area at the end of the > > first buffer will be properly initialized to link together subsequent > > buffers. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > include/net/xdp.h | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/include/net/xdp.h b/include/net/xdp.h > > index a5bc214a49d9..842580a61563 100644 > > --- a/include/net/xdp.h > > +++ b/include/net/xdp.h > > @@ -73,7 +73,10 @@ 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 > > + */ > > This comment would have fit just fine on one line: > > /* frame size to deduce data_hard_end/reserved tailroom */ ack, thx I will fix it in v9 Regards, Lorenzo > > > + u32 mb:1; /* xdp non-linear buffer */ > > }; > > > > static __always_inline void > > @@ -81,6 +84,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq) > > { > > xdp->frame_sz = frame_sz; > > xdp->rxq = rxq; > > + xdp->mb = 0; > > } > > > > static __always_inline void > > @@ -116,7 +120,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. > > */ > > @@ -179,6 +184,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 > > @@ -205,6 +211,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; > > } > > -- > > 2.30.2 > >
On Thu, 8 Apr 2021 14:50:53 +0200 Lorenzo Bianconi <lorenzo@kernel.org> wrote: > Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure > in order to specify if this is a linear buffer (mb = 0) or a multi-buffer > frame (mb = 1). In the latter case the shared_info area at the end of the > first buffer will be properly initialized to link together subsequent > buffers. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > include/net/xdp.h | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/include/net/xdp.h b/include/net/xdp.h > index a5bc214a49d9..842580a61563 100644 > --- a/include/net/xdp.h > +++ b/include/net/xdp.h > @@ -73,7 +73,10 @@ 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 */ > }; > > static __always_inline void > @@ -81,6 +84,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq) > { > xdp->frame_sz = frame_sz; > xdp->rxq = rxq; > + xdp->mb = 0; > } > > static __always_inline void > @@ -116,7 +120,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. > */ So, it seems that these bitfield's are the root-cause of the performance regression. Credit to Alexei whom wisely already point this out[1] in V2 ;-) [1] https://lore.kernel.org/netdev/20200904010705.jm6dnuyj3oq4cpjd@ast-mbp.dhcp.thefacebook.com/ > @@ -179,6 +184,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 > @@ -205,6 +211,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; > }
> > static __always_inline void > > @@ -116,7 +120,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. > > */ > > So, it seems that these bitfield's are the root-cause of the > performance regression. Credit to Alexei whom wisely already point > this out[1] in V2 ;-) > > [1] https://lore.kernel.org/netdev/20200904010705.jm6dnuyj3oq4cpjd@ast-mbp.dhcp.thefacebook.com/ yes, shame on me..yesterday I recalled email from Alexei debugging the issue reported by Magnus. In the current approach I am testing (not posted upstream yet) I reduced the size of xdp_mem_info as proposed by Jesper in [0] and I added a flags field in xdp_frame/xdp_buff we can use for multiple features (e.g. multi-buff or hw csum hints). Doing so, running xdp_rxq_info sample on ixgbe 10Gbps NIC I do not have any performance regressions for xdp_tx or xdp_drop. Same results have been reported by Magnus off-list on i40e (we have a 1% regression on xdp_sock tests iiuc). I will continue working on this. Regards, Lorenzo [0] https://patchwork.kernel.org/project/netdevbpf/patch/20210409223801.104657-2-mcroce@linux.microsoft.com/ > > > > @@ -179,6 +184,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 > > @@ -205,6 +211,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; > > } > > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer >
diff --git a/include/net/xdp.h b/include/net/xdp.h index a5bc214a49d9..842580a61563 100644 --- a/include/net/xdp.h +++ b/include/net/xdp.h @@ -73,7 +73,10 @@ 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 */ }; static __always_inline void @@ -81,6 +84,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq) { xdp->frame_sz = frame_sz; xdp->rxq = rxq; + xdp->mb = 0; } static __always_inline void @@ -116,7 +120,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. */ @@ -179,6 +184,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 @@ -205,6 +211,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; }
Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure in order to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer will be properly initialized to link together subsequent buffers. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- include/net/xdp.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)