| Message ID | 168545704139.2996228.2516528552939485216.stgit@firesoul (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 60548b825b082cedf89b275c21c28b1e1d030e50 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] samples/bpf: xdp1 and xdp2 reduce XDPBUFSIZE to 60 | expand |
On 30/05/2023 17:30, Jesper Dangaard Brouer wrote: > Default samples/pktgen scripts send 60 byte packets as hardware > adds 4-bytes FCS checksum, which fulfils minimum Ethernet 64 bytes > frame size. > > XDP layer will not necessary have access to the 4-bytes FCS checksum. > > This leads to bpf_xdp_load_bytes() failing as it tries to copy > 64-bytes from an XDP packet that only have 60-bytes available. > > Fixes: 772251742262 ("samples/bpf: fixup some tools to be able to support xdp multibuffer") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > samples/bpf/xdp1_kern.c | 2 +- > samples/bpf/xdp2_kern.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/samples/bpf/xdp1_kern.c b/samples/bpf/xdp1_kern.c > index 0a5c704badd0..d91f27cbcfa9 100644 > --- a/samples/bpf/xdp1_kern.c > +++ b/samples/bpf/xdp1_kern.c > @@ -39,7 +39,7 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end) > return ip6h->nexthdr; > } > > -#define XDPBUFSIZE 64 > +#define XDPBUFSIZE 60 Perf with the presence of load/store copies is far from being optimal.. Still, do we care if memcpy of 60 bytes performs worse than 64 (full cacheline)? Maybe not really in this case, looking forward for the replacement of memcpy with the proper dyncptr API. Other than that: Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > SEC("xdp.frags") > int xdp_prog1(struct xdp_md *ctx) > { > diff --git a/samples/bpf/xdp2_kern.c b/samples/bpf/xdp2_kern.c > index 67804ecf7ce3..8bca674451ed 100644 > --- a/samples/bpf/xdp2_kern.c > +++ b/samples/bpf/xdp2_kern.c > @@ -55,7 +55,7 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end) > return ip6h->nexthdr; > } > > -#define XDPBUFSIZE 64 > +#define XDPBUFSIZE 60 > SEC("xdp.frags") > int xdp_prog1(struct xdp_md *ctx) > { > >
On 31/05/2023 09.23, Tariq Toukan wrote: > > > On 30/05/2023 17:30, Jesper Dangaard Brouer wrote: >> Default samples/pktgen scripts send 60 byte packets as hardware >> adds 4-bytes FCS checksum, which fulfils minimum Ethernet 64 bytes >> frame size. >> >> XDP layer will not necessary have access to the 4-bytes FCS checksum. >> >> This leads to bpf_xdp_load_bytes() failing as it tries to copy >> 64-bytes from an XDP packet that only have 60-bytes available. >> >> Fixes: 772251742262 ("samples/bpf: fixup some tools to be able to >> support xdp multibuffer") >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >> --- >> samples/bpf/xdp1_kern.c | 2 +- >> samples/bpf/xdp2_kern.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/samples/bpf/xdp1_kern.c b/samples/bpf/xdp1_kern.c >> index 0a5c704badd0..d91f27cbcfa9 100644 >> --- a/samples/bpf/xdp1_kern.c >> +++ b/samples/bpf/xdp1_kern.c >> @@ -39,7 +39,7 @@ static int parse_ipv6(void *data, u64 nh_off, void >> *data_end) >> return ip6h->nexthdr; >> } >> -#define XDPBUFSIZE 64 >> +#define XDPBUFSIZE 60 > > Perf with the presence of load/store copies is far from being optimal.. > Still, do we care if memcpy of 60 bytes performs worse than 64 (full > cacheline)? In this case that statement isn't true. I tested it and the 60 bytes define performs (slightly) better than 64 bytes one. > Maybe not really in this case, looking forward for the replacement of > memcpy with the proper dyncptr API. > This is a fix to allow sending minimum sized Ethernet frames to these samples. Looking forward, yes once dynptr is ready, we should update these samples to use that, because the use of bpf_xdp_load_bytes() have a surprisingly large overhead. But we cannot leave these samples broken in the mean while. > Other than that: > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > Thanks > >> SEC("xdp.frags") >> int xdp_prog1(struct xdp_md *ctx) >> { >> diff --git a/samples/bpf/xdp2_kern.c b/samples/bpf/xdp2_kern.c >> index 67804ecf7ce3..8bca674451ed 100644 >> --- a/samples/bpf/xdp2_kern.c >> +++ b/samples/bpf/xdp2_kern.c >> @@ -55,7 +55,7 @@ static int parse_ipv6(void *data, u64 nh_off, void >> *data_end) >> return ip6h->nexthdr; >> } >> -#define XDPBUFSIZE 64 >> +#define XDPBUFSIZE 60 >> SEC("xdp.frags") >> int xdp_prog1(struct xdp_md *ctx) >> { >> >> >
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Tue, 30 May 2023 16:30:41 +0200 you wrote: > Default samples/pktgen scripts send 60 byte packets as hardware > adds 4-bytes FCS checksum, which fulfils minimum Ethernet 64 bytes > frame size. > > XDP layer will not necessary have access to the 4-bytes FCS checksum. > > This leads to bpf_xdp_load_bytes() failing as it tries to copy > 64-bytes from an XDP packet that only have 60-bytes available. > > [...] Here is the summary with links: - [bpf-next] samples/bpf: xdp1 and xdp2 reduce XDPBUFSIZE to 60 https://git.kernel.org/bpf/bpf-next/c/60548b825b08 You are awesome, thank you!
diff --git a/samples/bpf/xdp1_kern.c b/samples/bpf/xdp1_kern.c index 0a5c704badd0..d91f27cbcfa9 100644 --- a/samples/bpf/xdp1_kern.c +++ b/samples/bpf/xdp1_kern.c @@ -39,7 +39,7 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end) return ip6h->nexthdr; } -#define XDPBUFSIZE 64 +#define XDPBUFSIZE 60 SEC("xdp.frags") int xdp_prog1(struct xdp_md *ctx) { diff --git a/samples/bpf/xdp2_kern.c b/samples/bpf/xdp2_kern.c index 67804ecf7ce3..8bca674451ed 100644 --- a/samples/bpf/xdp2_kern.c +++ b/samples/bpf/xdp2_kern.c @@ -55,7 +55,7 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end) return ip6h->nexthdr; } -#define XDPBUFSIZE 64 +#define XDPBUFSIZE 60 SEC("xdp.frags") int xdp_prog1(struct xdp_md *ctx) {
Default samples/pktgen scripts send 60 byte packets as hardware adds 4-bytes FCS checksum, which fulfils minimum Ethernet 64 bytes frame size. XDP layer will not necessary have access to the 4-bytes FCS checksum. This leads to bpf_xdp_load_bytes() failing as it tries to copy 64-bytes from an XDP packet that only have 60-bytes available. Fixes: 772251742262 ("samples/bpf: fixup some tools to be able to support xdp multibuffer") Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- samples/bpf/xdp1_kern.c | 2 +- samples/bpf/xdp2_kern.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)