| Message ID | 20230529110608.597534-2-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | multi-buffer support for XDP_REDIRECT samples | expand |
On 29/05/2023 13.06, Tariq Toukan wrote: > Expand the xdp multi-buffer support to xdp_redirect tool. > Similar to what's done in commit > 772251742262 ("samples/bpf: fixup some tools to be able to support xdp multibuffer") > and its fix commit > 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern"). > Have you tested if this cause a performance degradation? (Also found possible bug below) > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> > Reviewed-by: Nimrod Oren <noren@nvidia.com> > --- > samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/samples/bpf/xdp_redirect.bpf.c b/samples/bpf/xdp_redirect.bpf.c > index 7c02bacfe96b..620163eb7e19 100644 > --- a/samples/bpf/xdp_redirect.bpf.c > +++ b/samples/bpf/xdp_redirect.bpf.c > @@ -16,16 +16,21 @@ > > const volatile int ifindex_out; > > -SEC("xdp") > +#define XDPBUFSIZE 64 Pktgen sample scripts will default send with 60 pkt length, because the 4 bytes FCS (end-frame checksum) is added by hardware. Will this result in an error when bpf_xdp_load_bytes() tries to copy 64 bytes from a 60 bytes packet? > +SEC("xdp.frags") > int xdp_redirect_prog(struct xdp_md *ctx) > { > - void *data_end = (void *)(long)ctx->data_end; > - void *data = (void *)(long)ctx->data; > + __u8 pkt[XDPBUFSIZE] = {}; > + void *data_end = &pkt[XDPBUFSIZE-1]; > + void *data = pkt; > u32 key = bpf_get_smp_processor_id(); > struct ethhdr *eth = data; > struct datarec *rec; > u64 nh_off; > > + if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt))) > + return XDP_DROP; E.g. sizeof(pkt) = 64 bytes here. > + > nh_off = sizeof(*eth); > if (data + nh_off > data_end) > return XDP_DROP; > @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx) > NO_TEAR_INC(rec->processed); > > swap_src_dst_mac(data); > + if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt))) > + return XDP_DROP; > + > return bpf_redirect(ifindex_out, 0); > } > > /* Redirect require an XDP bpf_prog loaded on the TX device */ > -SEC("xdp") > +SEC("xdp.frags") > int xdp_redirect_dummy_prog(struct xdp_md *ctx) > { > return XDP_PASS;
On 30/05/2023 14:33, Jesper Dangaard Brouer wrote: > > > On 29/05/2023 13.06, Tariq Toukan wrote: >> Expand the xdp multi-buffer support to xdp_redirect tool. >> Similar to what's done in commit >> 772251742262 ("samples/bpf: fixup some tools to be able to support xdp >> multibuffer") >> and its fix commit >> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern"). >> > > Have you tested if this cause a performance degradation? > > (Also found possible bug below) > Hi Jesper, This introduces the same known perf degradation we already have in xdp1 and xdp2. Unfortunately, this is the API we have today to safely support multi-buffer. Note that both perf and functional (noted below) degradation should be eliminated once replacing the load/store operations with dynptr logic that returns a pointer to the scatter entry instead of copying it. I initiated a discussion on this topic a few months ago. dynptr was accepted since then, but I'm not aware of any in-progress followup work that addresses this. >> Signed-off-by: Tariq Toukan <tariqt@nvidia.com> >> Reviewed-by: Nimrod Oren <noren@nvidia.com> >> --- >> samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/samples/bpf/xdp_redirect.bpf.c >> b/samples/bpf/xdp_redirect.bpf.c >> index 7c02bacfe96b..620163eb7e19 100644 >> --- a/samples/bpf/xdp_redirect.bpf.c >> +++ b/samples/bpf/xdp_redirect.bpf.c >> @@ -16,16 +16,21 @@ >> const volatile int ifindex_out; >> -SEC("xdp") >> +#define XDPBUFSIZE 64 > > Pktgen sample scripts will default send with 60 pkt length, because the > 4 bytes FCS (end-frame checksum) is added by hardware. > > Will this result in an error when bpf_xdp_load_bytes() tries to copy 64 > bytes from a 60 bytes packet? > Yes. This can be resolved by reducing XDPBUFSIZE to 60. Need to check if it's OK to disregard these last 4 bytes without hurting the XDP program logic. If so, do you suggest changing xdp1 and xdp2 as well? >> +SEC("xdp.frags") >> int xdp_redirect_prog(struct xdp_md *ctx) >> { >> - void *data_end = (void *)(long)ctx->data_end; >> - void *data = (void *)(long)ctx->data; >> + __u8 pkt[XDPBUFSIZE] = {}; >> + void *data_end = &pkt[XDPBUFSIZE-1]; >> + void *data = pkt; >> u32 key = bpf_get_smp_processor_id(); >> struct ethhdr *eth = data; >> struct datarec *rec; >> u64 nh_off; >> + if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt))) >> + return XDP_DROP; > > E.g. sizeof(pkt) = 64 bytes here. > >> + >> nh_off = sizeof(*eth); >> if (data + nh_off > data_end) >> return XDP_DROP; >> @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx) >> NO_TEAR_INC(rec->processed); >> swap_src_dst_mac(data); >> + if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt))) >> + return XDP_DROP; >> + >> return bpf_redirect(ifindex_out, 0); >> } >> /* Redirect require an XDP bpf_prog loaded on the TX device */ >> -SEC("xdp") >> +SEC("xdp.frags") >> int xdp_redirect_dummy_prog(struct xdp_md *ctx) >> { >> return XDP_PASS; >
On 30/05/2023 14.17, Tariq Toukan wrote: > > On 30/05/2023 14:33, Jesper Dangaard Brouer wrote: >> >> >> On 29/05/2023 13.06, Tariq Toukan wrote: >>> Expand the xdp multi-buffer support to xdp_redirect tool. >>> Similar to what's done in commit >>> 772251742262 ("samples/bpf: fixup some tools to be able to support >>> xdp multibuffer") >>> and its fix commit >>> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern"). >>> >> >> Have you tested if this cause a performance degradation? >> >> (Also found possible bug below) >> > > Hi Jesper, > > This introduces the same known perf degradation we already have in xdp1 > and xdp2. Did a quick test with xdp1, the performance degradation is around 18%. Before: 22,917,961 pps After: 18,798,336 pps (1-(18798336/22917961))*100 = 17.97% > Unfortunately, this is the API we have today to safely support > multi-buffer. > Note that both perf and functional (noted below) degradation should be > eliminated once replacing the load/store operations with dynptr logic > that returns a pointer to the scatter entry instead of copying it. > Well, should we use dynptr logic in this patch then? Does it make sense to add sample code that does thing in a way that is sub-optimal and we want to replace? ... (I fear people will copy paste the sample code). > I initiated a discussion on this topic a few months ago. dynptr was > accepted since then, but I'm not aware of any in-progress followup work > that addresses this. > Are you saying some more work is needed on dynptr? >>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com> >>> Reviewed-by: Nimrod Oren <noren@nvidia.com> >>> --- >>> samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++---- >>> 1 file changed, 12 insertions(+), 4 deletions(-) >>> >>> diff --git a/samples/bpf/xdp_redirect.bpf.c >>> b/samples/bpf/xdp_redirect.bpf.c >>> index 7c02bacfe96b..620163eb7e19 100644 >>> --- a/samples/bpf/xdp_redirect.bpf.c >>> +++ b/samples/bpf/xdp_redirect.bpf.c >>> @@ -16,16 +16,21 @@ >>> const volatile int ifindex_out; >>> -SEC("xdp") >>> +#define XDPBUFSIZE 64 >> >> Pktgen sample scripts will default send with 60 pkt length, because the >> 4 bytes FCS (end-frame checksum) is added by hardware. >> >> Will this result in an error when bpf_xdp_load_bytes() tries to copy 64 >> bytes from a 60 bytes packet? >> > > Yes. > > This can be resolved by reducing XDPBUFSIZE to 60. > Need to check if it's OK to disregard these last 4 bytes without hurting > the XDP program logic. > > If so, do you suggest changing xdp1 and xdp2 as well? > I can take care of reducing XDPBUFSIZE to 60 on xpd1 and xdp2, as I already had to make these changes for the above quick bench work ;-) I'll send out patches shortly. >>> +SEC("xdp.frags") >>> int xdp_redirect_prog(struct xdp_md *ctx) >>> { >>> - void *data_end = (void *)(long)ctx->data_end; >>> - void *data = (void *)(long)ctx->data; >>> + __u8 pkt[XDPBUFSIZE] = {}; >>> + void *data_end = &pkt[XDPBUFSIZE-1]; >>> + void *data = pkt; >>> u32 key = bpf_get_smp_processor_id(); >>> struct ethhdr *eth = data; >>> struct datarec *rec; >>> u64 nh_off; >>> + if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt))) >>> + return XDP_DROP; >> >> E.g. sizeof(pkt) = 64 bytes here. >> >>> + >>> nh_off = sizeof(*eth); >>> if (data + nh_off > data_end) >>> return XDP_DROP; >>> @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx) >>> NO_TEAR_INC(rec->processed); >>> swap_src_dst_mac(data); >>> + if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt))) >>> + return XDP_DROP; >>> + >>> return bpf_redirect(ifindex_out, 0); >>> } >>> /* Redirect require an XDP bpf_prog loaded on the TX device */ >>> -SEC("xdp") >>> +SEC("xdp.frags") >>> int xdp_redirect_dummy_prog(struct xdp_md *ctx) >>> { >>> return XDP_PASS; >> >
On 30/05/2023 15:40, Jesper Dangaard Brouer wrote: > > > On 30/05/2023 14.17, Tariq Toukan wrote: >> >> On 30/05/2023 14:33, Jesper Dangaard Brouer wrote: >>> >>> >>> On 29/05/2023 13.06, Tariq Toukan wrote: >>>> Expand the xdp multi-buffer support to xdp_redirect tool. >>>> Similar to what's done in commit >>>> 772251742262 ("samples/bpf: fixup some tools to be able to support >>>> xdp multibuffer") >>>> and its fix commit >>>> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern"). >>>> >>> >>> Have you tested if this cause a performance degradation? >>> >>> (Also found possible bug below) >>> >> >> Hi Jesper, >> >> This introduces the same known perf degradation we already have in >> xdp1 and xdp2. > > Did a quick test with xdp1, the performance degradation is around 18%. > > Before: 22,917,961 pps > After: 18,798,336 pps > > (1-(18798336/22917961))*100 = 17.97% > > >> Unfortunately, this is the API we have today to safely support >> multi-buffer. >> Note that both perf and functional (noted below) degradation should be >> eliminated once replacing the load/store operations with dynptr logic >> that returns a pointer to the scatter entry instead of copying it. >> > > Well, should we use dynptr logic in this patch then? > AFAIU it's not there ready to be used... Not sure what parts are missing, I'll need to review it a bit deeper. > Does it make sense to add sample code that does thing in a way that is > sub-optimal and we want to replace? > ... (I fear people will copy paste the sample code). > I get your point. As xdp1 and xdp2 are already there, I thought that we'd want to expose multi-buffer samples in XDP_REDIRECT as well. We use these samples for internal testing. >> I initiated a discussion on this topic a few months ago. dynptr was >> accepted since then, but I'm not aware of any in-progress followup >> work that addresses this. >> > > Are you saying some more work is needed on dynptr? > AFAIU yes. But I might be wrong... I need to revisit this. Do you think/know that dynptr can be used immediately? >>>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com> >>>> Reviewed-by: Nimrod Oren <noren@nvidia.com> >>>> --- >>>> samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++---- >>>> 1 file changed, 12 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/samples/bpf/xdp_redirect.bpf.c >>>> b/samples/bpf/xdp_redirect.bpf.c >>>> index 7c02bacfe96b..620163eb7e19 100644 >>>> --- a/samples/bpf/xdp_redirect.bpf.c >>>> +++ b/samples/bpf/xdp_redirect.bpf.c >>>> @@ -16,16 +16,21 @@ >>>> const volatile int ifindex_out; >>>> -SEC("xdp") >>>> +#define XDPBUFSIZE 64 >>> >>> Pktgen sample scripts will default send with 60 pkt length, because the >>> 4 bytes FCS (end-frame checksum) is added by hardware. >>> >>> Will this result in an error when bpf_xdp_load_bytes() tries to copy 64 >>> bytes from a 60 bytes packet? >>> >> >> Yes. >> >> This can be resolved by reducing XDPBUFSIZE to 60. >> Need to check if it's OK to disregard these last 4 bytes without >> hurting the XDP program logic. >> >> If so, do you suggest changing xdp1 and xdp2 as well? >> > > I can take care of reducing XDPBUFSIZE to 60 on xpd1 and xdp2, as I > already had to make these changes for the above quick bench work ;-) > I'll send out patches shortly. > > Thanks. Are we fine with the above? Should we just change the array size to 60 and re-spin? >>>> +SEC("xdp.frags") >>>> int xdp_redirect_prog(struct xdp_md *ctx) >>>> { >>>> - void *data_end = (void *)(long)ctx->data_end; >>>> - void *data = (void *)(long)ctx->data; >>>> + __u8 pkt[XDPBUFSIZE] = {}; >>>> + void *data_end = &pkt[XDPBUFSIZE-1]; >>>> + void *data = pkt; >>>> u32 key = bpf_get_smp_processor_id(); >>>> struct ethhdr *eth = data; >>>> struct datarec *rec; >>>> u64 nh_off; >>>> + if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt))) >>>> + return XDP_DROP; >>> >>> E.g. sizeof(pkt) = 64 bytes here. >>> >>>> + >>>> nh_off = sizeof(*eth); >>>> if (data + nh_off > data_end) >>>> return XDP_DROP; >>>> @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx) >>>> NO_TEAR_INC(rec->processed); >>>> swap_src_dst_mac(data); >>>> + if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt))) >>>> + return XDP_DROP; >>>> + >>>> return bpf_redirect(ifindex_out, 0); >>>> } >>>> /* Redirect require an XDP bpf_prog loaded on the TX device */ >>>> -SEC("xdp") >>>> +SEC("xdp.frags") >>>> int xdp_redirect_dummy_prog(struct xdp_md *ctx) >>>> { >>>> return XDP_PASS; >>> >> >
Tariq Toukan <ttoukan.linux@gmail.com> writes: > On 30/05/2023 15:40, Jesper Dangaard Brouer wrote: >> >> >> On 30/05/2023 14.17, Tariq Toukan wrote: >>> >>> On 30/05/2023 14:33, Jesper Dangaard Brouer wrote: >>>> >>>> >>>> On 29/05/2023 13.06, Tariq Toukan wrote: >>>>> Expand the xdp multi-buffer support to xdp_redirect tool. >>>>> Similar to what's done in commit >>>>> 772251742262 ("samples/bpf: fixup some tools to be able to support >>>>> xdp multibuffer") >>>>> and its fix commit >>>>> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern"). >>>>> >>>> >>>> Have you tested if this cause a performance degradation? >>>> >>>> (Also found possible bug below) >>>> >>> >>> Hi Jesper, >>> >>> This introduces the same known perf degradation we already have in >>> xdp1 and xdp2. >> >> Did a quick test with xdp1, the performance degradation is around 18%. >> >> Before: 22,917,961 pps >> After: 18,798,336 pps >> >> (1-(18798336/22917961))*100 = 17.97% >> >> >>> Unfortunately, this is the API we have today to safely support >>> multi-buffer. >>> Note that both perf and functional (noted below) degradation should be >>> eliminated once replacing the load/store operations with dynptr logic >>> that returns a pointer to the scatter entry instead of copying it. >>> >> >> Well, should we use dynptr logic in this patch then? >> > > AFAIU it's not there ready to be used... > Not sure what parts are missing, I'll need to review it a bit deeper. > >> Does it make sense to add sample code that does thing in a way that is >> sub-optimal and we want to replace? >> ... (I fear people will copy paste the sample code). >> > > I get your point. > As xdp1 and xdp2 are already there, I thought that we'd want to expose > multi-buffer samples in XDP_REDIRECT as well. We use these samples for > internal testing. Note that I am planning to send a patch to remove these utilities in the not-so distant future. We have merged the xdp-bench utility into xdp-tools as of v1.3.0, and that should contain all functionality of both the xdp1/2 utilities and the xdp_redirect* utilities, without being dependent on the (slowly bitrotting) samples/bpf directory. The only reason I haven't sent the patch to remove the utilities yet is that I haven't yet merged the multibuf support (WiP PR here: https://github.com/xdp-project/xdp-tools/pull/314). I'll try to move that up on my list of things to get done, but in the meantime I'd advice against expending too much effort on improving these tools :) -Toke
On 5/30/23 6:40 AM, Tariq Toukan wrote: >>> I initiated a discussion on this topic a few months ago. dynptr was accepted >>> since then, but I'm not aware of any in-progress followup work that addresses >>> this. >>> >> >> Are you saying some more work is needed on dynptr? >> > > AFAIU yes. > But I might be wrong... I need to revisit this. > Do you think/know that dynptr can be used immediately? Not sure if you are aware of the bpf_dynptr_slice[_rdwr]() which could be useful here. It only does a copy when the requested slice is across different frags: https://lore.kernel.org/all/20230301154953.641654-10-joannelkoong@gmail.com/
On 30/05/2023 20:22, Martin KaFai Lau wrote: > On 5/30/23 6:40 AM, Tariq Toukan wrote: >>>> I initiated a discussion on this topic a few months ago. dynptr was >>>> accepted since then, but I'm not aware of any in-progress followup >>>> work that addresses this. >>>> >>> >>> Are you saying some more work is needed on dynptr? >>> >> >> AFAIU yes. >> But I might be wrong... I need to revisit this. >> Do you think/know that dynptr can be used immediately? > > Not sure if you are aware of the bpf_dynptr_slice[_rdwr]() which could > be useful here. It only does a copy when the requested slice is across > different frags: > https://lore.kernel.org/all/20230301154953.641654-10-joannelkoong@gmail.com/ > Thanks for the pointer. I missed it. Looks promising! I'll take a deeper look soon and do some perf tests.
diff --git a/samples/bpf/xdp_redirect.bpf.c b/samples/bpf/xdp_redirect.bpf.c index 7c02bacfe96b..620163eb7e19 100644 --- a/samples/bpf/xdp_redirect.bpf.c +++ b/samples/bpf/xdp_redirect.bpf.c @@ -16,16 +16,21 @@ const volatile int ifindex_out; -SEC("xdp") +#define XDPBUFSIZE 64 +SEC("xdp.frags") int xdp_redirect_prog(struct xdp_md *ctx) { - void *data_end = (void *)(long)ctx->data_end; - void *data = (void *)(long)ctx->data; + __u8 pkt[XDPBUFSIZE] = {}; + void *data_end = &pkt[XDPBUFSIZE-1]; + void *data = pkt; u32 key = bpf_get_smp_processor_id(); struct ethhdr *eth = data; struct datarec *rec; u64 nh_off; + if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt))) + return XDP_DROP; + nh_off = sizeof(*eth); if (data + nh_off > data_end) return XDP_DROP; @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx) NO_TEAR_INC(rec->processed); swap_src_dst_mac(data); + if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt))) + return XDP_DROP; + return bpf_redirect(ifindex_out, 0); } /* Redirect require an XDP bpf_prog loaded on the TX device */ -SEC("xdp") +SEC("xdp.frags") int xdp_redirect_dummy_prog(struct xdp_md *ctx) { return XDP_PASS;
Expand the xdp multi-buffer support to xdp_redirect tool. Similar to what's done in commit 772251742262 ("samples/bpf: fixup some tools to be able to support xdp multibuffer") and its fix commit 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern"). Signed-off-by: Tariq Toukan <tariqt@nvidia.com> Reviewed-by: Nimrod Oren <noren@nvidia.com> --- samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)