| Message ID | 20220414011004.2378350-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c9a40d1c87e9b5c74b5ac73e81ed3d5be4d1b1af |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net_sched: make qdisc_reset() smaller | expand |
On 2022-04-13 21:10, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > For some unknown reason qdisc_reset() is using > a convoluted way of freeing two lists of skbs. > > Use __skb_queue_purge() instead. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/sched/sch_generic.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index 5bab9f8b8f453526185c3b6df57065450b1e3d89..dba0b3e24af5e84f7116ae9b6fdb6f66b01a896c 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -1019,22 +1019,14 @@ EXPORT_SYMBOL(qdisc_create_dflt); > void qdisc_reset(struct Qdisc *qdisc) > { > const struct Qdisc_ops *ops = qdisc->ops; > - struct sk_buff *skb, *tmp; > > trace_qdisc_reset(qdisc); > > if (ops->reset) > ops->reset(qdisc); > > - skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) { > - __skb_unlink(skb, &qdisc->gso_skb); > - kfree_skb_list(skb); > - } > - > - skb_queue_walk_safe(&qdisc->skb_bad_txq, skb, tmp) { > - __skb_unlink(skb, &qdisc->skb_bad_txq); > - kfree_skb_list(skb); > - } > + __skb_queue_purge(&qdisc->gso_skb); > + __skb_queue_purge(&qdisc->skb_bad_txq); > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 13 Apr 2022 18:10:04 -0700 you wrote: > From: Eric Dumazet <edumazet@google.com> > > For some unknown reason qdisc_reset() is using > a convoluted way of freeing two lists of skbs. > > Use __skb_queue_purge() instead. > > [...] Here is the summary with links: - [net-next] net_sched: make qdisc_reset() smaller https://git.kernel.org/netdev/net-next/c/c9a40d1c87e9 You are awesome, thank you!
On Wed, Apr 13, 2022 at 6:11 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > > For some unknown reason qdisc_reset() is using > a convoluted way of freeing two lists of skbs. > > Use __skb_queue_purge() instead. [...] > > - skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) { > - __skb_unlink(skb, &qdisc->gso_skb); > - kfree_skb_list(skb); Isn't it precisely because of this kfree_skb_list()? Thanks.
On Sun, Apr 17, 2022 at 3:30 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Wed, Apr 13, 2022 at 6:11 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > From: Eric Dumazet <edumazet@google.com> > > > > For some unknown reason qdisc_reset() is using > > a convoluted way of freeing two lists of skbs. > > > > Use __skb_queue_purge() instead. > [...] > > > > - skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) { > > - __skb_unlink(skb, &qdisc->gso_skb); > > - kfree_skb_list(skb); > > Isn't it precisely because of this kfree_skb_list()? Note the __skb_unlink(skb, &qdisc->gso_skb) which happens at the line above. __skb_unlink(...) { ... skb->next = skb->prev = NULL; ... } This means skb->next is NULL, thus there is no list of skb attached to @skb ? kfree_skb_list(skb) is thus a convoluted way to call kfree_skb(skb) It seems to me that this construct had been copy/pasted from elsewhere.
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 5bab9f8b8f453526185c3b6df57065450b1e3d89..dba0b3e24af5e84f7116ae9b6fdb6f66b01a896c 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -1019,22 +1019,14 @@ EXPORT_SYMBOL(qdisc_create_dflt); void qdisc_reset(struct Qdisc *qdisc) { const struct Qdisc_ops *ops = qdisc->ops; - struct sk_buff *skb, *tmp; trace_qdisc_reset(qdisc); if (ops->reset) ops->reset(qdisc); - skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) { - __skb_unlink(skb, &qdisc->gso_skb); - kfree_skb_list(skb); - } - - skb_queue_walk_safe(&qdisc->skb_bad_txq, skb, tmp) { - __skb_unlink(skb, &qdisc->skb_bad_txq); - kfree_skb_list(skb); - } + __skb_queue_purge(&qdisc->gso_skb); + __skb_queue_purge(&qdisc->skb_bad_txq); qdisc->q.qlen = 0; qdisc->qstats.backlog = 0;