| Message ID | cover.1611086134.git.lorenzo@kernel.org |
|---|---|
| Headers | show |
| Series | mvneta: introduce XDP multi-buffer support | expand |
Hi Lorenzo, On 1/19/21 9:20 PM, Lorenzo Bianconi wrote: > This series introduce XDP multi-buffer support. The mvneta driver is > the first to support these new "non-linear" xdp_{buff,frame}. Reviewers > please focus on how these new types of xdp_{buff,frame} packets > traverse the different layers and the layout design. It is on purpose > that BPF-helpers are kept simple, as we don't want to expose the > internal layout to allow later changes. > > For now, to keep the design simple and to maintain performance, the XDP > BPF-prog (still) only have access to the first-buffer. It is left for > later (another patchset) to add payload access across multiple buffers. I think xmas break has mostly wiped my memory from 2020 ;) so it would be good to describe the sketched out design for how this will look like inside the cover letter in terms of planned uapi exposure. (Additionally discussing api design proposal could also be sth for BPF office hour to move things quicker + posting a summary to the list for transparency of course .. just a thought.) Glancing over the series, while you've addressed the bpf_xdp_adjust_tail() helper API, this series will be breaking one assumption of programs at least for the mvneta driver from one kernel to another if you then use the multi buff mode, and that is basically bpf_xdp_event_output() API: the assumption is that you can do full packet capture by passing in the xdp buff len that is data_end - data ptr. We use it this way for sampling & others might as well (e.g. xdpcap). But bpf_xdp_copy() would only copy the first buffer today which would break the full pkt visibility assumption. Just walking the frags if xdp->mb bit is set would still need some sort of struct xdp_md exposure so the prog can figure out the actual full size.. > This patchset should still allow for these future extensions. The goal > is to lift the XDP MTU restriction that comes with XDP, but maintain > same performance as before. > > The main idea for the new multi-buffer layout is to reuse the same > layout used for non-linear SKB. We introduced a "xdp_shared_info" data > structure at the end of the first buffer to link together subsequent buffers. > xdp_shared_info will alias skb_shared_info allowing to keep most of the frags > in the same cache-line (while with skb_shared_info only the first fragment will > be placed in the first "shared_info" cache-line). Moreover we introduced some > xdp_shared_info helpers aligned to skb_frag* ones. > Converting xdp_frame to SKB and deliver it to the network stack is shown in > cpumap code (patch 7/8). Building the SKB, the xdp_shared_info structure > will be converted in a skb_shared_info one. > > A multi-buffer bit (mb) has been introduced in xdp_{buff,frame} structure > to notify the bpf/network layer if this is a xdp multi-buffer frame (mb = 1) > or not (mb = 0). > The mb bit will be set by a xdp multi-buffer capable driver only for > non-linear frames maintaining the capability to receive linear frames > without any extra cost since the xdp_shared_info structure at the end > of the first buffer will be initialized only if mb is set. > > Typical use cases for this series are: > - Jumbo-frames > - Packet header split (please see Google’s use-case @ NetDevConf 0x14, [0]) > - TSO > > bpf_xdp_adjust_tail helper has been modified to take info account xdp > multi-buff frames. Also in terms of logistics (I think mentioned earlier already), for the series to be merged - as with other networking features spanning core + driver (example af_xdp) - we also need a second driver (ideally mlx5, i40e or ice) implementing this and ideally be submitted together in the same series for review. For that it probably also makes sense to more cleanly split out the core pieces from the driver ones. Either way, how is progress on that side coming along? Thanks, Daniel
> Hi Lorenzo, Hi Daniel, sorry for the delay. > > On 1/19/21 9:20 PM, Lorenzo Bianconi wrote: > > This series introduce XDP multi-buffer support. The mvneta driver is > > the first to support these new "non-linear" xdp_{buff,frame}. Reviewers > > please focus on how these new types of xdp_{buff,frame} packets > > traverse the different layers and the layout design. It is on purpose > > that BPF-helpers are kept simple, as we don't want to expose the > > internal layout to allow later changes. > > > > For now, to keep the design simple and to maintain performance, the XDP > > BPF-prog (still) only have access to the first-buffer. It is left for > > later (another patchset) to add payload access across multiple buffers. > > I think xmas break has mostly wiped my memory from 2020 ;) so it would be > good to describe the sketched out design for how this will look like inside > the cover letter in terms of planned uapi exposure. (Additionally discussing > api design proposal could also be sth for BPF office hour to move things > quicker + posting a summary to the list for transparency of course .. just > a thought.) I guess the main goal of this series is to add the multi-buffer support to the xdp core (e.g. in xdp_frame/xdp_buff or in xdp_return_{buff/frame}) and to provide the first driver with xdp mult-ibuff support. We tried to make the changes independent from eBPF helpers since we do not have defined use cases for them yet and we don't want to expose the internal layout to allow later changes. One possible example is bpf_xdp_adjust_mb_header() helper we sent in v2 patch 6/9 [0] to try to address use-case explained by Eric @ NetDev 0x14 [1]. Anyway I agree there are some missing bits we need to address (e.g. what is the behaviour when we redirect a mb xdp_frame to a driver not supporting it?) Ack, I agree we can discuss about mb eBPF helper APIs in BPF office hour mtg in order to speed-up the process. > > Glancing over the series, while you've addressed the bpf_xdp_adjust_tail() > helper API, this series will be breaking one assumption of programs at least > for the mvneta driver from one kernel to another if you then use the multi > buff mode, and that is basically bpf_xdp_event_output() API: the assumption > is that you can do full packet capture by passing in the xdp buff len that > is data_end - data ptr. We use it this way for sampling & others might as well > (e.g. xdpcap). But bpf_xdp_copy() would only copy the first buffer today which > would break the full pkt visibility assumption. Just walking the frags if > xdp->mb bit is set would still need some sort of struct xdp_md exposure so > the prog can figure out the actual full size.. ack, thx for pointing this out, I will take a look to it. Eelco added xdp_len to xdp_md in the previous series (he is still working on it). Another possible approach would be defining a helper, what do you think? > > > This patchset should still allow for these future extensions. The goal > > is to lift the XDP MTU restriction that comes with XDP, but maintain > > same performance as before. > > > > The main idea for the new multi-buffer layout is to reuse the same > > layout used for non-linear SKB. We introduced a "xdp_shared_info" data > > structure at the end of the first buffer to link together subsequent buffers. > > xdp_shared_info will alias skb_shared_info allowing to keep most of the frags > > in the same cache-line (while with skb_shared_info only the first fragment will > > be placed in the first "shared_info" cache-line). Moreover we introduced some > > xdp_shared_info helpers aligned to skb_frag* ones. > > Converting xdp_frame to SKB and deliver it to the network stack is shown in > > cpumap code (patch 7/8). Building the SKB, the xdp_shared_info structure > > will be converted in a skb_shared_info one. > > > > A multi-buffer bit (mb) has been introduced in xdp_{buff,frame} structure > > to notify the bpf/network layer if this is a xdp multi-buffer frame (mb = 1) > > or not (mb = 0). > > The mb bit will be set by a xdp multi-buffer capable driver only for > > non-linear frames maintaining the capability to receive linear frames > > without any extra cost since the xdp_shared_info structure at the end > > of the first buffer will be initialized only if mb is set. > > > > Typical use cases for this series are: > > - Jumbo-frames > > - Packet header split (please see Google’s use-case @ NetDevConf 0x14, [0]) > > - TSO > > > > bpf_xdp_adjust_tail helper has been modified to take info account xdp > > multi-buff frames. > > Also in terms of logistics (I think mentioned earlier already), for the series to > be merged - as with other networking features spanning core + driver (example > af_xdp) - we also need a second driver (ideally mlx5, i40e or ice) implementing > this and ideally be submitted together in the same series for review. For that > it probably also makes sense to more cleanly split out the core pieces from the > driver ones. Either way, how is progress on that side coming along? I do not have any updated news about it so far, but afaik amazon folks were working on adding mb support to ena driver, while intel was planning to add it to af_xdp. Moreover Jason was looking to add it to virtio-net. > > Thanks, > Daniel Regards, Lorenzo [0] https://patchwork.ozlabs.org/project/netdev/patch/b7475687bb09aac6ec051596a8ccbb311a54cb8a.1599165031.git.lorenzo@kernel.org/ [1] https://netdevconf.info/0x14/session.html?talk-the-path-to-tcp-4k-mtu-and-rx-zerocopy
> -----Original Message----- > From: Lorenzo Bianconi <lorenzo@kernel.org> > Sent: Sunday, January 31, 2021 7:24 PM > To: Daniel Borkmann <daniel@iogearbox.net> > Cc: bpf@vger.kernel.org; netdev@vger.kernel.org; > lorenzo.bianconi@redhat.com; davem@davemloft.net; kuba@kernel.org; > ast@kernel.org; Agroskin, Shay <shayagr@amazon.com>; > john.fastabend@gmail.com; dsahern@kernel.org; brouer@redhat.com; > echaudro@redhat.com; jasowang@redhat.com; > alexander.duyck@gmail.com; saeed@kernel.org; > maciej.fijalkowski@intel.com; Jubran, Samih <sameehj@amazon.com> > Subject: RE: [EXTERNAL] [PATCH v6 bpf-next 0/8] mvneta: introduce XDP > multi-buffer support > > > Hi Lorenzo, > > Hi Daniel, > > sorry for the delay. > > > > > On 1/19/21 9:20 PM, Lorenzo Bianconi wrote: > > > This series introduce XDP multi-buffer support. The mvneta driver is > > > the first to support these new "non-linear" xdp_{buff,frame}. > > > Reviewers please focus on how these new types of xdp_{buff,frame} > > > packets traverse the different layers and the layout design. It is > > > on purpose that BPF-helpers are kept simple, as we don't want to > > > expose the internal layout to allow later changes. > > > > > > For now, to keep the design simple and to maintain performance, the > > > XDP BPF-prog (still) only have access to the first-buffer. It is > > > left for later (another patchset) to add payload access across multiple > buffers. > > > > I think xmas break has mostly wiped my memory from 2020 ;) so it would > > be good to describe the sketched out design for how this will look > > like inside the cover letter in terms of planned uapi exposure. > > (Additionally discussing api design proposal could also be sth for BPF > > office hour to move things quicker + posting a summary to the list for > > transparency of course .. just a thought.) > > I guess the main goal of this series is to add the multi-buffer support to the > xdp core (e.g. in xdp_frame/xdp_buff or in xdp_return_{buff/frame}) and to > provide the first driver with xdp mult-ibuff support. We tried to make the > changes independent from eBPF helpers since we do not have defined use > cases for them yet and we don't want to expose the internal layout to allow > later changes. > One possible example is bpf_xdp_adjust_mb_header() helper we sent in v2 > patch 6/9 [0] to try to address use-case explained by Eric @ NetDev 0x14 [1]. > Anyway I agree there are some missing bits we need to address (e.g. what is > the behaviour when we redirect a mb xdp_frame to a driver not supporting > it?) > > Ack, I agree we can discuss about mb eBPF helper APIs in BPF office hour mtg > in order to speed-up the process. > > > > > Glancing over the series, while you've addressed the > > bpf_xdp_adjust_tail() helper API, this series will be breaking one > > assumption of programs at least for the mvneta driver from one kernel > > to another if you then use the multi buff mode, and that is basically > > bpf_xdp_event_output() API: the assumption is that you can do full > > packet capture by passing in the xdp buff len that is data_end - data > > ptr. We use it this way for sampling & others might as well (e.g. > > xdpcap). But bpf_xdp_copy() would only copy the first buffer today > > which would break the full pkt visibility assumption. Just walking the > > frags if > > xdp->mb bit is set would still need some sort of struct xdp_md > > xdp->exposure so > > the prog can figure out the actual full size.. > > ack, thx for pointing this out, I will take a look to it. > Eelco added xdp_len to xdp_md in the previous series (he is still working on > it). Another possible approach would be defining a helper, what do you > think? > > > > > > This patchset should still allow for these future extensions. The > > > goal is to lift the XDP MTU restriction that comes with XDP, but > > > maintain same performance as before. > > > > > > The main idea for the new multi-buffer layout is to reuse the same > > > layout used for non-linear SKB. We introduced a "xdp_shared_info" > > > data structure at the end of the first buffer to link together subsequent > buffers. > > > xdp_shared_info will alias skb_shared_info allowing to keep most of > > > the frags in the same cache-line (while with skb_shared_info only > > > the first fragment will be placed in the first "shared_info" > > > cache-line). Moreover we introduced some xdp_shared_info helpers > aligned to skb_frag* ones. > > > Converting xdp_frame to SKB and deliver it to the network stack is > > > shown in cpumap code (patch 7/8). Building the SKB, the > > > xdp_shared_info structure will be converted in a skb_shared_info one. > > > > > > A multi-buffer bit (mb) has been introduced in xdp_{buff,frame} > > > structure to notify the bpf/network layer if this is a xdp > > > multi-buffer frame (mb = 1) or not (mb = 0). > > > The mb bit will be set by a xdp multi-buffer capable driver only for > > > non-linear frames maintaining the capability to receive linear > > > frames without any extra cost since the xdp_shared_info structure at > > > the end of the first buffer will be initialized only if mb is set. > > > > > > Typical use cases for this series are: > > > - Jumbo-frames > > > - Packet header split (please see Google’s use-case @ NetDevConf > > > 0x14, [0]) > > > - TSO > > > > > > bpf_xdp_adjust_tail helper has been modified to take info account > > > xdp multi-buff frames. > > > > Also in terms of logistics (I think mentioned earlier already), for > > the series to be merged - as with other networking features spanning > > core + driver (example > > af_xdp) - we also need a second driver (ideally mlx5, i40e or ice) > > implementing this and ideally be submitted together in the same series > > for review. For that it probably also makes sense to more cleanly > > split out the core pieces from the driver ones. Either way, how is progress > on that side coming along? > > I do not have any updated news about it so far, but afaik amazon folks were > working on adding mb support to ena driver, while intel was planning to add > it to af_xdp. Hi all, The ENA XDP MB implementation is currently being rebased on top of this series. We are in final stages of polishing and testing the code and looking forward to send it for review, hopefully till the end of March. > Moreover Jason was looking to add it to virtio-net. > > > > > Thanks, > > Daniel > > Regards, > Lorenzo > Best regards, Sameeh > [0] > https://patchwork.ozlabs.org/project/netdev/patch/b7475687bb09aac6ec05 > 1596a8ccbb311a54cb8a.1599165031.git.lorenzo@kernel.org/ > [1] https://netdevconf.info/0x14/session.html?talk-the-path-to-tcp-4k-mtu- > and-rx-zerocopy
This series introduce XDP multi-buffer support. The mvneta driver is the first to support these new "non-linear" xdp_{buff,frame}. Reviewers please focus on how these new types of xdp_{buff,frame} packets traverse the different layers and the layout design. It is on purpose that BPF-helpers are kept simple, as we don't want to expose the internal layout to allow later changes. For now, to keep the design simple and to maintain performance, the XDP BPF-prog (still) only have access to the first-buffer. It is left for later (another patchset) to add payload access across multiple buffers. This patchset should still allow for these future extensions. The goal is to lift the XDP MTU restriction that comes with XDP, but maintain same performance as before. The main idea for the new multi-buffer layout is to reuse the same layout used for non-linear SKB. We introduced a "xdp_shared_info" data structure at the end of the first buffer to link together subsequent buffers. xdp_shared_info will alias skb_shared_info allowing to keep most of the frags in the same cache-line (while with skb_shared_info only the first fragment will be placed in the first "shared_info" cache-line). Moreover we introduced some xdp_shared_info helpers aligned to skb_frag* ones. Converting xdp_frame to SKB and deliver it to the network stack is shown in cpumap code (patch 7/8). Building the SKB, the xdp_shared_info structure will be converted in a skb_shared_info one. A multi-buffer bit (mb) has been introduced in xdp_{buff,frame} structure to notify the bpf/network layer if this is a xdp multi-buffer frame (mb = 1) or not (mb = 0). The mb bit will be set by a xdp multi-buffer capable driver only for non-linear frames maintaining the capability to receive linear frames without any extra cost since the xdp_shared_info structure at the end of the first buffer will be initialized only if mb is set. Typical use cases for this series are: - Jumbo-frames - Packet header split (please see Google’s use-case @ NetDevConf 0x14, [0]) - TSO bpf_xdp_adjust_tail helper has been modified to take info account xdp multi-buff frames. More info about the main idea behind this approach can be found here [1][2]. Changes since v5: - rebase on top of bpf-next - initialize mb bit in xdp_init_buff() and drop per-driver initialization - drop xdp->mb initialization in xdp_convert_zc_to_xdp_frame() - postpone introduction of frame_length field in XDP ctx to another series - minor changes Changes since v4: - rebase ontop of bpf-next - introduce xdp_shared_info to build xdp multi-buff instead of using the skb_shared_info struct - introduce frame_length in xdp ctx - drop previous bpf helpers - fix bpf_xdp_adjust_tail for xdp multi-buff - introduce xdp multi-buff self-tests for bpf_xdp_adjust_tail - fix xdp_return_frame_bulk for xdp multi-buff Changes since v3: - rebase ontop of bpf-next - add patch 10/13 to copy back paged data from a xdp multi-buff frame to userspace buffer for xdp multi-buff selftests Changes since v2: - add throughput measurements - drop bpf_xdp_adjust_mb_header bpf helper - introduce selftest for xdp multibuffer - addressed comments on bpf_xdp_get_frags_count - introduce xdp multi-buff support to cpumaps Changes since v1: - Fix use-after-free in xdp_return_{buff/frame} - Introduce bpf helpers - Introduce xdp_mb sample program - access skb_shared_info->nr_frags only on the last fragment Changes since RFC: - squash multi-buffer bit initialization in a single patch - add mvneta non-linear XDP buff support for tx side [0] https://netdevconf.info/0x14/session.html?talk-the-path-to-tcp-4k-mtu-and-rx-zerocopy [1] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org [2] https://netdevconf.info/0x14/session.html?tutorial-add-XDP-support-to-a-NIC-driver (XDPmulti-buffers section) Eelco Chaudron (1): bpf: add multi-buff support to the bpf_xdp_adjust_tail() API Lorenzo Bianconi (7): xdp: introduce mb in xdp_buff/xdp_frame xdp: add xdp_shared_info data structure net: mvneta: update mb bit before passing the xdp buffer to eBPF layer xdp: add multi-buff support to xdp_return_{buff/frame} net: mvneta: add multi buffer support to XDP_TX net: mvneta: enable jumbo frames for XDP net: xdp: add multi-buff support to xdp_build_skb_from_fram drivers/net/ethernet/marvell/mvneta.c | 182 +++++++++++++++----------- include/net/xdp.h | 88 +++++++++++-- net/core/filter.c | 63 +++++++++ net/core/xdp.c | 102 ++++++++++++++- 4 files changed, 349 insertions(+), 86 deletions(-)