[PATCH net-next] net: cache for same cpu skb_attempt_defer_free

[PATCH net-next] net: cache for same cpu skb_attempt_defer_free

[Pavel Begunkov]

Optimise skb_attempt_defer_free() executed by the CPU the skb was
allocated on. Instead of __kfree_skb() -> kmem_cache_free() we can
disable softirqs and put the buffer into cpu local caches.

Trying it with a TCP CPU bound ping pong benchmark (i.e. netbench), it
showed a 1% throughput improvement (392.2 -> 396.4 Krps). Cross checking
with profiles, the total CPU share of skb_attempt_defer_free() dropped by
0.6%. Note, I'd expect the win doubled with rx only benchmarks, as the
optimisation is for the receive path, but the test spends >55% of CPU
doing writes.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 net/core/skbuff.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index edbbef563d4d..5ac3c353c8a4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -6877,6 +6877,20 @@ void __skb_ext_put(struct skb_ext *ext)
 EXPORT_SYMBOL(__skb_ext_put);
 #endif /* CONFIG_SKB_EXTENSIONS */
 
+static void kfree_skb_napi_cache(struct sk_buff *skb)
+{
+	/* if SKB is a clone, don't handle this case */
+	if (skb->fclone != SKB_FCLONE_UNAVAILABLE || in_hardirq()) {
+		__kfree_skb(skb);
+		return;
+	}
+
+	local_bh_disable();
+	skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false);
+	napi_skb_cache_put(skb);
+	local_bh_enable();
+}
+
 /**
  * skb_attempt_defer_free - queue skb for remote freeing
  * @skb: buffer
@@ -6895,7 +6909,7 @@ void skb_attempt_defer_free(struct sk_buff *skb)
 	if (WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
 	    !cpu_online(cpu) ||
 	    cpu == raw_smp_processor_id()) {
-nodefer:	__kfree_skb(skb);
+nodefer:	kfree_skb_napi_cache(skb);
 		return;
 	}
 
-- 
2.43.0

Re: [PATCH net-next] net: cache for same cpu skb_attempt_defer_free

[Eric Dumazet]

On Wed, Feb 7, 2024 at 3:42 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> Optimise skb_attempt_defer_free() executed by the CPU the skb was
> allocated on. Instead of __kfree_skb() -> kmem_cache_free() we can
> disable softirqs and put the buffer into cpu local caches.
>
> Trying it with a TCP CPU bound ping pong benchmark (i.e. netbench), it
> showed a 1% throughput improvement (392.2 -> 396.4 Krps). Cross checking
> with profiles, the total CPU share of skb_attempt_defer_free() dropped by
> 0.6%. Note, I'd expect the win doubled with rx only benchmarks, as the
> optimisation is for the receive path, but the test spends >55% of CPU
> doing writes.
>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  net/core/skbuff.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index edbbef563d4d..5ac3c353c8a4 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6877,6 +6877,20 @@ void __skb_ext_put(struct skb_ext *ext)
>  EXPORT_SYMBOL(__skb_ext_put);
>  #endif /* CONFIG_SKB_EXTENSIONS */
>
> +static void kfree_skb_napi_cache(struct sk_buff *skb)
> +{
> +       /* if SKB is a clone, don't handle this case */
> +       if (skb->fclone != SKB_FCLONE_UNAVAILABLE || in_hardirq()) {

skb_attempt_defer_free() can not run from hard irq, please do not add
code suggesting otherwise...

> +               __kfree_skb(skb);
> +               return;
> +       }
> +
> +       local_bh_disable();
> +       skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false);
> +       napi_skb_cache_put(skb);
> +       local_bh_enable();
> +}
> +

I had a patch adding local per-cpu caches of ~8 skbs, to batch
sd->defer_lock acquisitions,
it seems I forgot to finish it.

Re: [PATCH net-next] net: cache for same cpu skb_attempt_defer_free

[Pavel Begunkov]

On 2/7/24 15:26, Eric Dumazet wrote:
> On Wed, Feb 7, 2024 at 3:42 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> Optimise skb_attempt_defer_free() executed by the CPU the skb was
>> allocated on. Instead of __kfree_skb() -> kmem_cache_free() we can
>> disable softirqs and put the buffer into cpu local caches.
>>
>> Trying it with a TCP CPU bound ping pong benchmark (i.e. netbench), it
>> showed a 1% throughput improvement (392.2 -> 396.4 Krps). Cross checking
>> with profiles, the total CPU share of skb_attempt_defer_free() dropped by
>> 0.6%. Note, I'd expect the win doubled with rx only benchmarks, as the
>> optimisation is for the receive path, but the test spends >55% of CPU
>> doing writes.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   net/core/skbuff.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index edbbef563d4d..5ac3c353c8a4 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -6877,6 +6877,20 @@ void __skb_ext_put(struct skb_ext *ext)
>>   EXPORT_SYMBOL(__skb_ext_put);
>>   #endif /* CONFIG_SKB_EXTENSIONS */
>>
>> +static void kfree_skb_napi_cache(struct sk_buff *skb)
>> +{
>> +       /* if SKB is a clone, don't handle this case */
>> +       if (skb->fclone != SKB_FCLONE_UNAVAILABLE || in_hardirq()) {
> 
> skb_attempt_defer_free() can not run from hard irq, please do not add
> code suggesting otherwise...

I'll add the change, thanks

>> +               __kfree_skb(skb);
>> +               return;
>> +       }
>> +
>> +       local_bh_disable();
>> +       skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false);
>> +       napi_skb_cache_put(skb);
>> +       local_bh_enable();
>> +}
>> +
> 
> I had a patch adding local per-cpu caches of ~8 skbs, to batch
> sd->defer_lock acquisitions,
> it seems I forgot to finish it.

I played with some naive batching approaches there before but couldn't
get anything out of it. From my observations,  skb_attempt_defer_free was
rarely getting SKBs targeting the same CPU, but there are probably irq
affinity configurations where it'd make more sense.

Just to note that this patch is targeting cases with perfect affinity, so
it's orthogonal or complimentary to defer batching.

-- 
Pavel Begunkov

Re: [PATCH net-next] net: cache for same cpu skb_attempt_defer_free

[Eric Dumazet]

On Wed, Feb 7, 2024 at 4:50 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 2/7/24 15:26, Eric Dumazet wrote:
> > On Wed, Feb 7, 2024 at 3:42 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
> >>
> >> Optimise skb_attempt_defer_free() executed by the CPU the skb was
> >> allocated on. Instead of __kfree_skb() -> kmem_cache_free() we can
> >> disable softirqs and put the buffer into cpu local caches.
> >>
> >> Trying it with a TCP CPU bound ping pong benchmark (i.e. netbench), it
> >> showed a 1% throughput improvement (392.2 -> 396.4 Krps). Cross checking
> >> with profiles, the total CPU share of skb_attempt_defer_free() dropped by
> >> 0.6%. Note, I'd expect the win doubled with rx only benchmarks, as the
> >> optimisation is for the receive path, but the test spends >55% of CPU
> >> doing writes.
> >>
> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> >> ---
> >>   net/core/skbuff.c | 16 +++++++++++++++-
> >>   1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> index edbbef563d4d..5ac3c353c8a4 100644
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -6877,6 +6877,20 @@ void __skb_ext_put(struct skb_ext *ext)
> >>   EXPORT_SYMBOL(__skb_ext_put);
> >>   #endif /* CONFIG_SKB_EXTENSIONS */
> >>
> >> +static void kfree_skb_napi_cache(struct sk_buff *skb)
> >> +{
> >> +       /* if SKB is a clone, don't handle this case */
> >> +       if (skb->fclone != SKB_FCLONE_UNAVAILABLE || in_hardirq()) {
> >
> > skb_attempt_defer_free() can not run from hard irq, please do not add
> > code suggesting otherwise...
>
> I'll add the change, thanks
>
> >> +               __kfree_skb(skb);
> >> +               return;
> >> +       }
> >> +
> >> +       local_bh_disable();
> >> +       skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false);
> >> +       napi_skb_cache_put(skb);
> >> +       local_bh_enable();
> >> +}
> >> +
> >
> > I had a patch adding local per-cpu caches of ~8 skbs, to batch
> > sd->defer_lock acquisitions,
> > it seems I forgot to finish it.
>
> I played with some naive batching approaches there before but couldn't
> get anything out of it. From my observations,  skb_attempt_defer_free was
> rarely getting SKBs targeting the same CPU, but there are probably irq
> affinity configurations where it'd make more sense.

Well, you mentioned a high cost in cpu profiles for skb_attempt_defer_free()

This is what my patch was trying to reduce. Reducing false sharing
(acquiring remote spinlocks) was the goal.


>
> Just to note that this patch is targeting cases with perfect affinity, so
> it's orthogonal or complimentary to defer batching.
>
> --
> Pavel Begunkov

Re: [PATCH net-next] net: cache for same cpu skb_attempt_defer_free

[Pavel Begunkov]

On 2/7/24 15:56, Eric Dumazet wrote:
> On Wed, Feb 7, 2024 at 4:50 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 2/7/24 15:26, Eric Dumazet wrote:
>>> On Wed, Feb 7, 2024 at 3:42 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>>>
>>>> Optimise skb_attempt_defer_free() executed by the CPU the skb was
>>>> allocated on. Instead of __kfree_skb() -> kmem_cache_free() we can
>>>> disable softirqs and put the buffer into cpu local caches.
>>>>
>>>> Trying it with a TCP CPU bound ping pong benchmark (i.e. netbench), it
>>>> showed a 1% throughput improvement (392.2 -> 396.4 Krps). Cross checking
>>>> with profiles, the total CPU share of skb_attempt_defer_free() dropped by
>>>> 0.6%. Note, I'd expect the win doubled with rx only benchmarks, as the
>>>> optimisation is for the receive path, but the test spends >55% of CPU
>>>> doing writes.
>>>>
>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>> ---
>>>>    net/core/skbuff.c | 16 +++++++++++++++-
>>>>    1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>> index edbbef563d4d..5ac3c353c8a4 100644
>>>> --- a/net/core/skbuff.c
>>>> +++ b/net/core/skbuff.c
>>>> @@ -6877,6 +6877,20 @@ void __skb_ext_put(struct skb_ext *ext)
>>>>    EXPORT_SYMBOL(__skb_ext_put);
>>>>    #endif /* CONFIG_SKB_EXTENSIONS */
>>>>
>>>> +static void kfree_skb_napi_cache(struct sk_buff *skb)
>>>> +{
>>>> +       /* if SKB is a clone, don't handle this case */
>>>> +       if (skb->fclone != SKB_FCLONE_UNAVAILABLE || in_hardirq()) {
>>>
>>> skb_attempt_defer_free() can not run from hard irq, please do not add
>>> code suggesting otherwise...
>>
>> I'll add the change, thanks
>>
>>>> +               __kfree_skb(skb);
>>>> +               return;
>>>> +       }
>>>> +
>>>> +       local_bh_disable();
>>>> +       skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false);
>>>> +       napi_skb_cache_put(skb);
>>>> +       local_bh_enable();
>>>> +}
>>>> +
>>>
>>> I had a patch adding local per-cpu caches of ~8 skbs, to batch
>>> sd->defer_lock acquisitions,
>>> it seems I forgot to finish it.
>>
>> I played with some naive batching approaches there before but couldn't
>> get anything out of it. From my observations,  skb_attempt_defer_free was
>> rarely getting SKBs targeting the same CPU, but there are probably irq
>> affinity configurations where it'd make more sense.
> 
> Well, you mentioned a high cost in cpu profiles for skb_attempt_defer_free()
> 
> This is what my patch was trying to reduce. Reducing false sharing
> (acquiring remote spinlocks) was the goal.

That would be great. My point was that if there are 2 skbs with
->alloc_cpu X and Y, it'd still need to take 2 locks, for CPUs
X and Y, and that's what was happening. But there are likely smarter
ways than the approach I tried.

>> Just to note that this patch is targeting cases with perfect affinity, so
>> it's orthogonal or complimentary to defer batching.

-- 
Pavel Begunkov