| Message ID | 8ad0d38259a678fb42245368f974f1a5cf47d68d.1623674025.git.lorenzo@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v9,bpf-next,01/14] net: skbuff: add data_len field to skb_shared_info | expand |
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.
> 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
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).
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.
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?
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.
> > 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
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 --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()
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(-)