netdev - Re: [PATCH net-next V2 2/2] net: sched: generalize check for no-op qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ikndv438.fsf@toke.dk>
Date: Wed, 09 Apr 2025 15:47:55 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>, Eric Dumazet
 <eric.dumazet@...il.com>, netdev@...r.kernel.org, Jakub Kicinski
 <kuba@...nel.org>, edumazet@...gle.com
Cc: bpf@...r.kernel.org, tom@...bertland.com, "David S. Miller"
 <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>,
 dsahern@...nel.org, makita.toshiaki@....ntt.co.jp,
 kernel-team@...udflare.com
Subject: Re: [PATCH net-next V2 2/2] net: sched: generalize check for no-op
 qdisc

Jesper Dangaard Brouer <hawk@...nel.org> writes:

> On 08/04/2025 17.47, Eric Dumazet wrote:
>> 
>> On 4/8/25 5:31 PM, Jesper Dangaard Brouer wrote:
>>> Several drivers (e.g., veth, vrf) contain open-coded checks to determine
>>> whether a TX queue has a real qdisc attached - typically by testing if
>>> qdisc->enqueue is non-NULL.
>>>
>>> These checks are functionally equivalent to comparing the queue's qdisc
>>> pointer against &noop_qdisc (qdisc named "noqueue"). This equivalence
>>> stems from noqueue_init(), which explicitly clears the enqueue pointer
>>> for the "noqueue" qdisc. As a result, __dev_queue_xmit() treats the qdisc
>>> as a no-op only when enqueue == NULL.
>>>
>>> This patch introduces a common helper, qdisc_txq_is_noop() to standardize
>>> this check. The helper is added in sch_generic.h and replaces open-coded
>>> logic in both the veth and vrf drivers.
>>>
>>> This is a non-functional change.
>>>
>>> Signed-off-by: Jesper Dangaard Brouer <hawk@...nel.org>
>>> ---
>>>   drivers/net/veth.c        |   14 +-------------
>>>   drivers/net/vrf.c         |    3 +--
>>>   include/net/sch_generic.h |    7 ++++++-
>>>   3 files changed, 8 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>>> index f29a0db2ba36..83c7758534da 100644
>>> --- a/drivers/net/veth.c
>>> +++ b/drivers/net/veth.c
>>> @@ -341,18 +341,6 @@ static bool veth_skb_is_eligible_for_gro(const 
>>> struct net_device *dev,
>>>            rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD));
>>>   }
>>> -/* Does specific txq have a real qdisc attached? - see noqueue_init() */
>>> -static inline bool txq_has_qdisc(struct netdev_queue *txq)
>>> -{
>>> -    struct Qdisc *q;
>>> -
>>> -    q = rcu_dereference(txq->qdisc);
>>> -    if (q->enqueue)
>>> -        return true;
>>> -    else
>>> -        return false;
>>> -}
>>> -
>>>   static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device 
>>> *dev)
>>>   {
>>>       struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
>>> @@ -399,7 +387,7 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, 
>>> struct net_device *dev)
>>>            */
>>>           txq = netdev_get_tx_queue(dev, rxq);
>>> -        if (!txq_has_qdisc(txq)) {
>>> +        if (qdisc_txq_is_noop(txq)) {
>>>               dev_kfree_skb_any(skb);
>>>               goto drop;
>>>           }
>>> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
>>> index 7168b33adadb..d4fe36c55f29 100644
>>> --- a/drivers/net/vrf.c
>>> +++ b/drivers/net/vrf.c
>>> @@ -349,9 +349,8 @@ static bool qdisc_tx_is_default(const struct 
>>> net_device *dev)
>>>           return false;
>>>       txq = netdev_get_tx_queue(dev, 0);
>>> -    qdisc = rcu_access_pointer(txq->qdisc);
>>> -    return !qdisc->enqueue;
>>> +    return qdisc_txq_is_noop(txq);
>>>   }
>>>   /* Local traffic destined to local address. Reinsert the packet to rx
>>> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
>>> index d48c657191cd..eb90d5103371 100644
>>> --- a/include/net/sch_generic.h
>>> +++ b/include/net/sch_generic.h
>>> @@ -803,6 +803,11 @@ static inline bool qdisc_tx_changing(const struct 
>>> net_device *dev)
>>>       return false;
>>>   }
>>> +static inline bool qdisc_txq_is_noop(const struct netdev_queue *txq)
>>> +{
>>> +    return (rcu_access_pointer(txq->qdisc) == &noop_qdisc);
>> 
>> 
>> return (expression);
>> 
>> ->
>> 
>> return expression;
>> 
>> 
>> return rcu_access_pointer(txq->qdisc) == &noop_qdisc;
>
> Will fix in next iteration.
>
>> I also feel this patch should come first in the series ?
>> 
>
> To me it looks/feels wrong doing this before there are two users.
> With only the vrf driver, the changed looked unnecessary.
> The diff stats looks/feels wrong, when it's patch-1.

Generalising something in preparation for another user is pretty normal,
isn't it? Just write this in the commit message, like: "In preparation
for using this in more places, move the check for a noop qdisc from the
vrf driver into sch_generic.h" - or something like that?

-Toke