[PATCH net] sch_htb: Avoid grafting on htb_destroy_class_offload when destroying htb

[PATCH net] sch_htb: Avoid grafting on htb_destroy_class_offload when destroying htb

[Rahul Rameshbabu]

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
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.

Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Maxim Mikityanskiy <maxtram95@gmail.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

Re: [PATCH net] sch_htb: Avoid grafting on htb_destroy_class_offload when destroying htb

[Jiri Pirko]

Tue, Jan 10, 2023 at 09:20:04PM CET, rrameshbabu@nvidia.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.


>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?


>
>Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
>Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
>Cc: Eric Dumazet <edumazet@google.com>
>Cc: Maxim Mikityanskiy <maxtram95@gmail.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
>

Re: [PATCH net] sch_htb: Avoid grafting on htb_destroy_class_offload when destroying htb

[Rahul Rameshbabu]

Jiri Pirko <jiri@resnulli.us> writes:

> Tue, Jan 10, 2023 at 09:20:04PM CET, rrameshbabu@nvidia.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@nvidia.com>
>>Cc: Eric Dumazet <edumazet@google.com>
>>Cc: Maxim Mikityanskiy <maxtram95@gmail.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
>>