diff mbox series

[v9,bpf-next,01/14] net: skbuff: add data_len field to skb_shared_info

Message ID 8ad0d38259a678fb42245368f974f1a5cf47d68d.1623674025.git.lorenzo@kernel.org (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series mvneta: introduce XDP multi-buffer support | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 14 maintainers not CCed: jonathan.lemon@gmail.com yhs@fb.com kpsingh@kernel.org hawk@kernel.org andrii@kernel.org wenxu@ucloud.cn kafai@fb.com cong.wang@bytedance.com willemb@google.com elver@google.com songliubraving@fb.com alobakin@pm.me nogikh@google.com haokexin@gmail.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 6390 this patch: 6390
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 6455 this patch: 6455
netdev/header_inline success Link

Commit Message

Lorenzo Bianconi June 14, 2021, 12:49 p.m. UTC
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(-)

Comments

Alexander Duyck June 28, 2021, 7:58 p.m. UTC | #1
On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> 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(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index dbf820a50a39..332ec56c200d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -522,7 +522,10 @@ struct skb_shared_info {
>         struct sk_buff  *frag_list;
>         struct skb_shared_hwtstamps hwtstamps;
>         unsigned int    gso_type;
> -       u32             tskey;
> +       union {
> +               u32     tskey;
> +               u32     data_len;
> +       };
>

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.
Lorenzo Bianconi June 29, 2021, 12:44 p.m. UTC | #2
> On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > 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(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index dbf820a50a39..332ec56c200d 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -522,7 +522,10 @@ struct skb_shared_info {
> >         struct sk_buff  *frag_list;
> >         struct skb_shared_hwtstamps hwtstamps;
> >         unsigned int    gso_type;
> > -       u32             tskey;
> > +       union {
> > +               u32     tskey;
> > +               u32     data_len;
> > +       };
> >
> 
> 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
Jakub Kicinski June 29, 2021, 5:08 p.m. UTC | #3
On Tue, 29 Jun 2021 14:44:56 +0200 Lorenzo Bianconi wrote:
> > On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:  
> > >
> > > 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(-)
> > >
> > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > index dbf820a50a39..332ec56c200d 100644
> > > --- a/include/linux/skbuff.h
> > > +++ b/include/linux/skbuff.h
> > > @@ -522,7 +522,10 @@ struct skb_shared_info {
> > >         struct sk_buff  *frag_list;
> > >         struct skb_shared_hwtstamps hwtstamps;
> > >         unsigned int    gso_type;
> > > -       u32             tskey;
> > > +       union {
> > > +               u32     tskey;
> > > +               u32     data_len;
> > > +       };
> > >  
> > 
> > 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).
Alexander Duyck June 29, 2021, 6:18 p.m. UTC | #4
On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 29 Jun 2021 14:44:56 +0200 Lorenzo Bianconi wrote:
> > > On Mon, Jun 14, 2021 at 5:50 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > >
> > > > 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(-)
> > > >
> > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > > index dbf820a50a39..332ec56c200d 100644
> > > > --- a/include/linux/skbuff.h
> > > > +++ b/include/linux/skbuff.h
> > > > @@ -522,7 +522,10 @@ struct skb_shared_info {
> > > >         struct sk_buff  *frag_list;
> > > >         struct skb_shared_hwtstamps hwtstamps;
> > > >         unsigned int    gso_type;
> > > > -       u32             tskey;
> > > > +       union {
> > > > +               u32     tskey;
> > > > +               u32     data_len;
> > > > +       };
> > > >
> > >
> > > 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.
Jakub Kicinski June 29, 2021, 6:37 p.m. UTC | #5
On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
> On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > 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?
Jesper Dangaard Brouer June 29, 2021, 7:11 p.m. UTC | #6
On 29/06/2021 20.37, Jakub Kicinski wrote:
> On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
>> On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>>> 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.
Lorenzo Bianconi June 29, 2021, 7:18 p.m. UTC | #7
> 
> On 29/06/2021 20.37, Jakub Kicinski wrote:
> > On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
> > > On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > > 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
Alexander Duyck June 29, 2021, 8:45 p.m. UTC | #8
On Tue, Jun 29, 2021 at 12:18 PM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> >
> > On 29/06/2021 20.37, Jakub Kicinski wrote:
> > > On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:
> > > > On Tue, Jun 29, 2021 at 10:08 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > > > 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.
diff mbox series

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index dbf820a50a39..332ec56c200d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -522,7 +522,10 @@  struct skb_shared_info {
 	struct sk_buff	*frag_list;
 	struct skb_shared_hwtstamps hwtstamps;
 	unsigned int	gso_type;
-	u32		tskey;
+	union {
+		u32	tskey;
+		u32	data_len;
+	};
 
 	/*
 	 * Warning : all fields before dataref are cleared in __alloc_skb()