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: <e40c4f92-cc4f-49d2-9d7f-e2d88aeba873@kernel.org>
Date: Wed, 9 Apr 2025 15:28:55 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: 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>,
 Toke Høiland-Jørgensen <toke@...e.dk>,
 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



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.

As I have to respin anyhow, I will let you decide.
Please let me know, if you prefer this to be patch-1 ?

--Jesper