[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <875ydcrgjf.fsf@nvidia.com>
Date: Wed, 11 Jan 2023 11:56:52 -0800
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, Tariq Toukan <tariqt@...dia.com>,
Gal Pressman <gal@...dia.com>,
Saeed Mahameed <saeedm@...dia.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
David Miller <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Maxim Mikityanskiy <maxtram95@...il.com>
Subject: Re: [PATCH net] sch_htb: Avoid grafting on
htb_destroy_class_offload when destroying htb
Jiri Pirko <jiri@...nulli.us> writes:
> Tue, Jan 10, 2023 at 09:20:04PM CET, rrameshbabu@...dia.com wrote:
>>Peek at old qdisc and graft only when deleting leaf class in the htb. When
>>destroying the htb, the caller may already have grafted a new qdisc that is
>>not part of the htb structure being destroyed. htb_destroy_class_offload
>>should not peek at the qdisc of the netdev queue since that will either be
>
> You are not telling the codebase what to do. Do it in order to make it
> obvious what the patch is doing. That makes the patch description easier
> to understand.
>
I do think this description does describe what the patch is trying to
logically achieve rather than what the codebase itself is doing. The
goal of this patch is to correct htb_destroy_class_offload, so that it
will not attempt to graft a qdisc that does not belong to the htb (the
new qdisc), when destroying the htb entirely (the htb is destroyed when
replaced with another qdisc or explicitly destroyed). When deleting a
leaf, it makes sense to graft the leaf back if it failed to be deleted
by the device.
>
>>the new qdisc in the case of replacing the htb or simply a noop_qdisc is
>>the case of destroying the htb without a replacement qdisc.
>
>
> Looks to me like 2 fixes, shouldn't this be 2 patches instead?
This change is one logical fix in terms of refactoring the code to only
peek and graft the old qdisc when deleting a leaf of the htb. It
resolves issues in two use cases though.
tc qdisc delete dev eth2 root handle 1: htb default 1 # deleting the htb
tc qdisc replace dev eth2 root pfifo # replace htb with another qdisc
>
>
>>
>>Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
>>Signed-off-by: Rahul Rameshbabu <rrameshbabu@...dia.com>
>>Cc: Eric Dumazet <edumazet@...gle.com>
>>Cc: Maxim Mikityanskiy <maxtram95@...il.com>
>>---
>> net/sched/sch_htb.c | 23 +++++++++++++----------
>> 1 file changed, 13 insertions(+), 10 deletions(-)
>>
>>diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
>>index 2238edece1a4..360ce8616fd2 100644
>>--- a/net/sched/sch_htb.c
>>+++ b/net/sched/sch_htb.c
>>@@ -1557,14 +1557,13 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
>>
>> WARN_ON(!q);
>> dev_queue = htb_offload_get_queue(cl);
>>- old = htb_graft_helper(dev_queue, NULL);
>>- if (destroying)
>>- /* Before HTB is destroyed, the kernel grafts noop_qdisc to
>>- * all queues.
>>+ if (!destroying) {
>>+ old = htb_graft_helper(dev_queue, NULL);
>>+ /* Last qdisc grafted should be the same as cl->leaf.q when
>>+ * calling htb_destroy
>> */
>>- WARN_ON(!(old->flags & TCQ_F_BUILTIN));
>>- else
>> WARN_ON(old != q);
>>+ }
>>
>> if (cl->parent) {
>> _bstats_update(&cl->parent->bstats_bias,
>>@@ -1581,10 +1580,14 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
>> };
>> err = htb_offload(qdisc_dev(sch), &offload_opt);
>>
>>- if (!err || destroying)
>>- qdisc_put(old);
>>- else
>>- htb_graft_helper(dev_queue, old);
>>+ /* htb_offload related errors when destroying cannot be handled */
>>+ WARN_ON(err && destroying);
>>+ if (!destroying) {
>>+ if (!err)
>>+ qdisc_put(old);
>>+ else
>>+ htb_graft_helper(dev_queue, old);
>>+ }
>>
>> if (last_child)
>> return err;
>>--
>>2.36.2
>>
Powered by blists - more mailing lists