From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:50:34
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. This rely on the "skb_shared_info"
struct at the end of the first buffer to link together subsequent
buffers. Keeping the layout compatible with SKBs is also done to ease
and speedup creating an SKB from an xdp_{buff,frame}.
Converting xdp_frame to SKB and deliver it to the network stack is shown
in patch 07/14 (e.g. cpumaps).
A multi-buffer bit (mb) has been introduced in the flags field of 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 skb_shared_info structure at the end
of the first buffer will be initialized only if mb is set.
Moreover the flags field in xdp_{buff,frame} will be reused even for
xdp rx csum offloading in future series.
Typical use cases for this series are:
- Jumbo-frames
- Packet header split (please see Googleâs use-case @ NetDevConf 0x14, [0])
- TSO
A new bpf helper (bpf_xdp_get_buff_len) has been introduce in order to notify
the eBPF layer about the total frame size (linear + paged parts).
bpf_xdp_adjust_tail and bpf_xdp_copy helpers have been modified to take into
account xdp multi-buff frames.
More info about the main idea behind this approach can be found here [1][2].
Changes since v8:
- add proper dma unmapping if XDP_TX fails on mvneta for a xdp multi-buff
- switch back to skb_shared_info implementation from previous xdp_shared_info
one
- avoid using a bietfield in xdp_buff/xdp_frame since it introduces performance
regressions. Tested now on 10G NIC (ixgbe) to verify there are no performance
penalties for regular codebase
- add bpf_xdp_get_buff_len helper and remove frame_length field in xdp ctx
- add data_len field in skb_shared_info struct
Changes since v7:
- rebase on top of bpf-next
- fix sparse warnings
- improve comments for frame_length in include/net/xdp.h
Changes since v6:
- the main difference respect to previous versions is the new approach proposed
by Eelco to pass full length of the packet to eBPF layer in XDP context
- reintroduce multi-buff support to eBPF kself-tests
- reintroduce multi-buff support to bpf_xdp_adjust_tail helper
- introduce multi-buffer support to bpf_xdp_copy helper
- rebase on top of bpf-next
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 (3):
bpf: add multi-buff support to the bpf_xdp_adjust_tail() API
bpf: add multi-buffer support to xdp copy helpers
bpf: update xdp_adjust_tail selftest to include multi-buffer
Lorenzo Bianconi (11):
net: skbuff: add data_len field to skb_shared_info
xdp: introduce flags field in xdp_buff/xdp_frame
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_frame
bpf: introduce bpf_xdp_get_buff_len helper
bpf: move user_size out of bpf_test_init
bpf: introduce multibuff support to bpf_prog_test_run_xdp()
bpf: test_run: add xdp_shared_info pointer in bpf_test_finish
signature
drivers/net/ethernet/marvell/mvneta.c | 143 ++++++++++------
include/linux/skbuff.h | 5 +-
include/net/xdp.h | 56 ++++++-
include/uapi/linux/bpf.h | 7 +
kernel/trace/bpf_trace.c | 3 +
net/bpf/test_run.c | 108 +++++++++---
net/core/filter.c | 157 +++++++++++++++++-
net/core/xdp.c | 72 +++++++-
tools/include/uapi/linux/bpf.h | 7 +
.../bpf/prog_tests/xdp_adjust_tail.c | 105 ++++++++++++
.../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 127 +++++++++-----
.../bpf/progs/test_xdp_adjust_tail_grow.c | 10 +-
.../bpf/progs/test_xdp_adjust_tail_shrink.c | 32 +++-
.../selftests/bpf/progs/test_xdp_bpf2bpf.c | 2 +-
14 files changed, 705 insertions(+), 129 deletions(-)
--
2.31.1
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:50:40
data_len field will be used for paged frame len for xdp_buff/xdp_frame.
This is a preliminary patch to properly support xdp-multibuff
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
include/linux/skbuff.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:50:43
Introduce flags field in xdp_frame/xdp_buffer data structure
to define additional buffer features. At the moment the only
supported buffer feature is multi-buffer bit (mb). Multi-buffer bit
is used 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 | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
@@ -74,13 +78,30 @@ struct xdp_buff {structxdp_rxq_info*rxq;structxdp_txq_info*txq;u32frame_sz;/* frame size to deduce data_hard_end/reserved tailroom*/+u16flags;/* supported values defined in xdp_flags */};+static__always_inlineboolxdp_buff_is_mb(structxdp_buff*xdp)+{+return!!(xdp->flags&XDP_FLAGS_MULTI_BUFF);+}++static__always_inlinevoidxdp_buff_set_mb(structxdp_buff*xdp)+{+xdp->flags|=XDP_FLAGS_MULTI_BUFF;+}++static__always_inlinevoidxdp_buff_clear_mb(structxdp_buff*xdp)+{+xdp->flags&=~XDP_FLAGS_MULTI_BUFF;+}+static__always_inlinevoidxdp_init_buff(structxdp_buff*xdp,u32frame_sz,structxdp_rxq_info*rxq){xdp->frame_sz=frame_sz;xdp->rxq=rxq;+xdp->flags=0;}static__always_inlinevoid
@@ -117,13 +138,19 @@ struct xdp_frame {u16headroom;u32metasize:8;u32frame_sz:24;+structnet_device*dev_rx;/* used by cpumap *//* Lifetime of xdp_rxq_info is limited to NAPI/enqueue time,*whilememinfoisvalidonremoteCPU.*/structxdp_mem_infomem;-structnet_device*dev_rx;/* used by cpumap */+u16flags;/* supported values defined in xdp_flags */};+static__always_inlineboolxdp_frame_is_mb(structxdp_frame*frame)+{+return!!(frame->flags&XDP_FLAGS_MULTI_BUFF);+}+#define XDP_BULK_QUEUE_SIZE 16structxdp_frame_bulk{intcount;
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:50:48
Update multi-buffer bit (mb) in xdp_buff to notify XDP/eBPF layer and
XDP remote drivers if this is a "non-linear" XDP buffer. Access
xdp_shared_info only if xdp_buff mb is set.
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/ethernet/marvell/mvneta.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
@@ -2304,12 +2305,18 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,skb_frag_size_set(frag,data_len);__skb_frag_set_page(frag,page);+if(!xdp_buff_is_mb(xdp)){+xdp_sinfo->data_len=*size;+xdp_buff_set_mb(xdp);+}/* last fragment */if(len==*size){structskb_shared_info*sinfo;sinfo=xdp_get_shared_info_from_buff(xdp);sinfo->nr_frags=xdp_sinfo->nr_frags;+sinfo->data_len=xdp_sinfo->data_len;+memcpy(sinfo->frags,xdp_sinfo->frags,sinfo->nr_frags*sizeof(skb_frag_t));}
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:50:52
Take into account if the received xdp_buff/xdp_frame is non-linear
recycling/returning the frame memory to the allocator or into
xdp_frame_bulk.
Introduce xdp_return_num_frags_from_buff to return a given number of
fragments from a xdp multi-buff starting from the tail.
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
include/net/xdp.h | 18 ++++++++++++++--
net/core/xdp.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 69 insertions(+), 3 deletions(-)
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:51:01
Enable the capability to receive jumbo frames even if the interface is
running in XDP mode
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/ethernet/marvell/mvneta.c | 10 ----------
1 file changed, 10 deletions(-)
@@ -3780,11 +3780,6 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu)mtu=ALIGN(MVNETA_RX_PKT_SIZE(mtu),8);}-if(pp->xdp_prog&&mtu>MVNETA_MAX_RX_BUF_SIZE){-netdev_info(dev,"Illegal MTU value %d for XDP mode\n",mtu);-return-EINVAL;-}-dev->mtu=mtu;if(!netif_running(dev)){
@@ -4486,11 +4481,6 @@ static int mvneta_xdp_setup(struct net_device *dev, struct bpf_prog *prog,structmvneta_port*pp=netdev_priv(dev);structbpf_prog*old_prog;-if(prog&&dev->mtu>MVNETA_MAX_RX_BUF_SIZE){-NL_SET_ERR_MSG_MOD(extack,"MTU too large for XDP");-return-EOPNOTSUPP;-}-if(pp->bm_priv){NL_SET_ERR_MSG_MOD(extack,"Hardware Buffer Management not supported on XDP");
@@ -4778,6 +4778,12 @@ union bpf_attr {*ExecuteclosesyscallforgivenFD.*Return*Asyscallresult.+*+*intbpf_xdp_get_buff_len(structxdp_buff*xdp_md)+*Description+*Getthetotalsizeofagivenxdpbuff(linearandpagedarea)+*Return+*Thetotalsizeofagivenxdpbuffer.*/#define __BPF_FUNC_MAPPER(FN) \FN(unspec),\
@@ -4949,6 +4955,7 @@ union bpf_attr {FN(sys_bpf),\FN(btf_find_by_name_kind),\FN(sys_close),\+FN(xdp_get_buff_len),\/* *//* integer value in 'imm' field of BPF_CALL instruction selects which helper
@@ -3918,6 +3918,27 @@ static int bpf_xdp_mb_adjust_tail(struct xdp_buff *xdp, int offset)return0;}+BPF_CALL_1(bpf_xdp_get_buff_len,structxdp_buff*,xdp)+{+intdata_len=0;++if(unlikely(xdp_buff_is_mb(xdp))){+structskb_shared_info*sinfo;++sinfo=xdp_get_shared_info_from_buff(xdp);+data_len=sinfo->data_len;+}++return(xdp->data_end-xdp->data)+data_len;+}++conststructbpf_func_protobpf_xdp_get_buff_len_proto={+.func=bpf_xdp_get_buff_len,+.gpl_only=false,+.ret_type=RET_INTEGER,+.arg1_type=ARG_PTR_TO_CTX,+};+BPF_CALL_2(bpf_xdp_adjust_tail,structxdp_buff*,xdp,int,offset){void*data_hard_end=xdp_data_hard_end(xdp);/* use xdp->frame_sz */
@@ -4778,6 +4778,12 @@ union bpf_attr {*ExecuteclosesyscallforgivenFD.*Return*Asyscallresult.+*+*intbpf_xdp_get_buff_len(structxdp_buff*xdp_md)+*Description+*Getthetotalsizeofagivenxdpbuff(linearandpagedarea)+*Return+*Thetotalsizeofagivenxdpbuffer.*/#define __BPF_FUNC_MAPPER(FN) \FN(unspec),\
@@ -4949,6 +4955,7 @@ union bpf_attr {FN(sys_bpf),\FN(btf_find_by_name_kind),\FN(sys_close),\+FN(xdp_get_buff_len),\/* *//* integer value in 'imm' field of BPF_CALL instruction selects which helper
@@ -10,11 +10,20 @@ struct meta {intpkt_len;};+structtest_ctx_s{+boolpassed;+intpkt_size;+};++structtest_ctx_stest_ctx;+staticvoidon_sample(void*ctx,intcpu,void*data,__u32size){-intduration=0;structmeta*meta=(structmeta*)data;structipv4_packet*trace_pkt_v4=data+sizeof(*meta);+unsignedchar*raw_pkt=data+sizeof(*meta);+structtest_ctx_s*tst_ctx=ctx;+intduration=0;if(CHECK(size<sizeof(pkt_v4)+sizeof(*meta),"check_size","size %u < %zu\n",
@@ -25,25 +34,90 @@ static void on_sample(void *ctx, int cpu, void *data, __u32 size)"meta->ifindex = %d\n",meta->ifindex))return;-if(CHECK(meta->pkt_len!=sizeof(pkt_v4),"check_meta_pkt_len",-"meta->pkt_len = %zd\n",sizeof(pkt_v4)))+if(CHECK(meta->pkt_len!=tst_ctx->pkt_size,"check_meta_pkt_len",+"meta->pkt_len = %d\n",tst_ctx->pkt_size))return;if(CHECK(memcmp(trace_pkt_v4,&pkt_v4,sizeof(pkt_v4)),"check_packet_content","content not the same\n"))return;-*(bool*)ctx=true;+if(meta->pkt_len>sizeof(pkt_v4)){+for(inti=0;i<(meta->pkt_len-sizeof(pkt_v4));i++){+if(raw_pkt[i+sizeof(pkt_v4)]!=(unsignedchar)i){+CHECK(true,"check_packet_content",+"byte %zu does not match %u != %u\n",+i+sizeof(pkt_v4),+raw_pkt[i+sizeof(pkt_v4)],+(unsignedchar)i);+break;+}+}+}++tst_ctx->passed=true;}-voidtest_xdp_bpf2bpf(void)+staticintrun_xdp_bpf2bpf_pkt_size(intpkt_fd,structperf_buffer*pb,+structtest_xdp_bpf2bpf*ftrace_skel,+intpkt_size){__u32duration=0,retval,size;-charbuf[128];+unsignedcharbuf_in[9000];+unsignedcharbuf[9000];+interr;++if(pkt_size>sizeof(buf_in)||pkt_size<sizeof(pkt_v4))+return-EINVAL;++test_ctx.passed=false;+test_ctx.pkt_size=pkt_size;++memcpy(buf_in,&pkt_v4,sizeof(pkt_v4));+if(pkt_size>sizeof(pkt_v4)){+for(inti=0;i<(pkt_size-sizeof(pkt_v4));i++)+buf_in[i+sizeof(pkt_v4)]=i;+}++/* Run test program */+err=bpf_prog_test_run(pkt_fd,1,buf_in,pkt_size,+buf,&size,&retval,&duration);++if(CHECK(err||retval!=XDP_PASS||size!=pkt_size,+"ipv4","err %d errno %d retval %d size %d\n",+err,errno,retval,size))+return-1;++/* Make sure bpf_xdp_output() was triggered and it sent the expected+*datatotheperfringbuffer.+*/+err=perf_buffer__poll(pb,100);+if(CHECK(err<=0,"perf_buffer__poll","err %d\n",err))+return-1;++if(CHECK_FAIL(!test_ctx.passed))+return-1;++/* Verify test results */+if(CHECK(ftrace_skel->bss->test_result_fentry!=if_nametoindex("lo"),+"result","fentry failed err %llu\n",+ftrace_skel->bss->test_result_fentry))+return-1;++if(CHECK(ftrace_skel->bss->test_result_fexit!=XDP_PASS,"result",+"fexit failed err %llu\n",+ftrace_skel->bss->test_result_fexit))+return-1;++return0;+}++voidtest_xdp_bpf2bpf(void)+{interr,pkt_fd,map_fd;-boolpassed=false;-structiphdr*iph=(void*)buf+sizeof(structethhdr);-structiptnl_infovalue4={.family=AF_INET};+__u32duration=0;+intpkt_sizes[]={sizeof(pkt_v4),1024,4100,8200};+structiptnl_infovalue4={.family=AF_INET6};structtest_xdp*pkt_skel=NULL;structtest_xdp_bpf2bpf*ftrace_skel=NULL;structvipkey4={.protocol=6,.family=AF_INET};
@@ -87,40 +161,15 @@ void test_xdp_bpf2bpf(void)/* Set up perf buffer */pb_opts.sample_cb=on_sample;-pb_opts.ctx=&passed;+pb_opts.ctx=&test_ctx;pb=perf_buffer__new(bpf_map__fd(ftrace_skel->maps.perf_buf_map),-1,&pb_opts);+8,&pb_opts);if(!ASSERT_OK_PTR(pb,"perf_buf__new"))gotoout;-/* Run test program */-err=bpf_prog_test_run(pkt_fd,1,&pkt_v4,sizeof(pkt_v4),-buf,&size,&retval,&duration);--if(CHECK(err||retval!=XDP_TX||size!=74||-iph->protocol!=IPPROTO_IPIP,"ipv4",-"err %d errno %d retval %d size %d\n",-err,errno,retval,size))-gotoout;--/* Make sure bpf_xdp_output() was triggered and it sent the expected-*datatotheperfringbuffer.-*/-err=perf_buffer__poll(pb,100);-if(CHECK(err<0,"perf_buffer__poll","err %d\n",err))-gotoout;--CHECK_FAIL(!passed);--/* Verify test results */-if(CHECK(ftrace_skel->bss->test_result_fentry!=if_nametoindex("lo"),-"result","fentry failed err %llu\n",-ftrace_skel->bss->test_result_fentry))-gotoout;--CHECK(ftrace_skel->bss->test_result_fexit!=XDP_TX,"result",-"fexit failed err %llu\n",ftrace_skel->bss->test_result_fexit);-+for(inti=0;i<ARRAY_SIZE(pkt_sizes);i++)+run_xdp_bpf2bpf_pkt_size(pkt_fd,pb,ftrace_skel,+pkt_sizes[i]);out:if(pb)perf_buffer__free(pb);
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:51:50
Rely on data_size_in in bpf_test_init routine signature. This is a
preliminary patch to introduce xdp multi-buff selftest
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
net/bpf/test_run.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
@@ -570,7 +569,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,if(kattr->test.flags||kattr->test.cpu)return-EINVAL;-data=bpf_test_init(kattr,size,NET_SKB_PAD+NET_IP_ALIGN,+data=bpf_test_init(kattr,kattr->test.data_size_in,+size,NET_SKB_PAD+NET_IP_ALIGN,SKB_DATA_ALIGN(sizeof(structskb_shared_info)));if(IS_ERR(data))returnPTR_ERR(data);
@@ -707,7 +707,8 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,/* XDP have extra tailroom as (most) drivers use full page */max_data_sz=4096-headroom-tailroom;-data=bpf_test_init(kattr,max_data_sz,headroom,tailroom);+data=bpf_test_init(kattr,kattr->test.data_size_in,+max_data_sz,headroom,tailroom);if(IS_ERR(data))returnPTR_ERR(data);
@@ -769,7 +770,7 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,if(size<ETH_HLEN)return-EINVAL;-data=bpf_test_init(kattr,size,0,0);+data=bpf_test_init(kattr,kattr->test.data_size_in,size,0,0);if(IS_ERR(data))returnPTR_ERR(data);
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:52:01
Introduce the capability to allocate a xdp multi-buff in
bpf_prog_test_run_xdp routine. This is a preliminary patch to introduce
the selftests for new xdp multi-buff ebpf helpers
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
net/bpf/test_run.c | 52 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 44 insertions(+), 8 deletions(-)
@@ -692,23 +692,22 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,{u32tailroom=SKB_DATA_ALIGN(sizeof(structskb_shared_info));u32headroom=XDP_PACKET_HEADROOM;-u32size=kattr->test.data_size_in;u32repeat=kattr->test.repeat;structnetdev_rx_queue*rxqueue;+structskb_shared_info*sinfo;structxdp_buffxdp={};+u32max_data_sz,size;u32retval,duration;-u32max_data_sz;+inti,ret;void*data;-intret;if(kattr->test.ctx_in||kattr->test.ctx_out)return-EINVAL;-/* XDP have extra tailroom as (most) drivers use full page */max_data_sz=4096-headroom-tailroom;+size=min_t(u32,kattr->test.data_size_in,max_data_sz);-data=bpf_test_init(kattr,kattr->test.data_size_in,-max_data_sz,headroom,tailroom);+data=bpf_test_init(kattr,size,max_data_sz,headroom,tailroom);if(IS_ERR(data))returnPTR_ERR(data);
@@ -717,16 +716,53 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,&rxqueue->xdp_rxq);xdp_prepare_buff(&xdp,data,headroom,size,true);+sinfo=xdp_get_shared_info_from_buff(&xdp);+if(unlikely(kattr->test.data_size_in>size)){+void__user*data_in=u64_to_user_ptr(kattr->test.data_in);++while(size<kattr->test.data_size_in){+structpage*page;+skb_frag_t*frag;+intdata_len;++page=alloc_page(GFP_KERNEL);+if(!page){+ret=-ENOMEM;+gotoout;+}++frag=&sinfo->frags[sinfo->nr_frags++];+__skb_frag_set_page(frag,page);++data_len=min_t(int,kattr->test.data_size_in-size,+PAGE_SIZE);+skb_frag_size_set(frag,data_len);++if(copy_from_user(page_address(page),data_in+size,+data_len)){+ret=-EFAULT;+gotoout;+}+sinfo->data_len+=data_len;+size+=data_len;+}+xdp_buff_set_mb(&xdp);+}+bpf_prog_change_xdp(NULL,prog);ret=bpf_test_run(prog,&xdp,repeat,&retval,&duration,true);if(ret)gotoout;-if(xdp.data!=data+headroom||xdp.data_end!=xdp.data+size)-size=xdp.data_end-xdp.data;++size=xdp.data_end-xdp.data+sinfo->data_len;ret=bpf_test_finish(kattr,uattr,xdp.data,size,retval,duration);+out:bpf_prog_change_xdp(prog,NULL);+for(i=0;i<sinfo->nr_frags;i++)+__free_page(skb_frag_page(&sinfo->frags[i]));kfree(data);+returnret;}
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2021-06-14 12:52:03
introduce xdp_shared_info pointer in bpf_test_finish signature in order
to copy back paged data from a xdp multi-buff frame to userspace buffer
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
net/bpf/test_run.c | 47 ++++++++++++++++++++++++++++++++++++++--------
1 file changed, 39 insertions(+), 8 deletions(-)
@@ -673,7 +702,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,/* bpf program can never convert linear skb to non-linear */if(WARN_ON_ONCE(skb_is_nonlinear(skb)))size=skb_headlen(skb);-ret=bpf_test_finish(kattr,uattr,skb->data,size,retval,duration);+ret=bpf_test_finish(kattr,uattr,skb->data,NULL,size,retval,+duration);if(!ret)ret=bpf_ctx_finish(kattr,uattr,ctx,sizeof(struct__sk_buff));
@@ -755,7 +785,8 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,gotoout;size=xdp.data_end-xdp.data+sinfo->data_len;-ret=bpf_test_finish(kattr,uattr,xdp.data,size,retval,duration);+ret=bpf_test_finish(kattr,uattr,xdp.data,sinfo,size,retval,+duration);out:bpf_prog_change_xdp(prog,NULL);
@@ -841,8 +872,8 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,if(ret<0)gotoout;-ret=bpf_test_finish(kattr,uattr,&flow_keys,sizeof(flow_keys),-retval,duration);+ret=bpf_test_finish(kattr,uattr,&flow_keys,NULL,+sizeof(flow_keys),retval,duration);if(!ret)ret=bpf_ctx_finish(kattr,uattr,user_ctx,sizeof(structbpf_flow_keys));
@@ -946,7 +977,7 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog, const union bpf_attr *katuser_ctx->cookie=sock_gen_cookie(ctx.selected_sk);}-ret=bpf_test_finish(kattr,uattr,NULL,0,retval,duration);+ret=bpf_test_finish(kattr,uattr,NULL,NULL,0,retval,duration);if(!ret)ret=bpf_ctx_finish(kattr,uattr,user_ctx,sizeof(*user_ctx));
@@ -130,6 +130,107 @@ void test_xdp_adjust_tail_grow2(void)bpf_object__close(obj);}+voidtest_xdp_adjust_mb_tail_shrink(void)+{+constchar*file="./test_xdp_adjust_tail_shrink.o";+__u32duration,retval,size,exp_size;+structbpf_object*obj;+staticcharbuf[9000];+interr,prog_fd;++/* For the individual test cases, the first byte in the packet+*indicateswhichtestwillberun.+*/++err=bpf_prog_load(file,BPF_PROG_TYPE_XDP,&obj,&prog_fd);+if(CHECK_FAIL(err))+return;++/* Test case removing 10 bytes from last frag, NOT freeing it */+buf[0]=0;+exp_size=sizeof(buf)-10;+err=bpf_prog_test_run(prog_fd,1,buf,sizeof(buf),+buf,&size,&retval,&duration);++CHECK(err||retval!=XDP_TX||size!=exp_size,+"9k-10b","err %d errno %d retval %d[%d] size %d[%u]\n",+err,errno,retval,XDP_TX,size,exp_size);++/* Test case removing one of two pages, assuming 4K pages */+buf[0]=1;+exp_size=sizeof(buf)-4100;+err=bpf_prog_test_run(prog_fd,1,buf,sizeof(buf),+buf,&size,&retval,&duration);++CHECK(err||retval!=XDP_TX||size!=exp_size,+"9k-1p","err %d errno %d retval %d[%d] size %d[%u]\n",+err,errno,retval,XDP_TX,size,exp_size);++/* Test case removing two pages resulting in a non mb xdp_buff */+buf[0]=2;+exp_size=sizeof(buf)-8200;+err=bpf_prog_test_run(prog_fd,1,buf,sizeof(buf),+buf,&size,&retval,&duration);++CHECK(err||retval!=XDP_TX||size!=exp_size,+"9k-2p","err %d errno %d retval %d[%d] size %d[%u]\n",+err,errno,retval,XDP_TX,size,exp_size);++bpf_object__close(obj);+}++voidtest_xdp_adjust_mb_tail_grow(void)+{+constchar*file="./test_xdp_adjust_tail_grow.o";+__u32duration,retval,size,exp_size;+staticcharbuf[16384];+structbpf_object*obj;+interr,i,prog_fd;++err=bpf_prog_load(file,BPF_PROG_TYPE_XDP,&obj,&prog_fd);+if(CHECK_FAIL(err))+return;++/* Test case add 10 bytes to last frag */+memset(buf,1,sizeof(buf));+size=9000;+exp_size=size+10;+err=bpf_prog_test_run(prog_fd,1,buf,size,+buf,&size,&retval,&duration);++CHECK(err||retval!=XDP_TX||size!=exp_size,+"9k+10b","err %d retval %d[%d] size %d[%u]\n",+err,retval,XDP_TX,size,exp_size);++for(i=0;i<9000;i++)+CHECK(buf[i]!=1,"9k+10b-old",+"Old data not all ok, offset %i is failing [%u]!\n",+i,buf[i]);++for(i=9000;i<9010;i++)+CHECK(buf[i]!=0,"9k+10b-new",+"New data not all ok, offset %i is failing [%u]!\n",+i,buf[i]);++for(i=9010;i<sizeof(buf);i++)+CHECK(buf[i]!=1,"9k+10b-untouched",+"Unused data not all ok, offset %i is failing [%u]!\n",+i,buf[i]);++/* Test a too large grow */+memset(buf,1,sizeof(buf));+size=9001;+exp_size=size;+err=bpf_prog_test_run(prog_fd,1,buf,size,+buf,&size,&retval,&duration);++CHECK(err||retval!=XDP_DROP||size!=exp_size,+"9k+10b","err %d retval %d[%d] size %d[%u]\n",+err,retval,XDP_TX,size,exp_size);++bpf_object__close(obj);+}+voidtest_xdp_adjust_tail(void){if(test__start_subtest("xdp_adjust_tail_shrink"))
@@ -7,11 +7,10 @@ int _xdp_adjust_tail_grow(struct xdp_md *xdp){void*data_end=(void*)(long)xdp->data_end;void*data=(void*)(long)xdp->data;-unsignedintdata_len;+intdata_len=bpf_xdp_get_buff_len(xdp);intoffset=0;/* Data length determine test case */-data_len=data_end-data;if(data_len==54){/* sizeof(pkt_v4) */offset=4096;/* test too large offset */
@@ -20,7 +19,12 @@ int _xdp_adjust_tail_grow(struct xdp_md *xdp)}elseif(data_len==64){offset=128;}elseif(data_len==128){-offset=4096-256-320-data_len;/* Max tail grow 3520 */+/* Max tail grow 3520 */+offset=4096-256-320-data_len;+}elseif(data_len==9000){+offset=10;+}elseif(data_len==9001){+offset=4096;}else{returnXDP_ABORTED;/* No matching test */}
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-06-22 23:18:49
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.
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.
At this point I don't think we can have a partial implementation. At
the moment we have packet capture applications and protocol parsers
running in production. If we allow this to go in staged we are going
to break those applications that make the fundamental assumption they
have access to all the data in the packet.
There will be no way to fix it when it happens. The teams running the
applications wont necessarily be able to change the network MTU. Now
it doesn't work, hard stop. This is better than it sort of works some
of the time. Worse if we get in a situation where some drivers support
partial access and others support full access the support matrix gets worse.
I think we need to get full support and access to all bytes. I believe
I said this earlier, but now we've deployed apps that really do need
access to the payloads so its not a theoritical concern anymore, but
rather a real one based on deployed BPF programs.
The main idea for the new multi-buffer layout is to reuse the same
layout used for non-linear SKB. This rely on the "skb_shared_info"
struct at the end of the first buffer to link together subsequent
buffers. Keeping the layout compatible with SKBs is also done to ease
and speedup creating an SKB from an xdp_{buff,frame}.
Converting xdp_frame to SKB and deliver it to the network stack is shown
in patch 07/14 (e.g. cpumaps).
A multi-buffer bit (mb) has been introduced in the flags field of 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 skb_shared_info structure at the end
of the first buffer will be initialized only if mb is set.
Moreover the flags field in xdp_{buff,frame} will be reused even for
xdp rx csum offloading in future series.
Typical use cases for this series are:
- Jumbo-frames
- Packet header split (please see Google���s use-case @ NetDevConf 0x14, [0])
- TSO
A new bpf helper (bpf_xdp_get_buff_len) has been introduce in order to notify
the eBPF layer about the total frame size (linear + paged parts).
Is it possible to make currently working programs continue to work?
For a simple packet capture example a program might capture the
entire packet of bytes '(data_end - data_start)'. With above implementation
the program will continue to run, but will no longer be capturing
all the bytes... so its a silent failure. Otherwise I'll need to
backport fixes into my BPF programs and releases to ensure they
don't walk onto a new kernel with multi-buffer support enabled.
Its not ideal.
bpf_xdp_adjust_tail and bpf_xdp_copy helpers have been modified to take into
account xdp multi-buff frames.
More info about the main idea behind this approach can be found here [1][2].
Will read [1],[2].
Where did the perf data for the 40gbps NIC go? I think we want that
done again on this series with at least 40gbps NICs and better
yet 100gbps drivers. If its addressed in a patch commit message
I'm reading the series now.
Changes since v8:
- add proper dma unmapping if XDP_TX fails on mvneta for a xdp multi-buff
- switch back to skb_shared_info implementation from previous xdp_shared_info
one
- avoid using a bietfield in xdp_buff/xdp_frame since it introduces performance
regressions. Tested now on 10G NIC (ixgbe) to verify there are no performance
penalties for regular codebase
- add bpf_xdp_get_buff_len helper and remove frame_length field in xdp ctx
- add data_len field in skb_shared_info struct
Changes since v7:
- rebase on top of bpf-next
- fix sparse warnings
- improve comments for frame_length in include/net/xdp.h
Changes since v6:
- the main difference respect to previous versions is the new approach proposed
by Eelco to pass full length of the packet to eBPF layer in XDP context
- reintroduce multi-buff support to eBPF kself-tests
- reintroduce multi-buff support to bpf_xdp_adjust_tail helper
- introduce multi-buffer support to bpf_xdp_copy helper
- rebase on top of bpf-next
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 (3):
bpf: add multi-buff support to the bpf_xdp_adjust_tail() API
bpf: add multi-buffer support to xdp copy helpers
bpf: update xdp_adjust_tail selftest to include multi-buffer
Lorenzo Bianconi (11):
net: skbuff: add data_len field to skb_shared_info
xdp: introduce flags field in xdp_buff/xdp_frame
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_frame
bpf: introduce bpf_xdp_get_buff_len helper
bpf: move user_size out of bpf_test_init
bpf: introduce multibuff support to bpf_prog_test_run_xdp()
bpf: test_run: add xdp_shared_info pointer in bpf_test_finish
signature
drivers/net/ethernet/marvell/mvneta.c | 143 ++++++++++------
include/linux/skbuff.h | 5 +-
include/net/xdp.h | 56 ++++++-
include/uapi/linux/bpf.h | 7 +
kernel/trace/bpf_trace.c | 3 +
net/bpf/test_run.c | 108 +++++++++---
net/core/filter.c | 157 +++++++++++++++++-
net/core/xdp.c | 72 +++++++-
tools/include/uapi/linux/bpf.h | 7 +
.../bpf/prog_tests/xdp_adjust_tail.c | 105 ++++++++++++
.../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 127 +++++++++-----
.../bpf/progs/test_xdp_adjust_tail_grow.c | 10 +-
.../bpf/progs/test_xdp_adjust_tail_shrink.c | 32 +++-
.../selftests/bpf/progs/test_xdp_bpf2bpf.c | 2 +-
14 files changed, 705 insertions(+), 129 deletions(-)
--
2.31.1
Can you add some comment on how this works? So today I call
bpf_xdp_adjust_tail() to add some trailer to my packet.
This looks like it adds tailroom to the last frag? But, then
how do I insert my trailer? I don't think we can without the
extra multi-buffer access support right.
Also data_end will be unchanged yet it will return 0 so my
current programs will likely be a bit confused by this.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-06-22 23:49:53
Lorenzo Bianconi wrote:
From: Eelco Chaudron <echaudro@redhat.com>
This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Ah ok so at least xdp_output will work with all bytes. But this is
getting close to having access into the frags so I think doing
the last bit shouldn't be too hard?
This block reads odd to be because it requires looping over the frags
multiple times? Why not something like this,
if (off < base_len) {
src_buff = xdp->data + off
copy_len = min...
memcpy(dst_buff, src_buff, copy_len)
off += copylen
len -= copylen
dst_buff += copylen;
}
for (i = 0; i , nr_frags; i++) {
frag = ...
...
if frag_off < fraglen
...
memcpy()
update(off, len, dst_buff)
}
Maybe use a helper to set off,len and dst_buff if worried about the
duplication. Seems cleaner than walking through 0..n-1 frags for
each copy.
From: David Ahern <hidden> Date: 2021-06-23 03:41:46
On 6/22/21 5:18 PM, John Fastabend wrote:
At this point I don't think we can have a partial implementation. At
the moment we have packet capture applications and protocol parsers
running in production. If we allow this to go in staged we are going
to break those applications that make the fundamental assumption they
have access to all the data in the packet.
What about cases like netgpu where headers are accessible but data is
not (e.g., gpu memory)? If the API indicates limited buffer access, is
that sufficient?
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-06-23 05:48:29
David Ahern wrote:
On 6/22/21 5:18 PM, John Fastabend wrote:
quoted
At this point I don't think we can have a partial implementation. At
the moment we have packet capture applications and protocol parsers
running in production. If we allow this to go in staged we are going
to break those applications that make the fundamental assumption they
have access to all the data in the packet.
What about cases like netgpu where headers are accessible but data is
not (e.g., gpu memory)? If the API indicates limited buffer access, is
that sufficient?
I never consider netgpus and I guess I don't fully understand the
architecture to say. But, I would try to argue that an XDP API
should allow XDP to reach into the payload of these GPU packets as well.
Of course it might be slow.
I'm not really convinced just indicating its a limited buffer is enough.
I think we want to be able to read/write any byte in the packet. I see
two ways to do it,
/* xdp_pull_data moves data and data_end pointers into the frag
* containing the byte offset start.
*
* returns negative value on error otherwise returns offset of
* data pointer into payload.
*/
int xdp_pull_data(int start)
This would be a helper call to push the xdp->data{_end} pointers into
the correct frag and then normal verification should work. From my
side this works because I can always find the next frag by starting
at 'xdp_pull_data(xdp->data_end+1)'. And by returning offset we can
always figure out where we are in the payload. This is the easiest
thing I could come up with. And hopefully for _most_ cases the bytes
we need are in the initial data. Also I don't see how extending tail
works without something like this.
My other thought, but requires some verifier work would be to extend
'struct xdp_md' with a frags[] pointer.
struct xdp_md {
__u32 data;
__u32 data_end;
__u32 data_meta;
/* metadata stuff */
struct _xdp_md frags[]
__u32 frags_end;
}
Then a XDP program could read access a frag like so,
if (i < xdp->frags_end) {
frag = xdp->frags[i];
if (offset + hdr_size < frag->data_end)
memcpy(dst, frag->data[offset], hdr_size);
}
The nice bit about above is you avoid the call, but maybe it doesn't
matter if you are already looking into frags pps is probably not at
64B sizes anyways.
My main concern here is we hit a case where the driver doesn't pull in
the bytes we need and then we are stuck without a workaround. The helper
looks fairly straightforward to me could we try that?
Also I thought we had another driver in the works? Any ideas where
that went...
Last, I'll add thanks for working on this everyone.
.John
From: David Ahern <hidden> Date: 2021-06-23 14:40:51
On 6/22/21 11:48 PM, John Fastabend wrote:
David Ahern wrote:
quoted
On 6/22/21 5:18 PM, John Fastabend wrote:
quoted
At this point I don't think we can have a partial implementation. At
the moment we have packet capture applications and protocol parsers
running in production. If we allow this to go in staged we are going
to break those applications that make the fundamental assumption they
have access to all the data in the packet.
What about cases like netgpu where headers are accessible but data is
not (e.g., gpu memory)? If the API indicates limited buffer access, is
that sufficient?
I never consider netgpus and I guess I don't fully understand the
architecture to say. But, I would try to argue that an XDP API
should allow XDP to reach into the payload of these GPU packets as well.
Of course it might be slow.
AIUI S/W on the host can not access gpu memory, so that is not a
possibility at all.
Another use case is DDP and ZC. Mellanox has a proposal for NVME (with
intentions to extend to iscsi) to do direct data placement. This is
really just an example of zerocopy (and netgpu has morphed into zctap
with current prototype working for host memory) which will become more
prominent. XDP programs accessing memory already mapped to user space
will be racy.
To me these proposals suggest a trend and one that XDP APIs should be
ready to handle - like indicating limited access or specifying length
that can be accessed.
I'm not really convinced just indicating its a limited buffer is enough.
I think we want to be able to read/write any byte in the packet. I see
two ways to do it,
/* xdp_pull_data moves data and data_end pointers into the frag
* containing the byte offset start.
*
* returns negative value on error otherwise returns offset of
* data pointer into payload.
*/
int xdp_pull_data(int start)
This would be a helper call to push the xdp->data{_end} pointers into
the correct frag and then normal verification should work. From my
side this works because I can always find the next frag by starting
at 'xdp_pull_data(xdp->data_end+1)'. And by returning offset we can
always figure out where we are in the payload. This is the easiest
thing I could come up with. And hopefully for _most_ cases the bytes
we need are in the initial data. Also I don't see how extending tail
works without something like this.
My other thought, but requires some verifier work would be to extend
'struct xdp_md' with a frags[] pointer.
struct xdp_md {
__u32 data;
__u32 data_end;
__u32 data_meta;
/* metadata stuff */
struct _xdp_md frags[]
__u32 frags_end;
}
Then a XDP program could read access a frag like so,
if (i < xdp->frags_end) {
frag = xdp->frags[i];
if (offset + hdr_size < frag->data_end)
memcpy(dst, frag->data[offset], hdr_size);
}
The nice bit about above is you avoid the call, but maybe it doesn't
matter if you are already looking into frags pps is probably not at
64B sizes anyways.
My main concern here is we hit a case where the driver doesn't pull in
the bytes we need and then we are stuck without a workaround. The helper
looks fairly straightforward to me could we try that?
Also I thought we had another driver in the works? Any ideas where
that went...
Last, I'll add thanks for working on this everyone.
.John
Can you add some comment on how this works? So today I call
bpf_xdp_adjust_tail() to add some trailer to my packet.
This looks like it adds tailroom to the last frag? But, then
how do I insert my trailer? I don't think we can without the
extra multi-buffer access support right.
You are right, we need some kind of multi-buffer access helpers.
Also data_end will be unchanged yet it will return 0 so my
current programs will likely be a bit confused by this.
Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.
But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.
From: Eelco Chaudron <echaudro@redhat.com>
This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Ah ok so at least xdp_output will work with all bytes. But this is
getting close to having access into the frags so I think doing
the last bit shouldn't be too hard?
Guess you are talking about multi-buffer access in the XDP program?
I did suggest an API a while back, https://lore.kernel.org/bpf/FD3E6E08-DE78-4FBA-96F6-646C93E88631@redhat.com/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.
This block reads odd to be because it requires looping over the frags
multiple times? Why not something like this,
if (off < base_len) {
src_buff = xdp->data + off
copy_len = min...
memcpy(dst_buff, src_buff, copy_len)
off += copylen
len -= copylen
dst_buff += copylen;
}
for (i = 0; i , nr_frags; i++) {
frag = ...
...
if frag_off < fraglen
...
memcpy()
update(off, len, dst_buff)
}
Maybe use a helper to set off,len and dst_buff if worried about the
duplication. Seems cleaner than walking through 0..n-1 frags for
each copy.
You are right it looks odd, will re-write this in the next iteration.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-06-24 14:22:17
David Ahern wrote:
On 6/22/21 11:48 PM, John Fastabend wrote:
quoted
David Ahern wrote:
quoted
On 6/22/21 5:18 PM, John Fastabend wrote:
quoted
At this point I don't think we can have a partial implementation. At
the moment we have packet capture applications and protocol parsers
running in production. If we allow this to go in staged we are going
to break those applications that make the fundamental assumption they
have access to all the data in the packet.
What about cases like netgpu where headers are accessible but data is
not (e.g., gpu memory)? If the API indicates limited buffer access, is
that sufficient?
I never consider netgpus and I guess I don't fully understand the
architecture to say. But, I would try to argue that an XDP API
should allow XDP to reach into the payload of these GPU packets as well.
Of course it might be slow.
AIUI S/W on the host can not access gpu memory, so that is not a
possibility at all.
interesting.
Another use case is DDP and ZC. Mellanox has a proposal for NVME (with
intentions to extend to iscsi) to do direct data placement. This is
really just an example of zerocopy (and netgpu has morphed into zctap
with current prototype working for host memory) which will become more
prominent. XDP programs accessing memory already mapped to user space
will be racy.
Its racy in the sense that if the application is reading data before
the driver flips some bit to tell the application new data is available
XDP could write old data or read application changed data? I think
it would still "work" same as AF_XDP? If you allow DDP then you lose
ability to l7 security as far as I can tell. But, thats a general
comment not specific to XDP.
To me these proposals suggest a trend and one that XDP APIs should be
ready to handle - like indicating limited access or specifying length
that can be accessed.
I still think the only case is this net-gpu which we don't have in
kernel at the moment right? I think a bit or size or ... would make
sense if we had this hardware. And then for the other DDP/ZC case
the system owner would need to know what they are doing when they
turn on DDP or whatever.
.John
Can you add some comment on how this works? So today I call
bpf_xdp_adjust_tail() to add some trailer to my packet.
This looks like it adds tailroom to the last frag? But, then
how do I insert my trailer? I don't think we can without the
extra multi-buffer access support right.
You are right, we need some kind of multi-buffer access helpers.
quoted
Also data_end will be unchanged yet it will return 0 so my
current programs will likely be a bit confused by this.
Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.
But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.
Right that was my conclusion as well. Existing programs might
have subtle side effects if they start running on multibuffer
drivers as is. I don't have any good ideas though on how
to handle this.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-06-24 14:28:41
Eelco Chaudron wrote:
On 23 Jun 2021, at 1:49, John Fastabend wrote:
quoted
Lorenzo Bianconi wrote:
quoted
From: Eelco Chaudron <echaudro@redhat.com>
This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Ah ok so at least xdp_output will work with all bytes. But this is
getting close to having access into the frags so I think doing
the last bit shouldn't be too hard?
Guess you are talking about multi-buffer access in the XDP program?
I did suggest an API a while back, https://lore.kernel.org/bpf/FD3E6E08-DE78-4FBA-96F6-646C93E88631@redhat.com/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.
Ah great I think we had the same idea I called it xdp_pull_data()
though.
Whats the complication though it looks like it can be done by simply
moving the data and data_end pointers around then marking them
invalidated. This way the verifier knows the program needs to
rewrite them. I can probably look more into next week.
From my first glance it looks relatively straight forward to do
now. I really would like to avoid yet another iteration of
programs features I have to discover and somehow work around
if we can get the helper into this series. If you really don't
have time I can probably take a look early next week on an
RFC for something like above helper.
.John
Can you add some comment on how this works? So today I call
bpf_xdp_adjust_tail() to add some trailer to my packet.
This looks like it adds tailroom to the last frag? But, then
how do I insert my trailer? I don't think we can without the
extra multi-buffer access support right.
You are right, we need some kind of multi-buffer access helpers.
quoted
Also data_end will be unchanged yet it will return 0 so my
current programs will likely be a bit confused by this.
Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.
But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.
Right that was my conclusion as well. Existing programs might
have subtle side effects if they start running on multibuffer
drivers as is. I don't have any good ideas though on how
to handle this.
Would it be possible to detect multibuffer awareness of a program at load
(or attach) time, perhaps by looking for the use of the new multibuffer
helpers? That might make it possible to reject a non-multibuffer aware
program on multibuffer drivers (or maybe even put the driver into a
non-multibuffer mode at attach time), or at the very least issue a
warning?
From: Eelco Chaudron <echaudro@redhat.com>
This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Ah ok so at least xdp_output will work with all bytes. But this is
getting close to having access into the frags so I think doing
the last bit shouldn't be too hard?
Guess you are talking about multi-buffer access in the XDP program?
I did suggest an API a while back, https://lore.kernel.org/bpf/FD3E6E08-DE78-4FBA-96F6-646C93E88631@redhat.com/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.
Ah great I think we had the same idea I called it xdp_pull_data()
though.
Whats the complication though it looks like it can be done by simply
moving the data and data_end pointers around then marking them
invalidated. This way the verifier knows the program needs to
rewrite them. I can probably look more into next week.
From my first glance it looks relatively straight forward to do
now. I really would like to avoid yet another iteration of
programs features I have to discover and somehow work around
if we can get the helper into this series. If you really don't
have time I can probably take a look early next week on an
RFC for something like above helper.
I’m on a small side project for the next 2 to 3 weeks, and after that, I have some PTO, so if you have time for an RFC, that will speed up this patchset.
Thanks,
Eelco
From: Alexander Duyck <hidden> Date: 2021-06-28 19:58:50
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted hunk
data_len field will be used for paged frame len for xdp_buff/xdp_frame.
This is a preliminary patch to properly support xdp-multibuff
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
include/linux/skbuff.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Rather than use the tskey field why not repurpose the gso_size field?
I would think in the XDP paths that the gso fields would be unused
since LRO and HW_GRO would be incompatible with XDP anyway.
From: Alexander Duyck <hidden> Date: 2021-06-28 20:14:47
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
Introduce flags field in xdp_frame/xdp_buffer data structure
to define additional buffer features. At the moment the only
supported buffer feature is multi-buffer bit (mb). Multi-buffer bit
is used 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>
Instead of passing this between buffers and frames I wonder if this
wouldn't be better to place in something like the xdp_mem_info
structure since this is something that would be specific to how the
device is handling memory anyway. You could probably split the type
field into a 16b type and a 16b flags field. Then add your bit where 0
is linear/legacy and 1 is scatter-gather/multi-buffer.
So this is assuming the header frame and all of the frags are using
the same size. Rather than reading the frags out and then writing them
back, why not just directly rewrite the nr_frags, add the total size
to skb->len and skb->data_len, and then update the truesize?
Actually, I think you might need to store the truesize somewhere in
addition to the data_len that you were storing in the shared info.
From: Lorenzo Bianconi <hidden> Date: 2021-06-29 12:43:40
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
Introduce flags field in xdp_frame/xdp_buffer data structure
to define additional buffer features. At the moment the only
supported buffer feature is multi-buffer bit (mb). Multi-buffer bit
is used 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>
Instead of passing this between buffers and frames I wonder if this
wouldn't be better to place in something like the xdp_mem_info
structure since this is something that would be specific to how the
device is handling memory anyway. You could probably split the type
field into a 16b type and a 16b flags field. Then add your bit where 0
is linear/legacy and 1 is scatter-gather/multi-buffer.
ack, this should be fine but I put the flag field in xdp_buff/xdp_frame
in order to reuse it for some xdp hw-hints (e.g rx checksum type).
We can put it in xdp_mem_info too but I guess it would be less intuitive, what
do you think?
Regards,
Lorenzo
From: Lorenzo Bianconi <hidden> Date: 2021-06-29 12:45:05
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
data_len field will be used for paged frame len for xdp_buff/xdp_frame.
This is a preliminary patch to properly support xdp-multibuff
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
include/linux/skbuff.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Rather than use the tskey field why not repurpose the gso_size field?
I would think in the XDP paths that the gso fields would be unused
since LRO and HW_GRO would be incompatible with XDP anyway.
ack, I agree. I will fix it in v10.
Regards,
Lorenzo
From: Alexander Duyck <hidden> Date: 2021-06-29 13:07:36
On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
Introduce flags field in xdp_frame/xdp_buffer data structure
to define additional buffer features. At the moment the only
supported buffer feature is multi-buffer bit (mb). Multi-buffer bit
is used 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>
Instead of passing this between buffers and frames I wonder if this
wouldn't be better to place in something like the xdp_mem_info
structure since this is something that would be specific to how the
device is handling memory anyway. You could probably split the type
field into a 16b type and a 16b flags field. Then add your bit where 0
is linear/legacy and 1 is scatter-gather/multi-buffer.
ack, this should be fine but I put the flag field in xdp_buff/xdp_frame
in order to reuse it for some xdp hw-hints (e.g rx checksum type).
We can put it in xdp_mem_info too but I guess it would be less intuitive, what
do you think?
I think it makes the most sense in xdp_mem_info. It already tells us
what to expect in some respect in regards to memory layout as it tells
us if we are dealing with shared pages or whole pages and how to
recycle them. I would think that applies almost identically to
scatter-gather XDP the same way.
As far as the addition of flags there is still time for that later as
we still have the 32b of unused space after frame_sz.
From: Lorenzo Bianconi <hidden> Date: 2021-06-29 13:19:14
Eelco Chaudron wrote:
quoted
On 23 Jun 2021, at 1:37, John Fastabend wrote:
quoted
Lorenzo Bianconi wrote:
quoted
From: Eelco Chaudron <echaudro@redhat.com>
This change adds support for tail growing and shrinking for XDP multi-buff.
It would be nice if the commit message gave us some details on how the
growing/shrinking works in the multi-buff support.
[...]
quoted
Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.
But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.
Right that was my conclusion as well. Existing programs might
have subtle side effects if they start running on multibuffer
drivers as is. I don't have any good ideas though on how
to handle this.
what about checking the program capabilities at load time (e.g. with a
special program type) and disable mb feature if the bpf program is not
mb-aware? (e.g. forbid to set the MTU greater than 1500B in xdp mode).
Regards,
Lorenzo
From: Lorenzo Bianconi <hidden> Date: 2021-06-29 13:23:35
quoted
On 23 Jun 2021, at 1:49, John Fastabend wrote:
quoted
Lorenzo Bianconi wrote:
quoted
From: Eelco Chaudron <echaudro@redhat.com>
This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Ah ok so at least xdp_output will work with all bytes. But this is
getting close to having access into the frags so I think doing
the last bit shouldn't be too hard?
Guess you are talking about multi-buffer access in the XDP program?
I did suggest an API a while back, https://lore.kernel.org/bpf/FD3E6E08-DE78-4FBA-96F6-646C93E88631@redhat.com/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.
Ah great I think we had the same idea I called it xdp_pull_data()
though.
Whats the complication though it looks like it can be done by simply
moving the data and data_end pointers around then marking them
invalidated. This way the verifier knows the program needs to
rewrite them. I can probably look more into next week.
From my first glance it looks relatively straight forward to do
now. I really would like to avoid yet another iteration of
programs features I have to discover and somehow work around
if we can get the helper into this series. If you really don't
have time I can probably take a look early next week on an
RFC for something like above helper.
cool, thx :)
What about discussing APIs during the BPF mtg upstream on Thursday (probably
not next one since most of the people will be in PTO)? I will work on some docs.
Regards,
Lorenzo
From: Lorenzo Bianconi <hidden> Date: 2021-06-29 13:25:26
On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
quoted
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
Introduce flags field in xdp_frame/xdp_buffer data structure
to define additional buffer features. At the moment the only
supported buffer feature is multi-buffer bit (mb). Multi-buffer bit
is used 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>
Instead of passing this between buffers and frames I wonder if this
wouldn't be better to place in something like the xdp_mem_info
structure since this is something that would be specific to how the
device is handling memory anyway. You could probably split the type
field into a 16b type and a 16b flags field. Then add your bit where 0
is linear/legacy and 1 is scatter-gather/multi-buffer.
ack, this should be fine but I put the flag field in xdp_buff/xdp_frame
in order to reuse it for some xdp hw-hints (e.g rx checksum type).
We can put it in xdp_mem_info too but I guess it would be less intuitive, what
do you think?
I think it makes the most sense in xdp_mem_info. It already tells us
what to expect in some respect in regards to memory layout as it tells
us if we are dealing with shared pages or whole pages and how to
recycle them. I would think that applies almost identically to
scatter-gather XDP the same way.
As far as the addition of flags there is still time for that later as
we still have the 32b of unused space after frame_sz.
ack, I am fine with it. If everybody agree, I will fix it in v10.
Regards,
Lorenzo
From: Eelco Chaudron <echaudro@redhat.com>
This change adds support for tail growing and shrinking for XDP multi-buff.
It would be nice if the commit message gave us some details on how the
growing/shrinking works in the multi-buff support.
[...]
quoted
quoted
Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.
But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.
Right that was my conclusion as well. Existing programs might
have subtle side effects if they start running on multibuffer
drivers as is. I don't have any good ideas though on how
to handle this.
what about checking the program capabilities at load time (e.g. with a
special program type) and disable mb feature if the bpf program is not
mb-aware? (e.g. forbid to set the MTU greater than 1500B in xdp mode).
So what happens when that legacy program runs on a veth and gets an
mb-enabled frame redirected into it? :)
-Toke
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-06-29 17:08:55
On Tue, 29 Jun 2021 14:44:56 +0200 Lorenzo Bianconi wrote:
quoted
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
data_len field will be used for paged frame len for xdp_buff/xdp_frame.
This is a preliminary patch to properly support xdp-multibuff
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
include/linux/skbuff.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Rather than use the tskey field why not repurpose the gso_size field?
I would think in the XDP paths that the gso fields would be unused
since LRO and HW_GRO would be incompatible with XDP anyway.
ack, I agree. I will fix it in v10.
Why is XDP mb incompatible with LRO? I thought that was one of the use
cases (mentioned by Willem IIRC).
From: Alexander Duyck <hidden> Date: 2021-06-29 18:18:56
On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski [off-list ref] wrote:
On Tue, 29 Jun 2021 14:44:56 +0200 Lorenzo Bianconi wrote:
quoted
quoted
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
data_len field will be used for paged frame len for xdp_buff/xdp_frame.
This is a preliminary patch to properly support xdp-multibuff
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
include/linux/skbuff.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Rather than use the tskey field why not repurpose the gso_size field?
I would think in the XDP paths that the gso fields would be unused
since LRO and HW_GRO would be incompatible with XDP anyway.
ack, I agree. I will fix it in v10.
Why is XDP mb incompatible with LRO? I thought that was one of the use
cases (mentioned by Willem IIRC).
XDP is meant to be a per packet operation with support for TX and
REDIRECT, and LRO isn't routable. So we could put together a large LRO
frame but we wouldn't be able to break it apart again. If we allow
that then we are going to need a ton more exception handling added to
the XDP paths.
As far as GSO it would require setting many more fields in order to
actually make it offloadable by any hardware. My preference would be
to make use of gso_segs and gso_size to store the truesize and datalen
of the pages. That way we keep all of the data fields used in the
shared info in the first 8 bytes assuming we don't end up having to
actually use multiple buffers.
So this is assuming the header frame and all of the frags are using
the same size. Rather than reading the frags out and then writing them
back, why not just directly rewrite the nr_frags, add the total size
to skb->len and skb->data_len, and then update the truesize?
ack, thx. I will look into it.
Regards,
Lorenzo
Actually, I think you might need to store the truesize somewhere in
addition to the data_len that you were storing in the shared info.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-06-29 18:37:21
On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski [off-list ref] wrote:
quoted
quoted
ack, I agree. I will fix it in v10.
Why is XDP mb incompatible with LRO? I thought that was one of the use
cases (mentioned by Willem IIRC).
XDP is meant to be a per packet operation with support for TX and
REDIRECT, and LRO isn't routable. So we could put together a large LRO
frame but we wouldn't be able to break it apart again. If we allow
that then we are going to need a ton more exception handling added to
the XDP paths.
As far as GSO it would require setting many more fields in order to
actually make it offloadable by any hardware.
It would require more work, but TSO seems to be explicitly stated
as what the series builds towards (in the cover letter). It's fine
to make choices we'd need to redo later, I guess, I'm just trying
to understand the why.
My preference would be
to make use of gso_segs and gso_size to store the truesize and datalen
of the pages. That way we keep all of the data fields used in the
shared info in the first 8 bytes assuming we don't end up having to
actually use multiple buffers.
Is 8B significant? We expect the compiler to load 8B and then slice it
out? Can the CPU do that? We're not expecting sinfo to be misaligned
(e.g. placed directly after xdp_buff), right?
On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
quoted
On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski [off-list ref] wrote:
quoted
quoted
ack, I agree. I will fix it in v10.
Why is XDP mb incompatible with LRO? I thought that was one of the use
cases (mentioned by Willem IIRC).
XDP is meant to be a per packet operation with support for TX and
REDIRECT, and LRO isn't routable. So we could put together a large LRO
frame but we wouldn't be able to break it apart again. If we allow
that then we are going to need a ton more exception handling added to
the XDP paths.
As far as GSO it would require setting many more fields in order to
actually make it offloadable by any hardware.
It would require more work, but TSO seems to be explicitly stated
as what the series builds towards (in the cover letter). It's fine
to make choices we'd need to redo later, I guess, I'm just trying
to understand the why.
This is also my understanding that LRO and TSO is what this patchset is
working towards.
Sorry, I don't agree or understand this requested change.
From: Lorenzo Bianconi <hidden> Date: 2021-06-29 19:18:19
On 29/06/2021 20.37, Jakub Kicinski wrote:
quoted
On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
quoted
On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski [off-list ref] wrote:
quoted
quoted
ack, I agree. I will fix it in v10.
Why is XDP mb incompatible with LRO? I thought that was one of the use
cases (mentioned by Willem IIRC).
XDP is meant to be a per packet operation with support for TX and
REDIRECT, and LRO isn't routable. So we could put together a large LRO
frame but we wouldn't be able to break it apart again. If we allow
that then we are going to need a ton more exception handling added to
the XDP paths.
As far as GSO it would require setting many more fields in order to
actually make it offloadable by any hardware.
It would require more work, but TSO seems to be explicitly stated
as what the series builds towards (in the cover letter). It's fine
to make choices we'd need to redo later, I guess, I'm just trying
to understand the why.
This is also my understanding that LRO and TSO is what this patchset is
working towards.
Sorry, I don't agree or understand this requested change.
My understanding here is to use gso_size to store paged length of the
xdp multi-buffer. When converting the xdp_frame to a skb we will need
to overwrite it to support gro/lro. Is my understanding correct?
Regards,
Lorenzo
From: Alexander Duyck <hidden> Date: 2021-06-29 20:45:22
On Tue, Jun 29, 2021 at 12:18 PM Lorenzo Bianconi
[off-list ref] wrote:
quoted
On 29/06/2021 20.37, Jakub Kicinski wrote:
quoted
On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
quoted
On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski [off-list ref] wrote:
quoted
quoted
ack, I agree. I will fix it in v10.
Why is XDP mb incompatible with LRO? I thought that was one of the use
cases (mentioned by Willem IIRC).
XDP is meant to be a per packet operation with support for TX and
REDIRECT, and LRO isn't routable. So we could put together a large LRO
frame but we wouldn't be able to break it apart again. If we allow
that then we are going to need a ton more exception handling added to
the XDP paths.
As far as GSO it would require setting many more fields in order to
actually make it offloadable by any hardware.
It would require more work, but TSO seems to be explicitly stated
as what the series builds towards (in the cover letter). It's fine
to make choices we'd need to redo later, I guess, I'm just trying
to understand the why.
This is also my understanding that LRO and TSO is what this patchset is
working towards.
Sorry, I don't agree or understand this requested change.
My understanding here is to use gso_size to store paged length of the
xdp multi-buffer. When converting the xdp_frame to a skb we will need
to overwrite it to support gro/lro. Is my understanding correct?
Yes, I was thinking just of the xdp_buff, not the xdp_frame. My focus
for right now is mostly around the Rx side of things, xdp_buff to skb,
and around the XDP_TX path. If we want to drop/move where we keep the
data length when doing the conversion I would be fine with that.
From: Magnus Karlsson <hidden> Date: 2021-07-01 07:56:55
On Wed, Jun 23, 2021 at 1:19 AM John Fastabend [off-list ref] wrote:
Lorenzo Bianconi wrote:
quoted
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.
At this point I don't think we can have a partial implementation. At
the moment we have packet capture applications and protocol parsers
running in production. If we allow this to go in staged we are going
to break those applications that make the fundamental assumption they
have access to all the data in the packet.
There will be no way to fix it when it happens. The teams running the
applications wont necessarily be able to change the network MTU. Now
it doesn't work, hard stop. This is better than it sort of works some
of the time. Worse if we get in a situation where some drivers support
partial access and others support full access the support matrix gets worse.
I think we need to get full support and access to all bytes. I believe
I said this earlier, but now we've deployed apps that really do need
access to the payloads so its not a theoritical concern anymore, but
rather a real one based on deployed BPF programs.
quoted
The main idea for the new multi-buffer layout is to reuse the same
layout used for non-linear SKB. This rely on the "skb_shared_info"
struct at the end of the first buffer to link together subsequent
buffers. Keeping the layout compatible with SKBs is also done to ease
and speedup creating an SKB from an xdp_{buff,frame}.
Converting xdp_frame to SKB and deliver it to the network stack is shown
in patch 07/14 (e.g. cpumaps).
A multi-buffer bit (mb) has been introduced in the flags field of 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 skb_shared_info structure at the end
of the first buffer will be initialized only if mb is set.
Moreover the flags field in xdp_{buff,frame} will be reused even for
xdp rx csum offloading in future series.
Typical use cases for this series are:
- Jumbo-frames
- Packet header split (please see Google���s use-case @ NetDevConf 0x14, [0])
- TSO
A new bpf helper (bpf_xdp_get_buff_len) has been introduce in order to notify
the eBPF layer about the total frame size (linear + paged parts).
Is it possible to make currently working programs continue to work?
For a simple packet capture example a program might capture the
entire packet of bytes '(data_end - data_start)'. With above implementation
the program will continue to run, but will no longer be capturing
all the bytes... so its a silent failure. Otherwise I'll need to
backport fixes into my BPF programs and releases to ensure they
don't walk onto a new kernel with multi-buffer support enabled.
Its not ideal.
quoted
bpf_xdp_adjust_tail and bpf_xdp_copy helpers have been modified to take into
account xdp multi-buff frames.
More info about the main idea behind this approach can be found here [1][2].
Will read [1],[2].
Where did the perf data for the 40gbps NIC go? I think we want that
done again on this series with at least 40gbps NICs and better
yet 100gbps drivers. If its addressed in a patch commit message
I'm reading the series now.
Here is the perf data for a 40 gbps i40e on my 2.1 GHz Cascade Lake server.
xdpsock -r XDP_DROP XDP_TX
Lorenzo's patches: -2%/+1.5 cycles -3%/+3 +2%/-6 (Yes,
it gets better!)
+ i40e support: -5.5%/+5 -8%/+9 -9%/+31
It seems that it is the driver support itself that hurts now. The
overhead of the base support has decreased substantially over time
which is good.
quoted
Changes since v8:
- add proper dma unmapping if XDP_TX fails on mvneta for a xdp multi-buff
- switch back to skb_shared_info implementation from previous xdp_shared_info
one
- avoid using a bietfield in xdp_buff/xdp_frame since it introduces performance
regressions. Tested now on 10G NIC (ixgbe) to verify there are no performance
penalties for regular codebase
- add bpf_xdp_get_buff_len helper and remove frame_length field in xdp ctx
- add data_len field in skb_shared_info struct
Changes since v7:
- rebase on top of bpf-next
- fix sparse warnings
- improve comments for frame_length in include/net/xdp.h
Changes since v6:
- the main difference respect to previous versions is the new approach proposed
by Eelco to pass full length of the packet to eBPF layer in XDP context
- reintroduce multi-buff support to eBPF kself-tests
- reintroduce multi-buff support to bpf_xdp_adjust_tail helper
- introduce multi-buffer support to bpf_xdp_copy helper
- rebase on top of bpf-next
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 (3):
bpf: add multi-buff support to the bpf_xdp_adjust_tail() API
bpf: add multi-buffer support to xdp copy helpers
bpf: update xdp_adjust_tail selftest to include multi-buffer
Lorenzo Bianconi (11):
net: skbuff: add data_len field to skb_shared_info
xdp: introduce flags field in xdp_buff/xdp_frame
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_frame
bpf: introduce bpf_xdp_get_buff_len helper
bpf: move user_size out of bpf_test_init
bpf: introduce multibuff support to bpf_prog_test_run_xdp()
bpf: test_run: add xdp_shared_info pointer in bpf_test_finish
signature
drivers/net/ethernet/marvell/mvneta.c | 143 ++++++++++------
include/linux/skbuff.h | 5 +-
include/net/xdp.h | 56 ++++++-
include/uapi/linux/bpf.h | 7 +
kernel/trace/bpf_trace.c | 3 +
net/bpf/test_run.c | 108 +++++++++---
net/core/filter.c | 157 +++++++++++++++++-
net/core/xdp.c | 72 +++++++-
tools/include/uapi/linux/bpf.h | 7 +
.../bpf/prog_tests/xdp_adjust_tail.c | 105 ++++++++++++
.../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 127 +++++++++-----
.../bpf/progs/test_xdp_adjust_tail_grow.c | 10 +-
.../bpf/progs/test_xdp_adjust_tail_shrink.c | 32 +++-
.../selftests/bpf/progs/test_xdp_bpf2bpf.c | 2 +-
14 files changed, 705 insertions(+), 129 deletions(-)
--
2.31.1
From: Lorenzo Bianconi <hidden> Date: 2021-07-05 15:52:59
On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
quoted
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
Introduce flags field in xdp_frame/xdp_buffer data structure
to define additional buffer features. At the moment the only
supported buffer feature is multi-buffer bit (mb). Multi-buffer bit
is used 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>
Instead of passing this between buffers and frames I wonder if this
wouldn't be better to place in something like the xdp_mem_info
structure since this is something that would be specific to how the
device is handling memory anyway. You could probably split the type
field into a 16b type and a 16b flags field. Then add your bit where 0
is linear/legacy and 1 is scatter-gather/multi-buffer.
ack, this should be fine but I put the flag field in xdp_buff/xdp_frame
in order to reuse it for some xdp hw-hints (e.g rx checksum type).
We can put it in xdp_mem_info too but I guess it would be less intuitive, what
do you think?
I think it makes the most sense in xdp_mem_info. It already tells us
what to expect in some respect in regards to memory layout as it tells
us if we are dealing with shared pages or whole pages and how to
recycle them. I would think that applies almost identically to
scatter-gather XDP the same way.
Hi Alex,
Reviewing the code to address this comment I think I spotted a corner case
where we can't use this approach. Whenever we run dev_map_bpf_prog_run()
we loose mb info converting xdp_frame to xdp_buff since
xdp_convert_frame_to_buff() does not copy it and we have no xdp_rxq_info there.
Do you think we should add a rxq_info there similar to what we did for cpumap?
I think it is better to keep the previous approach since it seems cleaner and
reusable in the future. What do you think?
Regards,
Lorenzo
As far as the addition of flags there is still time for that later as
we still have the 32b of unused space after frame_sz.
From: Alexander Duyck <hidden> Date: 2021-07-05 21:36:12
On Mon, Jul 5, 2021 at 8:52 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
quoted
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi [off-list ref] wrote:
quoted
Introduce flags field in xdp_frame/xdp_buffer data structure
to define additional buffer features. At the moment the only
supported buffer feature is multi-buffer bit (mb). Multi-buffer bit
is used 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>
Instead of passing this between buffers and frames I wonder if this
wouldn't be better to place in something like the xdp_mem_info
structure since this is something that would be specific to how the
device is handling memory anyway. You could probably split the type
field into a 16b type and a 16b flags field. Then add your bit where 0
is linear/legacy and 1 is scatter-gather/multi-buffer.
ack, this should be fine but I put the flag field in xdp_buff/xdp_frame
in order to reuse it for some xdp hw-hints (e.g rx checksum type).
We can put it in xdp_mem_info too but I guess it would be less intuitive, what
do you think?
I think it makes the most sense in xdp_mem_info. It already tells us
what to expect in some respect in regards to memory layout as it tells
us if we are dealing with shared pages or whole pages and how to
recycle them. I would think that applies almost identically to
scatter-gather XDP the same way.
Hi Alex,
Reviewing the code to address this comment I think I spotted a corner case
where we can't use this approach. Whenever we run dev_map_bpf_prog_run()
we loose mb info converting xdp_frame to xdp_buff since
xdp_convert_frame_to_buff() does not copy it and we have no xdp_rxq_info there.
Do you think we should add a rxq_info there similar to what we did for cpumap?
I think it is better to keep the previous approach since it seems cleaner and
reusable in the future. What do you think?
Hi Lorenzo,
What about doing something like breaking up the type value in
xdp_mem_info? The fact is having it as an enum doesn't get us much
since we have a 32b type field but are only storing 4 possible values
there currently
The way I see it, scatter-gather is just another memory model
attribute rather than being something entirely new. It makes as much
sense to have a bit there for MEM_TYPE_PAGE_SG as it does for
MEM_TYPE_PAGE_SHARED. I would consider either splitting the type field
into two 16b fields. For example you might have one field that
describes the source pool which is currently either allocated page
(ORDER0, SHARED), page_pool (PAGE_POOL), or XSK pool (XSK_BUFF_POOL),
and then two flags for type with there being either shared and/or
scatter-gather.
Also, looking over the code I don't see any reason why current
ORDER0/SHARED couldn't be merged as the free paths are essentially
identical since the MEM_TYPE_PAGE_SHARED path would function perfectly
fine to free MEM_TYPE_PAGE_ORDER0 pages.
Thanks,
- Alex
From: Eelco Chaudron <echaudro@redhat.com>
This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Ah ok so at least xdp_output will work with all bytes. But this is
getting close to having access into the frags so I think doing
the last bit shouldn't be too hard?
Guess you are talking about multi-buffer access in the XDP program?
I did suggest an API a while back, https://lore.kernel.org/bpf/FD3E6E08-DE78-4FBA-96F6-646C93E88631@redhat.com/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.
Ah great I think we had the same idea I called it xdp_pull_data()
though.
Whats the complication though it looks like it can be done by simply
moving the data and data_end pointers around then marking them
invalidated. This way the verifier knows the program needs to
rewrite them. I can probably look more into next week.
Sorry for the late response, but I did do a POC a while back with changing the data and data_end pointers, and this worked. The problem that got raised at the time was that it was not hiding the implementation. i.e. you had to put in the fragment number, and so you needed to know how many fragments existed and the size of each one.
With the API suggested in the above email link, I was trying to avoid this. But it needs a lot of work in the verifier I guess.
From my first glance it looks relatively straight forward to do
now. I really would like to avoid yet another iteration of
programs features I have to discover and somehow work around
if we can get the helper into this series. If you really don't
have time I can probably take a look early next week on an
RFC for something like above helper.
From: Lorenzo Bianconi <hidden> Date: 2021-07-06 12:14:39
On Mon, Jul 5, 2021 at 8:52 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
quoted
On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
[off-list ref] wrote:
[...]
Hi Lorenzo,
What about doing something like breaking up the type value in
xdp_mem_info? The fact is having it as an enum doesn't get us much
since we have a 32b type field but are only storing 4 possible values
there currently
The way I see it, scatter-gather is just another memory model
attribute rather than being something entirely new. It makes as much
sense to have a bit there for MEM_TYPE_PAGE_SG as it does for
MEM_TYPE_PAGE_SHARED. I would consider either splitting the type field
into two 16b fields. For example you might have one field that
describes the source pool which is currently either allocated page
(ORDER0, SHARED), page_pool (PAGE_POOL), or XSK pool (XSK_BUFF_POOL),
and then two flags for type with there being either shared and/or
scatter-gather.
Hi Alex,
I am fine reducing the xdp_mem_info size defining type field as u16 instead of
u32 but I think mb is a per-xdp_buff/xdp_frame property since at runtime we can
receive a tiny single page xdp_buff/xdp_frame and a "jumbo" xdp_buff/xdp_frame
composed by multiple pages. According to the documentation available in
include/net/xdp.h, xdp_rxq_info (where xdp_mem_info is contained for xdp_buff)
is "associated with the driver level RX-ring queues and it is information that
is specific to how the driver have configured a given RX-ring queue" so I guess
it is a little bit counterintuitive to add this info there.
Moreover we have the "issue" for devmap in dev_map_bpf_prog_run() when we
perform XDP_REDIRECT with the approach you proposed and last we can reuse this
new flags filed for XDP hw-hints support.
What about reducing xdp_mem_info and add the flags field in xdp_buff/xdp_frame
in order to avoid increasing the xdp_buff/xdp_frame size? Am I missing
something?
Regards,
Lorenzo
Also, looking over the code I don't see any reason why current
ORDER0/SHARED couldn't be merged as the free paths are essentially
identical since the MEM_TYPE_PAGE_SHARED path would function perfectly
fine to free MEM_TYPE_PAGE_ORDER0 pages.
Thanks,
- Alex
From: Alexander Duyck <hidden> Date: 2021-07-06 15:02:55
On Tue, Jul 6, 2021 at 4:53 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
On Mon, Jul 5, 2021 at 8:52 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
quoted
On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
[off-list ref] wrote:
[...]
quoted
Hi Lorenzo,
What about doing something like breaking up the type value in
xdp_mem_info? The fact is having it as an enum doesn't get us much
since we have a 32b type field but are only storing 4 possible values
there currently
The way I see it, scatter-gather is just another memory model
attribute rather than being something entirely new. It makes as much
sense to have a bit there for MEM_TYPE_PAGE_SG as it does for
MEM_TYPE_PAGE_SHARED. I would consider either splitting the type field
into two 16b fields. For example you might have one field that
describes the source pool which is currently either allocated page
(ORDER0, SHARED), page_pool (PAGE_POOL), or XSK pool (XSK_BUFF_POOL),
and then two flags for type with there being either shared and/or
scatter-gather.
Hi Alex,
I am fine reducing the xdp_mem_info size defining type field as u16 instead of
u32 but I think mb is a per-xdp_buff/xdp_frame property since at runtime we can
receive a tiny single page xdp_buff/xdp_frame and a "jumbo" xdp_buff/xdp_frame
composed by multiple pages. According to the documentation available in
include/net/xdp.h, xdp_rxq_info (where xdp_mem_info is contained for xdp_buff)
is "associated with the driver level RX-ring queues and it is information that
is specific to how the driver have configured a given RX-ring queue" so I guess
it is a little bit counterintuitive to add this info there.
It isn't really all that counterintuitive. However it does put the
onus on the driver to be consistent about things. So even a
single-buffer xdp_buff would technically have to be a scatter-gather
buff, but it would have no fragments in it. So the requirement would
be to initialize the frags and data_len fields to 0 for all xdp_buff
structures.
Moreover we have the "issue" for devmap in dev_map_bpf_prog_run() when we
perform XDP_REDIRECT with the approach you proposed and last we can reuse this
new flags filed for XDP hw-hints support.
What about reducing xdp_mem_info and add the flags field in xdp_buff/xdp_frame
in order to avoid increasing the xdp_buff/xdp_frame size? Am I missing
something?
The problem is there isn't a mem_info field in the xdp_buff. It is in
the Rx queue info structure.
Thanks,
- Alex
From: Lorenzo Bianconi <hidden> Date: 2021-07-06 17:47:38
On Tue, Jul 6, 2021 at 4:53 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
quoted
On Mon, Jul 5, 2021 at 8:52 AM Lorenzo Bianconi
[off-list ref] wrote:
quoted
quoted
On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
[off-list ref] wrote:
[...]
quoted
Hi Lorenzo,
What about doing something like breaking up the type value in
xdp_mem_info? The fact is having it as an enum doesn't get us much
since we have a 32b type field but are only storing 4 possible values
there currently
The way I see it, scatter-gather is just another memory model
attribute rather than being something entirely new. It makes as much
sense to have a bit there for MEM_TYPE_PAGE_SG as it does for
MEM_TYPE_PAGE_SHARED. I would consider either splitting the type field
into two 16b fields. For example you might have one field that
describes the source pool which is currently either allocated page
(ORDER0, SHARED), page_pool (PAGE_POOL), or XSK pool (XSK_BUFF_POOL),
and then two flags for type with there being either shared and/or
scatter-gather.
Hi Alex,
I am fine reducing the xdp_mem_info size defining type field as u16 instead of
u32 but I think mb is a per-xdp_buff/xdp_frame property since at runtime we can
receive a tiny single page xdp_buff/xdp_frame and a "jumbo" xdp_buff/xdp_frame
composed by multiple pages. According to the documentation available in
include/net/xdp.h, xdp_rxq_info (where xdp_mem_info is contained for xdp_buff)
is "associated with the driver level RX-ring queues and it is information that
is specific to how the driver have configured a given RX-ring queue" so I guess
it is a little bit counterintuitive to add this info there.
It isn't really all that counterintuitive. However it does put the
onus on the driver to be consistent about things. So even a
single-buffer xdp_buff would technically have to be a scatter-gather
buff, but it would have no fragments in it. So the requirement would
be to initialize the frags and data_len fields to 0 for all xdp_buff
structures.
nr_frags and data_len are currently defined in skb_shared_info(xdp_buff)
so I guess initialize them to 0 will trigger a cache miss (in fact we
introduced the mb bit just to avoid this initialization and introduce
penalties for legacy single-buffer use-case). Do you mean to have these
fields in xdp_buff/xdp_frame structure?
quoted
Moreover we have the "issue" for devmap in dev_map_bpf_prog_run() when we
perform XDP_REDIRECT with the approach you proposed and last we can reuse this
new flags filed for XDP hw-hints support.
What about reducing xdp_mem_info and add the flags field in xdp_buff/xdp_frame
in order to avoid increasing the xdp_buff/xdp_frame size? Am I missing
something?
The problem is there isn't a mem_info field in the xdp_buff. It is in
the Rx queue info structure.
Changing the subject to address this point specifically:
Right that was my conclusion as well. Existing programs might have
subtle side effects if they start running on multibuffer drivers as
is. I don't have any good ideas though on how to handle this.
So I had a chat about this with Lorenzo, Eelco and Jesper today, and
promised I'd summarise our discussion to you all, so this is my attempt
at that. Please excuse the long email, I'm just trying to be
comprehensive :)
So first off, a problem description: If an existing XDP program is
exposed to an xdp_buff that is really a multi-buffer, it may end up with
subtle and hard-to-debug bugs: If it's parsing the packet it'll only see
part of the payload and not be aware of that fact, and if it's
calculating the packet length, that will also only be wrong (only
counting the first fragment).
So what to do about this? First of all, to do anything about it, XDP
programs need to be able to declare themselves "multi-buffer aware" (but
see point 1 below). We could try to auto-detect it in the verifier by
which helpers the program is using, but since existing programs could be
perfectly happy to just keep running, it probably needs to be something
the program communicates explicitly. One option is to use the
expected_attach_type to encode this; programs can then declare it in the
source by section name, or the userspace loader can set the type for
existing programs if needed.
With this, the kernel will know if a given XDP program is multi-buff
aware and can decide what to do with that information. For this we came
up with basically three options:
1. Do nothing. This would make it up to users / sysadmins to avoid
anything breaking by manually making sure to not enable multi-buffer
support while loading any XDP programs that will malfunction if
presented with an mb frame. This will probably break in interesting
ways, but it's nice and simple from an implementation PoV. With this
we don't need the declaration discussed above either.
2. Add a check at runtime and drop the frames if they are mb-enabled and
the program doesn't understand it. This is relatively simple to
implement, but it also makes for difficult-to-understand issues (why
are my packets suddenly being dropped?), and it will incur runtime
overhead.
3. Reject loading of programs that are not MB-aware when running in an
MB-enabled mode. This would make things break in more obvious ways,
and still allow a userspace loader to declare a program "MB-aware" to
force it to run if necessary. The problem then becomes at what level
to block this?
Doing this at the driver level is not enough: while a particular
driver knows if it's running in multi-buff mode, we can't know for
sure if a particular XDP program is multi-buff aware at attach time:
it could be tail-calling other programs, or redirecting packets to
another interface where it will be processed by a non-MB aware
program.
So another option is to make it a global toggle: e.g., create a new
sysctl to enable multi-buffer. If this is set, reject loading any XDP
program that doesn't support multi-buffer mode, and if it's unset,
disable multi-buffer mode in all drivers. This will make it explicit
when the multi-buffer mode is used, and prevent any accidental subtle
malfunction of existing XDP programs. The drawback is that it's a
mode switch, so more configuration complexity.
None of these options are ideal, of course, but I hope the above
explanation at least makes sense. If anyone has any better ideas (or can
spot any flaws in the reasoning above) please don't hesitate to let us
know!
-Toke