Re: [PATCH AUTOSEL 5.13 09/19] sfc: ensure correct number of XDP queues

Íñigo Huguet
2021-07-23T10:13:04Z

Hi,

On Fri, Jul 23, 2021 at 5:57 AM Sasha Levin <sashal@kernel.org> wrote:
>
> From: Íñigo Huguet <ihuguet@redhat.com>
>
> [ Upstream commit 788bc000d4c2f25232db19ab3a0add0ba4e27671 ]

Applying this commit alone could be problematic, leading even to
kernel crash in certain situations.

The real fix is the previous one of the series:
f43a24f446da sfc: fix lack of XDP TX queues - error XDP TX failed (-22)

This one can be applied too, but not really a must-have.

> Commit 99ba0ea616aa ("sfc: adjust efx->xdp_tx_queue_count with the real
> number of initialized queues") intended to fix a problem caused by a
> round up when calculating the number of XDP channels and queues.
> However, this was not the real problem. The real problem was that the
> number of XDP TX queues had been reduced to half in
> commit e26ca4b53582 ("sfc: reduce the number of requested xdp ev queues"),
> but the variable xdp_tx_queue_count had remained the same.
>
> Once the correct number of XDP TX queues is created again in the
> previous patch of this series, this also can be reverted since the error
> doesn't actually exist.
>
> Only in the case that there is a bug in the code we can have different
> values in xdp_queue_number and efx->xdp_tx_queue_count. Because of this,
> and per Edward Cree's suggestion, I add instead a WARN_ON to catch if it
> happens again in the future.
>
> Note that the number of allocated queues can be higher than the number
> of used ones due to the round up, as explained in the existing comment
> in the code. That's why we also have to stop increasing xdp_queue_number
> beyond efx->xdp_tx_queue_count.
>
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/net/ethernet/sfc/efx_channels.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> index a3ca406a3561..bea0b27baf4b 100644
> --- a/drivers/net/ethernet/sfc/efx_channels.c
> +++ b/drivers/net/ethernet/sfc/efx_channels.c
> @@ -891,18 +891,20 @@ int efx_set_channels(struct efx_nic *efx)
> if (efx_channel_is_xdp_tx(channel)) {
> efx_for_each_channel_tx_queue(tx_queue, channel) {
> tx_queue->queue = next_queue++;
> - netif_dbg(efx, drv, efx->net_dev, "Channel %u TXQ %u is XDP %u, HW %u\n",
> - channel->channel, tx_queue->label,
> - xdp_queue_number, tx_queue->queue);
> +
> /* We may have a few left-over XDP TX
> * queues owing to xdp_tx_queue_count
> * not dividing evenly by EFX_MAX_TXQ_PER_CHANNEL.
> * We still allocate and probe those
> * TXQs, but never use them.
> */
> - if (xdp_queue_number < efx->xdp_tx_queue_count)
> + if (xdp_queue_number < efx->xdp_tx_queue_count) {
> + netif_dbg(efx, drv, efx->net_dev, "Channel %u TXQ %u is XDP %u, HW %u\n",
> + channel->channel, tx_queue->label,
> + xdp_queue_number, tx_queue->queue);
> efx->xdp_tx_queues[xdp_queue_number] = tx_queue;
> - xdp_queue_number++;
> + xdp_queue_number++;
> + }
> }
> } else {
> efx_for_each_channel_tx_queue(tx_queue, channel) {
> @@ -914,8 +916,7 @@ int efx_set_channels(struct efx_nic *efx)
> }
> }
> }
> - if (xdp_queue_number)
> - efx->xdp_tx_queue_count = xdp_queue_number;
> + WARN_ON(xdp_queue_number != efx->xdp_tx_queue_count);
>
> rc = netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
> if (rc)
> --
> 2.30.2
>

--
Íñigo Huguet