| Message ID | 20240809083500.2822656-2-chopps@chopps.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add 2 functions to skbuff for code sharing | expand |
On 8/9/24 10:34, Christian Hopps wrote: > From: Christian Hopps <chopps@labn.net> > > Factor out some common skb header copying code so that it can be re-used > outside of skbuff. > > Signed-off-by: Christian Hopps <chopps@labn.net> > --- > include/linux/skbuff.h | 1 + > net/core/skbuff.c | 8 +++++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 29c3ea5b6e93..8626f9a343db 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -1374,6 +1374,7 @@ struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src); > void skb_headers_offset_update(struct sk_buff *skb, int off); > int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask); > struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t priority); > +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old); > void skb_copy_header(struct sk_buff *new, const struct sk_buff *old); > struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t priority); > struct sk_buff *__pskb_copy_fclone(struct sk_buff *skb, int headroom, > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 83f8cd8aa2d1..da5a47d2c9ab 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -1515,7 +1515,7 @@ EXPORT_SYMBOL(napi_consume_skb); > BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ > offsetof(struct sk_buff, headers.field)); \ > > -static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > { > new->tstamp = old->tstamp; > /* We do not copy old->sk */ > @@ -1524,6 +1524,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > skb_dst_copy(new, old); > __skb_ext_copy(new, old); > __nf_copy(new, old, false); > +} > +EXPORT_SYMBOL_GPL(___copy_skb_header); > + > +static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > +{ > + ___copy_skb_header(new, old); > > /* Note : this field could be in the headers group. > * It is not yet because we do not want to have a 16 bit hole Could you please point where/how are you going to use this helper? factoring out this very core bits of skbuff copy looks quite bug prone - and exporting the helper could introduce additional unneeded function calls in the core code. Thanks, Paolo
On Wed, Aug 14, 2024 at 11:46:56AM +0200, Paolo Abeni wrote: > On 8/9/24 10:34, Christian Hopps wrote: > > From: Christian Hopps <chopps@labn.net> > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -1515,7 +1515,7 @@ EXPORT_SYMBOL(napi_consume_skb); > > BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ > > offsetof(struct sk_buff, headers.field)); \ > > -static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > > +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > > { > > new->tstamp = old->tstamp; > > /* We do not copy old->sk */ > > @@ -1524,6 +1524,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > > skb_dst_copy(new, old); > > __skb_ext_copy(new, old); > > __nf_copy(new, old, false); > > +} > > +EXPORT_SYMBOL_GPL(___copy_skb_header); > > + > > +static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > > +{ > > + ___copy_skb_header(new, old); > > > /* Note : this field could be in the headers group. > > * It is not yet because we do not want to have a 16 bit hole > > Could you please point where/how are you going to use this helper? factoring > out this very core bits of skbuff copy looks quite bug prone - and exporting > the helper could introduce additional unneeded function calls in the core > code. It is supposed to be used in the IPTFS pachset: https://lore.kernel.org/netdev/20240807211331.1081038-12-chopps@chopps.org/ It was open coded before, but there were some concerns that IPTFS won't get updated if __copy_skb_header changes.
On 8/14/24 12:38, Steffen Klassert wrote: > On Wed, Aug 14, 2024 at 11:46:56AM +0200, Paolo Abeni wrote: >> On 8/9/24 10:34, Christian Hopps wrote: >>> From: Christian Hopps <chopps@labn.net> >>> --- a/net/core/skbuff.c >>> +++ b/net/core/skbuff.c >>> @@ -1515,7 +1515,7 @@ EXPORT_SYMBOL(napi_consume_skb); >>> BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ >>> offsetof(struct sk_buff, headers.field)); \ >>> -static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>> +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>> { >>> new->tstamp = old->tstamp; >>> /* We do not copy old->sk */ >>> @@ -1524,6 +1524,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>> skb_dst_copy(new, old); >>> __skb_ext_copy(new, old); >>> __nf_copy(new, old, false); >>> +} >>> +EXPORT_SYMBOL_GPL(___copy_skb_header); >>> + >>> +static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>> +{ >>> + ___copy_skb_header(new, old); > >>> /* Note : this field could be in the headers group. >>> * It is not yet because we do not want to have a 16 bit hole >> >> Could you please point where/how are you going to use this helper? factoring >> out this very core bits of skbuff copy looks quite bug prone - and exporting >> the helper could introduce additional unneeded function calls in the core >> code. > > It is supposed to be used in the IPTFS pachset: > > https://lore.kernel.org/netdev/20240807211331.1081038-12-chopps@chopps.org/ > > It was open coded before, but there were some concerns that > IPTFS won't get updated if __copy_skb_header changes. The code is copying a subset of the skb header from a 'template' skb to a newly allocated skbuff. It's unclear to me why would be useful to copy only a subset of the skb header, excluding queue_mapping, priority, etc.. I think we need a good justification for that, otherwise we could end-up with a large amount of "almost copy" skb header slicing the skb in many different ways. Cheers, Paolo
Paolo Abeni <pabeni@redhat.com> writes: > On 8/14/24 12:38, Steffen Klassert wrote: >> On Wed, Aug 14, 2024 at 11:46:56AM +0200, Paolo Abeni wrote: >>> On 8/9/24 10:34, Christian Hopps wrote: >>>> From: Christian Hopps <chopps@labn.net> >>>> --- a/net/core/skbuff.c >>>> +++ b/net/core/skbuff.c >>>> @@ -1515,7 +1515,7 @@ EXPORT_SYMBOL(napi_consume_skb); >>>> BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ >>>> offsetof(struct sk_buff, headers.field)); \ >>>> -static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>> +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>> { >>>> new->tstamp = old->tstamp; >>>> /* We do not copy old->sk */ >>>> @@ -1524,6 +1524,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>> skb_dst_copy(new, old); >>>> __skb_ext_copy(new, old); >>>> __nf_copy(new, old, false); >>>> +} >>>> +EXPORT_SYMBOL_GPL(___copy_skb_header); >>>> + >>>> +static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>> +{ >>>> + ___copy_skb_header(new, old); > >>>> /* Note : this field could be in the headers group. >>>> * It is not yet because we do not want to have a 16 bit hole >>> >>> Could you please point where/how are you going to use this helper? factoring >>> out this very core bits of skbuff copy looks quite bug prone - and exporting >>> the helper could introduce additional unneeded function calls in the core >>> code. >> It is supposed to be used in the IPTFS pachset: >> https://lore.kernel.org/netdev/20240807211331.1081038-12-chopps@chopps.org/ >> It was open coded before, but there were some concerns that >> IPTFS won't get updated if __copy_skb_header changes. > > The code is copying a subset of the skb header from a 'template' skb to a newly > allocated skbuff. > It's unclear to me why would be useful to copy only a subset of the skb header, > excluding queue_mapping, priority, etc.. > I think we need a good justification for that, otherwise we could end-up with a > large amount of "almost copy" skb header slicing the skb in many different ways. IP-TFS sometimes needs to allocate new skb[s] to fragment a too-large tunnel ingress user packet. IP-TFS may also need to extract multiple aggregated user packets for tunnel egress from inside a single IPTFS tunnel packet. For these 1 to N cases (which are different from regular IPsec which is always 1-1 and thus re-using the existing skb) we need to create multiple skbs from a single source skb and we need to replicate the work done to the existing skb so far in the netdev/xfrm infrastructure (e.g. the _refdst and _nfct are expected to be there and refcounted as they are dropped later in the stack). This work is captured in those first few values that we are copying. The `headers` and other field values; however, are not appropriate to be copied (or clobbered e.g., alloc_cpu) into the new allocated skb. I originally had this code local to the IP-TFS implementation, but I was persuaded to move it in skbuff.c to track any possible changes to these fields in the future. Thanks, Chris. > Cheers, > > Paolo
Christian Hopps <chopps@chopps.org> writes: > Paolo Abeni <pabeni@redhat.com> writes: > >> On 8/14/24 12:38, Steffen Klassert wrote: >>> On Wed, Aug 14, 2024 at 11:46:56AM +0200, Paolo Abeni wrote: >>>> On 8/9/24 10:34, Christian Hopps wrote: >>>>> From: Christian Hopps <chopps@labn.net> >>>>> --- a/net/core/skbuff.c >>>>> +++ b/net/core/skbuff.c >>>>> @@ -1515,7 +1515,7 @@ EXPORT_SYMBOL(napi_consume_skb); >>>>> BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ >>>>> offsetof(struct sk_buff, headers.field)); \ >>>>> -static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>>> +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>>> { >>>>> new->tstamp = old->tstamp; >>>>> /* We do not copy old->sk */ >>>>> @@ -1524,6 +1524,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>>> skb_dst_copy(new, old); >>>>> __skb_ext_copy(new, old); >>>>> __nf_copy(new, old, false); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(___copy_skb_header); >>>>> + >>>>> +static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) >>>>> +{ >>>>> + ___copy_skb_header(new, old); > >>>>> /* Note : this field could be in the headers group. >>>>> * It is not yet because we do not want to have a 16 bit hole >>>> >>>> Could you please point where/how are you going to use this helper? factoring >>>> out this very core bits of skbuff copy looks quite bug prone - and exporting >>>> the helper could introduce additional unneeded function calls in the core >>>> code. >>> It is supposed to be used in the IPTFS pachset: >>> https://lore.kernel.org/netdev/20240807211331.1081038-12-chopps@chopps.org/ >>> It was open coded before, but there were some concerns that >>> IPTFS won't get updated if __copy_skb_header changes. >> >> The code is copying a subset of the skb header from a 'template' skb to a newly >> allocated skbuff. >> It's unclear to me why would be useful to copy only a subset of the skb header, >> excluding queue_mapping, priority, etc.. >> I think we need a good justification for that, otherwise we could end-up with a >> large amount of "almost copy" skb header slicing the skb in many different ways. > > IP-TFS sometimes needs to allocate new skb[s] to fragment a too-large tunnel > ingress user packet. IP-TFS may also need to extract multiple aggregated user > packets for tunnel egress from inside a single IPTFS tunnel packet. For these 1 > to N cases (which are different from regular IPsec which is always 1-1 and thus > re-using the existing skb) we need to create multiple skbs from a single source > skb and we need to replicate the work done to the existing skb so far in the > netdev/xfrm infrastructure (e.g. the _refdst and _nfct are expected to be there > and refcounted as they are dropped later in the stack). This work is captured in > those first few values that we are copying. The `headers` and other field > values; however, are not appropriate to be copied (or clobbered e.g., alloc_cpu) > into the new allocated skb. > > I originally had this code local to the IP-TFS implementation, but I was persuaded to move it in skbuff.c to track any possible changes to these fields in the future. I've gone back through the skb use again for both tunnel ingress and egress and I need even less than the subset of fields being copied here. I'm going to resend this patchset removing this somewhat controversial refactoring for now, and just leave the second commit with the useful copy from read_seq function. Thanks, Chris. > > Thanks, > Chris. > >> Cheers, >> >> Paolo a
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 29c3ea5b6e93..8626f9a343db 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1374,6 +1374,7 @@ struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src); void skb_headers_offset_update(struct sk_buff *skb, int off); int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask); struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t priority); +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old); void skb_copy_header(struct sk_buff *new, const struct sk_buff *old); struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t priority); struct sk_buff *__pskb_copy_fclone(struct sk_buff *skb, int headroom, diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 83f8cd8aa2d1..da5a47d2c9ab 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -1515,7 +1515,7 @@ EXPORT_SYMBOL(napi_consume_skb); BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ offsetof(struct sk_buff, headers.field)); \ -static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old) { new->tstamp = old->tstamp; /* We do not copy old->sk */ @@ -1524,6 +1524,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) skb_dst_copy(new, old); __skb_ext_copy(new, old); __nf_copy(new, old, false); +} +EXPORT_SYMBOL_GPL(___copy_skb_header); + +static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) +{ + ___copy_skb_header(new, old); /* Note : this field could be in the headers group. * It is not yet because we do not want to have a 16 bit hole