Re: [net-next, v4 10/10] bnge: add cpu_rmap support for IRQ affinity

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Now that this patch publishes rx_cpu_rmap, the core starts
  passing target queues to bnge_rx_flow_steer().

--

bnge: add cpu_rmap support for IRQ affinity

When CONFIG_RFS_ACCEL is enabled, this patch allocates dev->rx_cpu_rmap
and fills it with the RX IRQ vectors. The kernel's aRFS code can then map
flows to the RX queue serviced by the nearest CPU and call
ndo_rx_flow_steer() for that queue.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index de7818d2c2af2..a4156a12c8714 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2437,10 +2438,23 @@ static int bnge_setup_interrupts(struct bnge_net *bn)
>  {
>  	struct net_device *dev = bn->netdev;
>  	struct bnge_dev *bd = bn->bd;
> +	int rc;
>  
>  	bnge_setup_msix(bn);
>  
> -	return netif_set_real_num_queues(dev, bd->tx_nr_rings, bd->rx_nr_rings);
> +	rc = netif_set_real_num_queues(dev, bd->tx_nr_rings, bd->rx_nr_rings);
> +	if (rc)
> +		return rc;
> +
> +#ifdef CONFIG_RFS_ACCEL
> +	if (bn->priv_flags & BNGE_NET_EN_NTUPLE) {
> +		dev->rx_cpu_rmap = alloc_irq_cpu_rmap(bd->rx_nr_rings);

[Severity: Low]
Once dev->rx_cpu_rmap is set here, the core starts calling
bnge_rx_flow_steer() with CPU-derived target queues. That function came
from an earlier patch in this series ("bnge: add aRFS flow steering ndo
support"), but it can only be reached after this patch.

When bnge_rx_flow_steer() finds an existing filter for the flow, it
returns that filter's id. It never compares fltr->base.rxq with
rxq_index:

drivers/net/ethernet/broadcom/bnge/bnge_netdev.c:bnge_rx_flow_steer() {
    ...
	fltr = bnge_lookup_ntp_filter_from_idx(bn, new_fltr, idx);
	/* Filter already exists; return its id.  A stale filter (queue
	 * changed) is freed later via rps_may_expire_flow() and recreated.
	 */
	if (fltr) {
		rc = fltr->base.sw_id;
		rcu_read_unlock();
		goto err_free;
	}
    ...
}

set_rps_cpu() counts any non-negative return as success. It stores the id
in the target queue's flow table and clears the old queue's entry:

net/core/dev.c:set_rps_cpu() {
    ...
	rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
						rxq_index, flow_id);
	if (rc < 0)
		goto out;

	old_rflow = rflow;
	rflow = tmp_rflow;
	WRITE_ONCE(rflow->filter, rc);
	WRITE_ONCE(rflow->hash, hash);

	if (old_rflow->filter == rc)
		WRITE_ONCE(old_rflow->filter, RPS_NO_FILTER);
    ...
}

The hardware filter keeps steering to the old queue anyway.

Next, bnge_cfg_ntp_filters() calls rps_may_expire_flow() with
fltr->base.rxq, which is still the old queue. The core has already cleared
that slot, so the filter is expired and freed while the flow is still
active.

Is the "recreated" part of that comment actually guaranteed? The target
queue's entry still holds the sw_id, with cpu equal to next_cpu. After the
flow falls back to RSS, set_rps_cpu() can take this branch without calling
ndo_rx_flow_steer():

		if (READ_ONCE(tmp_rflow->filter) != RPS_NO_FILTER) {
			if (rps_flow_is_active(tmp_rflow,
					       rps_tag_to_log(q_tag_ptr),
					       tmp_cpu)) {
				if (hash != READ_ONCE(tmp_rflow->hash) ||
				    next_cpu == tmp_cpu)
					goto out;
			}
		}

That writes next_cpu into the RSS queue's entry. From then on,
get_rps_cpu() sees tcpu == next_cpu and never calls set_rps_cpu() again.

Could the flow then stay on software RFS, with no hardware filter, until
the application moves to another CPU?

mlx5 and sfc update the existing rule's queue in this case. bnxt uses the
same pattern as bnge.

Would it make sense for the existing-filter path to either reprogram the
filter to rxq_index, or return an error so the core doesn't move the
filter id to the new queue?

> +		if (!dev->rx_cpu_rmap)
> +			return -ENOMEM;
> +	}
> +#endif
> +
> +	return rc;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928061307.1172344-1-vikas.gupta%40broadcom.com