Re: [PATCH net-next 2/2] net: kfree_skb_list use kmem_cache_free_bulk

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 9 Jan 2023 13:24:54 +0100 Jesper Dangaard Brouer wrote:
> > Also the lack of perf numbers is a bit of a red flag.
> >  
> 
> I have run performance tests, but as I tried to explain in the
> cover letter, for the qdisc use-case this code path is only activated
> when we have overflow at enqueue.  Thus, this doesn't translate directly
> into a performance numbers, as TX-qdisc is 100% full caused by hardware
> device being backed up, and this patch makes us use less time on freeing
> memory.

I guess it's quite subjective, so it'd be good to get a third opinion.
To me that reads like a premature optimization. Saeed asked for perf
numbers, too.

Does anyone on the list want to cast the tie-break vote?

> I have been using pktgen script ./pktgen_bench_xmit_mode_queue_xmit.sh
> which can inject packets at the qdisc layer (invoking __dev_queue_xmit).
> And then used perf-record to see overhead of SLUB (__slab_free is top#4)
> is reduced.

Right, pktgen wasting time while still delivering line rate is not of
practical importance.

> > kfree_skb_list_bulk() ?  
> 
> Hmm, IMHO not really worth changing the function name.  The
> kfree_skb_list() is called in more places, (than qdisc enqueue-overflow
> case), which automatically benefits if we keep the function name
> kfree_skb_list().

To be clear - I was suggesting a simple
  s/kfree_skb_defer_local/kfree_skb_list_bulk/
on the patch, just renaming the static helper.

IMO now that we have multiple freeing optimizations using "defer" for
the TCP scheme and "bulk" for your prior slab bulk optimizations would
improve clarity.