Re: [PATCH net v3] net: dsa: clear the conduit RX queue on receive

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message lists what changes for "code that reads the
  recorded queue later" (skb_tx_hash() and sk_rx_queue_mapping/xps_rxqs).

--

net: dsa: clear the conduit RX queue on receive

This patch clears skb->queue_mapping in dsa_switch_rcv() after the tagger
has chosen skb->dev. The conduit's RX queue index is then no longer carried
over to a DSA user port, or to a bond/team that receives trunk-tagged
frames. With that, rps_cpus on user ports applies to traffic from every
conduit queue, not just queue 0.

> diff --git a/net/dsa/tag.c b/net/dsa/tag.c
> index 6a5aa7fc331c3..e53dc3f3c477c 100644
> --- a/net/dsa/tag.c
> +++ b/net/dsa/tag.c
> @@ -95,6 +95,13 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  	skb->pkt_type = PACKET_HOST;
>  	skb->protocol = eth_type_trans(skb, skb->dev);
>  
> +	/* The RX queue the conduit recorded is an index into the conduit's
> +	 * queues. It means nothing on skb->dev, a user port with a single RX
> +	 * queue or a bond/team with its own queue count, and get_rps_cpu()
> +	 * would check it against that device. Leave no queue recorded.
> +	 */
> +	skb_set_queue_mapping(skb, 0);

[Severity: Medium]
The commit message lists what changes for code that reads the recorded
queue later:

  This leaves no RX queue recorded, rather than recording queue 0. Both
  make get_rps_cpu() use the first RX queue's map. They differ for code
  that reads the recorded queue later:

It then covers skb_tx_hash() and sk_rx_queue_mapping/xps_rxqs. Are there
other readers of skb->queue_mapping on the user port or bond/team that
the message should mention?

One reader that userspace can see is PACKET_FANOUT_QM, in
net/packet/af_packet.c:

static unsigned int fanout_demux_qm(struct packet_fanout *f,
				    struct sk_buff *skb,
				    unsigned int num)
{
	return skb_get_queue_mapping(skb) % num;
}

Take a QM fanout group bound to a DSA user port, or to the bond/team that
receives trunk-tagged frames. Before this patch, its packets were spread
across members by conduit RX ring, as (ring + 1) % num.

Now that queue_mapping is always 0 here, will every packet go to member 0?
Recording queue 0 instead would send them all to member 1 % num. In both
cases the group would stop spreading load.

The same applies to tc/BPF ingress programs that read
__sk_buff->queue_mapping, and to the tc ematch meta queue_mapping
selector. On DSA ports they would now always see 0, not the conduit ring
index.

This patch has a Fixes: tag and is headed for net/stable. Could the
commit message also describe these two changes: how QM fanout distributes
packets, and the queue_mapping value that tc/BPF sees on DSA ports?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107210653.2007194-1-dencity7%40gmail.com