[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d56902b7-1e1a-402e-b0b8-9080e9115add@mojatatu.com>
Date: Fri, 2 May 2025 19:13:23 -0300
From: Victor Nogueira <victor@...atatu.com>
To: Paolo Abeni <pabeni@...hat.com>, Cong Wang <xiyou.wangcong@...il.com>,
netdev@...r.kernel.org
Cc: jhs@...atatu.com, jiri@...nulli.us, alan@...ie.me.uk
Subject: Re: [Patch net 1/2] sch_htb: make htb_deactivate() idempotent
On 5/2/25 06:59, Paolo Abeni wrote:
> On 4/30/25 5:19 PM, Victor Nogueira wrote:
>> If that's so, couldn't we instead of doing:
>>
>>> @@ -348,7 +348,8 @@ static void htb_add_to_wait_tree(struct htb_sched *q,
>>> */
>>> static inline void htb_next_rb_node(struct rb_node **n)
>>> {
>>> - *n = rb_next(*n);
>>> + if (*n)
>>> + *n = rb_next(*n);
>>> }
>>
>> do something like:
>>
>> @@ -921,7 +921,9 @@ static struct sk_buff *htb_dequeue_tree(struct
>> htb_sched *q, const int prio,
>> cl->leaf.deficit[level] -= qdisc_pkt_len(skb);
>> if (cl->leaf.deficit[level] < 0) {
>> cl->leaf.deficit[level] += cl->quantum;
>> - htb_next_rb_node(level ?
>> &cl->parent->inner.clprio[prio].ptr :
>> + /* Account for (fq_)codel child deactivating
>> after dequeue */
>> + if (likely(cl->prio_activity))
>> + htb_next_rb_node(level ?
>> &cl->parent->inner.clprio[prio].ptr :
>>
>> &q->hlevel[0].hprio[prio].ptr);
>> }
>> /* this used to be after charge_class but this constelation
>>
>> That way it's clear that the issue is a corner case where the
>> child qdisc deactivates the parent. Otherwise it seems like
>> we need to check for (*n) being NULL for every call to
>> htb_next_rb_node.
>
> @Victor, I think that the Cong's suggested patch is simpler to very/tie
> to code change to the actually addressed issue. I started at your
> suggest code for a bit, and out of sheer htb ignorance I'm less
> confident about it fixing the thing.
>
> Do you have strong feeling vs Cong's suggested approach?
I have no strong feelings, I'm ok with merging the patch
as is.
cheers,
Victor
Powered by blists - more mailing lists