| Message ID | 2b94ee2e65cfd4d2d7f30896ec796f3f9af0a733.1707316651.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: cache for same cpu skb_attempt_defer_free | expand |
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.
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.
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
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.
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; }
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(-)