Re: [PATCH net-next] net: skbuff: Skip early return in skb_unref when debugging

[Paolo Abeni]

On 7/29/24 12:47, Breno Leitao wrote:
> This patch modifies the skb_unref function to skip the early return
> optimization when CONFIG_DEBUG_NET is enabled. The change ensures that
> the reference count decrement always occurs in debug builds, allowing
> for more thorough checking of SKB reference counting.
>
> Previously, when the SKB's reference count was 1 and CONFIG_DEBUG_NET
> was not set, the function would return early after a memory barrier
> (smp_rmb()) without decrementing the reference count. This optimization
> assumes it's safe to proceed with freeing the SKB without the overhead
> of an atomic decrement from 1 to 0.
>
> With this change:
> - In non-debug builds (CONFIG_DEBUG_NET not set), behavior remains
>    unchanged, preserving the performance optimization.
> - In debug builds (CONFIG_DEBUG_NET set), the reference count is always
>    decremented, even when it's 1, allowing for consistent behavior and
>    potentially catching subtle SKB management bugs.
>
> This modification enhances debugging capabilities for networking code
> without impacting performance in production kernels. It helps kernel
> developers identify and diagnose issues related to SKB management and
> reference counting in the network stack.
>
> Cc: Chris Mason <clm@xxxxxx>
> Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
> ---
>   include/linux/skbuff.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 29c3ea5b6e93..cf8f6ce06742 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1225,7 +1225,7 @@ static inline bool skb_unref(struct sk_buff *skb)
>   {
>   	if (unlikely(!skb))
>   		return false;
> -	if (likely(refcount_read(&skb->users) == 1))
> +	if (!IS_ENABLED(CONFIG_DEBUG_NET) && likely(refcount_read(&skb->users) == 1))
>   		smp_rmb();
>   	else if (likely(!refcount_dec_and_test(&skb->users)))
>   		return false;

I think one assumption behind CONFIG_DEBUG_NET is that enabling such 
config should not have any measurable impact on performances.

I suspect the above could indeed cause some measurable impact, e.g. 
under UDP flood, when the user-space receiver and the BH runs on 
different cores, as this will increase pressure on the CPU cache. Could 
you please benchmark such scenario before and after this patch?

Thanks!

Paolo