Re: [PATCH 13/14] net: inline part of skb_csum_hwoffload_help

From: Pavel Begunkov <asml.silence@gmail.com>
To: David Laight <David.Laight@ACULAB.COM>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	David Ahern <dsahern@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 13/14] net: inline part of skb_csum_hwoffload_help
Date: Tue, 11 Jan 2022 20:48:12 +0000	[thread overview]
Message-ID: <d9714712-075f-17af-b4f0-67f82178abae@gmail.com> (raw)
In-Reply-To: <f2e3693ec8d1498aa376f72ebd49e058@AcuMS.aculab.com>

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


-- 
Pavel Begunkov