| Message ID | 20190324165644.21303-1-nbd@nbd.name |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] net: use bulk free in kfree_skb_list | expand |
On Sun, 24 Mar 2019 17:56:44 +0100 Felix Fietkau <nbd@nbd.name> wrote: > Since we're freeing multiple skbs, we might as well use bulk free to save a > few cycles. Use the same conditions for bulk free as in napi_consume_skb. > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > v2: call kmem_cache_free_bulk once the skb array is full instead of > falling back to kfree_skb Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
On 03/24/2019 09:56 AM, Felix Fietkau wrote: > Since we're freeing multiple skbs, we might as well use bulk free to save a > few cycles. Use the same conditions for bulk free as in napi_consume_skb. > I do not believe kfree_skb_list() is used in the fast path, so do we really need to make it so complex ?
Hi, On Sun, 2019-03-24 at 17:56 +0100, Felix Fietkau wrote: > Since we're freeing multiple skbs, we might as well use bulk free to save a > few cycles. Use the same conditions for bulk free as in napi_consume_skb. > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > v2: call kmem_cache_free_bulk once the skb array is full instead of > falling back to kfree_skb > net/core/skbuff.c | 40 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 36 insertions(+), 4 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 2415d9cb9b89..1eeaa264d2a4 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -666,12 +666,44 @@ EXPORT_SYMBOL(kfree_skb); > > void kfree_skb_list(struct sk_buff *segs) > { > - while (segs) { > - struct sk_buff *next = segs->next; > + struct sk_buff *next = segs; > + void *skbs[16]; > + int n_skbs = 0; > > - kfree_skb(segs); > - segs = next; > + while ((segs = next) != NULL) { > + next = segs->next; > + > + if (!skb_unref(segs)) > + continue; > + > + if (fclone != SKB_FCLONE_UNAVAILABLE) { > + kfree_skb(segs); > + continue; > + } I think you should swap the order of skb_unref() and the above check, or skbs with 'fclone != SKB_FCLONE_UNAVAILABLE' will go twice in skb_unref() (kfree_skb() calls skb_unref(), too). Other than that LGTM, Thanks, Paolo
On 2019-03-25 09:51, Eric Dumazet wrote: > > > On 03/24/2019 09:56 AM, Felix Fietkau wrote: >> Since we're freeing multiple skbs, we might as well use bulk free to save a >> few cycles. Use the same conditions for bulk free as in napi_consume_skb. >> > > I do not believe kfree_skb_list() is used in the fast path, so do we really > need to make it so complex ? mac80211 uses it to free the fraglist from A-MSDU aggregated packets in the tx status path. That's one fast path where it gets used right now and the reason it was showing up in my perf traces. - Felix
On 03/25/2019 02:09 AM, Felix Fietkau wrote: > On 2019-03-25 09:51, Eric Dumazet wrote: >> >> >> On 03/24/2019 09:56 AM, Felix Fietkau wrote: >>> Since we're freeing multiple skbs, we might as well use bulk free to save a >>> few cycles. Use the same conditions for bulk free as in napi_consume_skb. >>> >> >> I do not believe kfree_skb_list() is used in the fast path, so do we really >> need to make it so complex ? > mac80211 uses it to free the fraglist from A-MSDU aggregated packets in > the tx status path. That's one fast path where it gets used right now > and the reason it was showing up in my perf traces. This is not drop monitor friendly then.... TX completion should use consume_skb() or dev_kfree_skb() BTW, I wonder what drop-monitor signal bulk free is sending ?
On 2019-03-25 10:31, Eric Dumazet wrote: > > > On 03/25/2019 02:09 AM, Felix Fietkau wrote: >> On 2019-03-25 09:51, Eric Dumazet wrote: >>> >>> >>> On 03/24/2019 09:56 AM, Felix Fietkau wrote: >>>> Since we're freeing multiple skbs, we might as well use bulk free to save a >>>> few cycles. Use the same conditions for bulk free as in napi_consume_skb. >>>> >>> >>> I do not believe kfree_skb_list() is used in the fast path, so do we really >>> need to make it so complex ? >> mac80211 uses it to free the fraglist from A-MSDU aggregated packets in >> the tx status path. That's one fast path where it gets used right now >> and the reason it was showing up in my perf traces. > > This is not drop monitor friendly then.... > > TX completion should use consume_skb() or dev_kfree_skb() > > BTW, I wonder what drop-monitor signal bulk free is sending ? Good point about the drop monitor. Would you prefer that I replace this patch with one that adds a consume_skb_list function and another one that makes mac80211 use it? - Felix
On Mon, 25 Mar 2019 10:35:35 +0100 Felix Fietkau <nbd@nbd.name> wrote: > On 2019-03-25 10:31, Eric Dumazet wrote: > > On 03/25/2019 02:09 AM, Felix Fietkau wrote: > >> On 2019-03-25 09:51, Eric Dumazet wrote: > >>> On 03/24/2019 09:56 AM, Felix Fietkau wrote: > >>>> > >>>> Since we're freeing multiple skbs, we might as well use bulk > >>>> free to save a few cycles. Use the same conditions for bulk free > >>>> as in napi_consume_skb. > >>> > >>> I do not believe kfree_skb_list() is used in the fast path, so do > >>> we really need to make it so complex ? > >> > >> mac80211 uses it to free the fraglist from A-MSDU aggregated > >> packets in the tx status path. That's one fast path where it gets > >> used right now and the reason it was showing up in my perf > >> traces. Notice that kfree_skb_list(to_free) is used in (what I consider) "fast-path" when the qdisc system drops packets, happens via to_free in bottom of __dev_xmit_skb(). And fq_codel (which is default in most distro's today) have an optimization (in fq_codel_drop()) for bulk freeing SKBs, which will also benefit from this. Thus, I still think it will be valuable to optimize kfree_skb_list(). > > This is not drop monitor friendly then.... > > > > TX completion should use consume_skb() or dev_kfree_skb() > > > > BTW, I wonder what drop-monitor signal bulk free is sending ? > > Good point about the drop monitor. Would you prefer that I replace > this patch with one that adds a consume_skb_list function and another > one that makes mac80211 use it?
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 2415d9cb9b89..1eeaa264d2a4 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -666,12 +666,44 @@ EXPORT_SYMBOL(kfree_skb); void kfree_skb_list(struct sk_buff *segs) { - while (segs) { - struct sk_buff *next = segs->next; + struct sk_buff *next = segs; + void *skbs[16]; + int n_skbs = 0; - kfree_skb(segs); - segs = next; + while ((segs = next) != NULL) { + next = segs->next; + + if (!skb_unref(segs)) + continue; + + if (segs->fclone != SKB_FCLONE_UNAVAILABLE) { + kfree_skb(segs); + continue; + } + + trace_kfree_skb(segs, __builtin_return_address(0)); + + /* drop skb->head and call any destructors for packet */ + skb_release_all(segs); + +#ifdef CONFIG_SLUB + /* SLUB writes into objects when freeing */ + prefetchw(segs); +#endif + + skbs[n_skbs++] = segs; + + if (n_skbs < ARRAY_SIZE(skbs)) + continue; + + kmem_cache_free_bulk(skbuff_head_cache, n_skbs, skbs); + n_skbs = 0; } + + if (!n_skbs) + return; + + kmem_cache_free_bulk(skbuff_head_cache, n_skbs, skbs); } EXPORT_SYMBOL(kfree_skb_list);
Since we're freeing multiple skbs, we might as well use bulk free to save a few cycles. Use the same conditions for bulk free as in napi_consume_skb. Signed-off-by: Felix Fietkau <nbd@nbd.name> --- v2: call kmem_cache_free_bulk once the skb array is full instead of falling back to kfree_skb net/core/skbuff.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)