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: <1ad134d3-4173-4d43-b2ad-0b2c5165bbc1@gmail.com>
Date: Tue, 8 Apr 2025 17:47:49 +0200
From: Eric Dumazet <eric.dumazet@...il.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>, 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 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;

I also feel this patch should come first in the series ?