[PATCH net-next] net_sched: make qdisc_reset() smaller

[PATCH net-next] net_sched: make qdisc_reset() smaller

[Eric Dumazet]

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);
 
 	qdisc->q.qlen = 0;
 	qdisc->qstats.backlog = 0;
-- 
2.35.1.1178.g4f1659d476-goog

Re: [PATCH net-next] net_sched: make qdisc_reset() smaller

[Jamal Hadi Salim]

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

Re: [PATCH net-next] net_sched: make qdisc_reset() smaller

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net_sched: make qdisc_reset() smaller

[Cong Wang]

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.

Re: [PATCH net-next] net_sched: make qdisc_reset() smaller

[Eric Dumazet]

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.