| Message ID | 0bc041d2d38a08064a642c05ca8cceb0ca165f88.1641863490.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | udp optimisation | expand |
From: Pavel Begunkov > Sent: 11 January 2022 01:22 > > Inline a HW csum'ed part of skb_csum_hwoffload_help(). > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > include/linux/netdevice.h | 16 ++++++++++++++-- > net/core/dev.c | 13 +++---------- > 2 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 3213c7227b59..fbe6c764ce57 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -4596,8 +4596,20 @@ void netdev_rss_key_fill(void *buffer, size_t len); > > int skb_checksum_help(struct sk_buff *skb); > int skb_crc32c_csum_help(struct sk_buff *skb); > -int skb_csum_hwoffload_help(struct sk_buff *skb, > - const netdev_features_t features); > +int __skb_csum_hwoffload_help(struct sk_buff *skb, > + const netdev_features_t features); > + > +static inline int skb_csum_hwoffload_help(struct sk_buff *skb, > + const netdev_features_t features) > +{ > + if (unlikely(skb_csum_is_sctp(skb))) > + return !!(features & NETIF_F_SCTP_CRC) ? 0 : If that !! doing anything? - doesn't look like it. > + skb_crc32c_csum_help(skb); > + > + if (features & NETIF_F_HW_CSUM) > + return 0; > + return __skb_csum_hwoffload_help(skb, features); > +} Maybe you should remove some bloat by moving the sctp code into the called function. This probably needs something like? { if (features & NETIF_F_HW_CSUM && !skb_csum_is_sctp(skb)) return 0; return __skb_csum_hw_offload(skb, features); } David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 1/11/22 09:24, David Laight wrote: > From: Pavel Begunkov >> Sent: 11 January 2022 01:22 >> >> Inline a HW csum'ed part of skb_csum_hwoffload_help(). >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> include/linux/netdevice.h | 16 ++++++++++++++-- >> net/core/dev.c | 13 +++---------- >> 2 files changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 3213c7227b59..fbe6c764ce57 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -4596,8 +4596,20 @@ void netdev_rss_key_fill(void *buffer, size_t len); >> >> int skb_checksum_help(struct sk_buff *skb); >> int skb_crc32c_csum_help(struct sk_buff *skb); >> -int skb_csum_hwoffload_help(struct sk_buff *skb, >> - const netdev_features_t features); >> +int __skb_csum_hwoffload_help(struct sk_buff *skb, >> + const netdev_features_t features); >> + >> +static inline int skb_csum_hwoffload_help(struct sk_buff *skb, >> + const netdev_features_t features) >> +{ >> + if (unlikely(skb_csum_is_sctp(skb))) >> + return !!(features & NETIF_F_SCTP_CRC) ? 0 : > > If that !! doing anything? - doesn't look like it. It doesn't, but left the original style >> + skb_crc32c_csum_help(skb); >> + >> + if (features & NETIF_F_HW_CSUM) >> + return 0; >> + return __skb_csum_hwoffload_help(skb, features); >> +} > > Maybe you should remove some bloat by moving the sctp code > into the called function. > This probably needs something like? > > { > if (features & NETIF_F_HW_CSUM && !skb_csum_is_sctp(skb)) > return 0; > return __skb_csum_hw_offload(skb, features); > } I don't like inlining that sctp chunk myself. It seems your way would need another skb_csum_is_sctp() in __skb_csum_hw_offload(), if so I don't think it's worth it. Would've been great to put the NETIF_F_HW_CSUM check first and hide sctp, but don't think it's correct. Would be great to hear some ideas.
From: Pavel Begunkov > Sent: 11 January 2022 16:59 > > On 1/11/22 09:24, David Laight wrote: > > From: Pavel Begunkov > >> Sent: 11 January 2022 01:22 > >> > >> Inline a HW csum'ed part of skb_csum_hwoffload_help(). > >> > >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > >> --- > >> include/linux/netdevice.h | 16 ++++++++++++++-- > >> net/core/dev.c | 13 +++---------- > >> 2 files changed, 17 insertions(+), 12 deletions(-) > >> > >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > >> index 3213c7227b59..fbe6c764ce57 100644 > >> --- a/include/linux/netdevice.h > >> +++ b/include/linux/netdevice.h > >> @@ -4596,8 +4596,20 @@ void netdev_rss_key_fill(void *buffer, size_t len); > >> > >> int skb_checksum_help(struct sk_buff *skb); > >> int skb_crc32c_csum_help(struct sk_buff *skb); > >> -int skb_csum_hwoffload_help(struct sk_buff *skb, > >> - const netdev_features_t features); > >> +int __skb_csum_hwoffload_help(struct sk_buff *skb, > >> + const netdev_features_t features); > >> + > >> +static inline int skb_csum_hwoffload_help(struct sk_buff *skb, > >> + const netdev_features_t features) > >> +{ > >> + if (unlikely(skb_csum_is_sctp(skb))) > >> + return !!(features & NETIF_F_SCTP_CRC) ? 0 : > > > > If that !! doing anything? - doesn't look like it. > > It doesn't, but left the original style It just makes you think it is needed... > >> + skb_crc32c_csum_help(skb); > >> + > >> + if (features & NETIF_F_HW_CSUM) > >> + return 0; > >> + return __skb_csum_hwoffload_help(skb, features); > >> +} > > > > Maybe you should remove some bloat by moving the sctp code > > into the called function. > > This probably needs something like? > > > > { > > if (features & NETIF_F_HW_CSUM && !skb_csum_is_sctp(skb)) > > return 0; > > return __skb_csum_hw_offload(skb, features); > > } > > I don't like inlining that sctp chunk myself. It seems your way would > need another skb_csum_is_sctp() in __skb_csum_hw_offload(), if so I > don't think it's worth it. Would've been great to put the > NETIF_F_HW_CSUM check first and hide sctp, but don't think it's > correct. Would be great to hear some ideas. Given the definition: static inline bool skb_csum_is_sctp(struct sk_buff *skb) { return skb->csum_not_inet; } I wouldn't worry about doing it twice. Also skb_crc32_csum_help() is only called one. Make it static (so inlined) and pass 'features' into it. In reality sctp is such a slow crappy protocol that a few extra function calls will make diddly-squit difference. (And yes, we do actually use the sctp stack.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 1/11/22 17:25, David Laight wrote: > From: Pavel Begunkov >> Sent: 11 January 2022 16:59 >> >> On 1/11/22 09:24, David Laight wrote: >>> From: Pavel Begunkov >>>> Sent: 11 January 2022 01:22 >>>> >>>> Inline a HW csum'ed part of skb_csum_hwoffload_help(). >>>> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> include/linux/netdevice.h | 16 ++++++++++++++-- >>>> net/core/dev.c | 13 +++---------- >>>> 2 files changed, 17 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>> index 3213c7227b59..fbe6c764ce57 100644 >>>> --- a/include/linux/netdevice.h >>>> +++ b/include/linux/netdevice.h >>>> @@ -4596,8 +4596,20 @@ void netdev_rss_key_fill(void *buffer, size_t len); >>>> >>>> int skb_checksum_help(struct sk_buff *skb); >>>> int skb_crc32c_csum_help(struct sk_buff *skb); >>>> -int skb_csum_hwoffload_help(struct sk_buff *skb, >>>> - const netdev_features_t features); >>>> +int __skb_csum_hwoffload_help(struct sk_buff *skb, >>>> + const netdev_features_t features); >>>> + >>>> +static inline int skb_csum_hwoffload_help(struct sk_buff *skb, >>>> + const netdev_features_t features) >>>> +{ >>>> + if (unlikely(skb_csum_is_sctp(skb))) >>>> + return !!(features & NETIF_F_SCTP_CRC) ? 0 : >>> >>> If that !! doing anything? - doesn't look like it. >> >> It doesn't, but left the original style > > It just makes you think it is needed... > >>>> + skb_crc32c_csum_help(skb); >>>> + >>>> + if (features & NETIF_F_HW_CSUM) >>>> + return 0; >>>> + return __skb_csum_hwoffload_help(skb, features); >>>> +} >>> >>> Maybe you should remove some bloat by moving the sctp code >>> into the called function. >>> This probably needs something like? >>> >>> { >>> if (features & NETIF_F_HW_CSUM && !skb_csum_is_sctp(skb)) >>> return 0; >>> return __skb_csum_hw_offload(skb, features); >>> } >> >> I don't like inlining that sctp chunk myself. It seems your way would >> need another skb_csum_is_sctp() in __skb_csum_hw_offload(), if so I >> don't think it's worth it. Would've been great to put the >> NETIF_F_HW_CSUM check first and hide sctp, but don't think it's >> correct. Would be great to hear some ideas. > > Given the definition: > > static inline bool skb_csum_is_sctp(struct sk_buff *skb) > { > return skb->csum_not_inet; > } > > I wouldn't worry about doing it twice. > > Also skb_crc32_csum_help() is only called one. > Make it static (so inlined) and pass 'features' into it. > > In reality sctp is such a slow crappy protocol that a few extra > function calls will make diddly-squit difference. > (And yes, we do actually use the sctp stack.) I was more thinking about non-sctp path without NETIF_F_HW_CSUM
From: Pavel Begunkov > Sent: 11 January 2022 20:48 > On 1/11/22 17:25, David Laight wrote: > > From: Pavel Begunkov > >> Sent: 11 January 2022 16:59 > >> > >> On 1/11/22 09:24, David Laight wrote: > >>> From: Pavel Begunkov > >>>> Sent: 11 January 2022 01:22 > >>>> > >>>> Inline a HW csum'ed part of skb_csum_hwoffload_help(). > >>>> > >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > >>>> --- > >>>> include/linux/netdevice.h | 16 ++++++++++++++-- > >>>> net/core/dev.c | 13 +++---------- > >>>> 2 files changed, 17 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > >>>> index 3213c7227b59..fbe6c764ce57 100644 > >>>> --- a/include/linux/netdevice.h > >>>> +++ b/include/linux/netdevice.h > >>>> @@ -4596,8 +4596,20 @@ void netdev_rss_key_fill(void *buffer, size_t len); > >>>> > >>>> int skb_checksum_help(struct sk_buff *skb); > >>>> int skb_crc32c_csum_help(struct sk_buff *skb); > >>>> -int skb_csum_hwoffload_help(struct sk_buff *skb, > >>>> - const netdev_features_t features); > >>>> +int __skb_csum_hwoffload_help(struct sk_buff *skb, > >>>> + const netdev_features_t features); > >>>> + > >>>> +static inline int skb_csum_hwoffload_help(struct sk_buff *skb, > >>>> + const netdev_features_t features) > >>>> +{ > >>>> + if (unlikely(skb_csum_is_sctp(skb))) > >>>> + return !!(features & NETIF_F_SCTP_CRC) ? 0 : > >>> > >>> If that !! doing anything? - doesn't look like it. > >> > >> It doesn't, but left the original style > > > > It just makes you think it is needed... > > > >>>> + skb_crc32c_csum_help(skb); > >>>> + > >>>> + if (features & NETIF_F_HW_CSUM) > >>>> + return 0; > >>>> + return __skb_csum_hwoffload_help(skb, features); > >>>> +} > >>> > >>> Maybe you should remove some bloat by moving the sctp code > >>> into the called function. > >>> This probably needs something like? > >>> > >>> { > >>> if (features & NETIF_F_HW_CSUM && !skb_csum_is_sctp(skb)) > >>> return 0; > >>> return __skb_csum_hw_offload(skb, features); > >>> } > >> > >> I don't like inlining that sctp chunk myself. It seems your way would > >> need another skb_csum_is_sctp() in __skb_csum_hw_offload(), if so I > >> don't think it's worth it. Would've been great to put the > >> NETIF_F_HW_CSUM check first and hide sctp, but don't think it's > >> correct. Would be great to hear some ideas. > > > > Given the definition: > > > > static inline bool skb_csum_is_sctp(struct sk_buff *skb) > > { > > return skb->csum_not_inet; > > } > > > > I wouldn't worry about doing it twice. > > > > Also skb_crc32_csum_help() is only called one. > > Make it static (so inlined) and pass 'features' into it. > > > > In reality sctp is such a slow crappy protocol that a few extra > > function calls will make diddly-squit difference. > > (And yes, we do actually use the sctp stack.) > > I was more thinking about non-sctp path without NETIF_F_HW_CSUM In which case you need the body of __skb_csum_hw_offload() and end up doing the 'sctp' check once inside it. The 'sctp' check is only done twice for sctp. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 1/12/22 02:41, David Laight wrote: > From: Pavel Begunkov >> Sent: 11 January 2022 20:48 >> On 1/11/22 17:25, David Laight wrote: >>> From: Pavel Begunkov >>>> Sent: 11 January 2022 16:59 >>>> >>>> On 1/11/22 09:24, David Laight wrote: >>>>> From: Pavel Begunkov >>>>>> Sent: 11 January 2022 01:22 >>>>>> >>>>>> Inline a HW csum'ed part of skb_csum_hwoffload_help(). >>>>>> >>>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>>>> --- >>>>>> include/linux/netdevice.h | 16 ++++++++++++++-- >>>>>> net/core/dev.c | 13 +++---------- >>>>>> 2 files changed, 17 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>>>> index 3213c7227b59..fbe6c764ce57 100644 >>>>>> --- a/include/linux/netdevice.h >>>>>> +++ b/include/linux/netdevice.h >>>>>> @@ -4596,8 +4596,20 @@ void netdev_rss_key_fill(void *buffer, size_t len); >>>>>> >>>>>> int skb_checksum_help(struct sk_buff *skb); >>>>>> int skb_crc32c_csum_help(struct sk_buff *skb); >>>>>> -int skb_csum_hwoffload_help(struct sk_buff *skb, >>>>>> - const netdev_features_t features); >>>>>> +int __skb_csum_hwoffload_help(struct sk_buff *skb, >>>>>> + const netdev_features_t features); >>>>>> + >>>>>> +static inline int skb_csum_hwoffload_help(struct sk_buff *skb, >>>>>> + const netdev_features_t features) >>>>>> +{ >>>>>> + if (unlikely(skb_csum_is_sctp(skb))) >>>>>> + return !!(features & NETIF_F_SCTP_CRC) ? 0 : >>>>> >>>>> If that !! doing anything? - doesn't look like it. >>>> >>>> It doesn't, but left the original style >>> >>> It just makes you think it is needed... >>> >>>>>> + skb_crc32c_csum_help(skb); >>>>>> + >>>>>> + if (features & NETIF_F_HW_CSUM) >>>>>> + return 0; >>>>>> + return __skb_csum_hwoffload_help(skb, features); >>>>>> +} >>>>> >>>>> Maybe you should remove some bloat by moving the sctp code >>>>> into the called function. >>>>> This probably needs something like? >>>>> >>>>> { >>>>> if (features & NETIF_F_HW_CSUM && !skb_csum_is_sctp(skb)) >>>>> return 0; >>>>> return __skb_csum_hw_offload(skb, features); >>>>> } >>>> >>>> I don't like inlining that sctp chunk myself. It seems your way would >>>> need another skb_csum_is_sctp() in __skb_csum_hw_offload(), if so I >>>> don't think it's worth it. Would've been great to put the >>>> NETIF_F_HW_CSUM check first and hide sctp, but don't think it's >>>> correct. Would be great to hear some ideas. >>> >>> Given the definition: >>> >>> static inline bool skb_csum_is_sctp(struct sk_buff *skb) >>> { >>> return skb->csum_not_inet; >>> } >>> >>> I wouldn't worry about doing it twice. >>> >>> Also skb_crc32_csum_help() is only called one. >>> Make it static (so inlined) and pass 'features' into it. >>> >>> In reality sctp is such a slow crappy protocol that a few extra >>> function calls will make diddly-squit difference. >>> (And yes, we do actually use the sctp stack.) >> >> I was more thinking about non-sctp path without NETIF_F_HW_CSUM > > In which case you need the body of __skb_csum_hw_offload() > and end up doing the 'sctp' check once inside it. > The 'sctp' check is only done twice for sctp. Ah yes, might be better indeed
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 3213c7227b59..fbe6c764ce57 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -4596,8 +4596,20 @@ void netdev_rss_key_fill(void *buffer, size_t len); int skb_checksum_help(struct sk_buff *skb); int skb_crc32c_csum_help(struct sk_buff *skb); -int skb_csum_hwoffload_help(struct sk_buff *skb, - const netdev_features_t features); +int __skb_csum_hwoffload_help(struct sk_buff *skb, + const netdev_features_t features); + +static inline int skb_csum_hwoffload_help(struct sk_buff *skb, + const netdev_features_t features) +{ + if (unlikely(skb_csum_is_sctp(skb))) + return !!(features & NETIF_F_SCTP_CRC) ? 0 : + skb_crc32c_csum_help(skb); + + if (features & NETIF_F_HW_CSUM) + return 0; + return __skb_csum_hwoffload_help(skb, features); +} struct sk_buff *__skb_gso_segment(struct sk_buff *skb, netdev_features_t features, bool tx_path); diff --git a/net/core/dev.c b/net/core/dev.c index 877ebc0f72bd..e65a3b311810 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3513,16 +3513,9 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb, return skb; } -int skb_csum_hwoffload_help(struct sk_buff *skb, - const netdev_features_t features) +int __skb_csum_hwoffload_help(struct sk_buff *skb, + const netdev_features_t features) { - if (unlikely(skb_csum_is_sctp(skb))) - return !!(features & NETIF_F_SCTP_CRC) ? 0 : - skb_crc32c_csum_help(skb); - - if (features & NETIF_F_HW_CSUM) - return 0; - if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { switch (skb->csum_offset) { case offsetof(struct tcphdr, check): @@ -3533,7 +3526,7 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, return skb_checksum_help(skb); } -EXPORT_SYMBOL(skb_csum_hwoffload_help); +EXPORT_SYMBOL(__skb_csum_hwoffload_help); static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev, bool *again) {
Inline a HW csum'ed part of skb_csum_hwoffload_help(). Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- include/linux/netdevice.h | 16 ++++++++++++++-- net/core/dev.c | 13 +++---------- 2 files changed, 17 insertions(+), 12 deletions(-)