Re: [PATCH net-next 2/5] sfc: separate channel->tx_queue and efx->xdp_tx_queue mappings

[Martin Habets <habetsm.xilinx@gmail.com>]

On Tue, May 10, 2022 at 10:44:40AM +0200, Íñigo Huguet wrote:
> Channels that contains tx queues need to determine the mapping of this
> queue structures to hw queue numbers. This applies both to all tx
> queues, no matter if they are normal tx queues, xdp_tx queues or both at
> the same time.

In my thinking XDP-only channels are a different channel type, so it
would be cleaner to define a separate struct efx_channel_type for those.

> 
> Also, a lookup table to map each cpu to a xdp_tx queue is created,
> containing pointers to the xdp_tx queues, that should already be
> allocated in one or more channels. This lookup table is global to all
> efx_nic structure.

I'm not keen on a direct CPU to queue mapping, but rather map the
specific XDP-only channels to CPUs. Also for such a mapping there is
XPS already. Ideally that configuration will be used.

> Mappings to hw queues and xdp lookup table creation were done at the
> same time in efx_set_channels, but it had a bit messy and not very clear
> code. Then, commit 059a47f1da93 ("net: sfc: add missing xdp queue
> reinitialization") moved part of that initialization to a separate
> function to fix a bug produced because the xdp_tx queues lookup table
> was not reinitialized after channels reallocation, leaving it pointing
> to deallocated queues. Not all of that initialization needs to be
> redone, but only the xdp_tx queues lookup table, and not the mappings to
> hw queues. So this resulted in even less clear code.
> 
> This patch moves back the part of that code that doesn't need to be
> reinitialized. That is, the mapping of tx queues with hw queues numbers.
> As a result, xdp queues lookup table creation and this are done in
> different places, conforming to single responsibility principle and
> resulting in more clear code.
> 
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> ---
>  drivers/net/ethernet/sfc/efx_channels.c | 69 +++++++++++++------------
>  1 file changed, 37 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> index 3f28f9861dfa..8feba80f0a34 100644
> --- a/drivers/net/ethernet/sfc/efx_channels.c
> +++ b/drivers/net/ethernet/sfc/efx_channels.c
> @@ -767,6 +767,19 @@ void efx_remove_channels(struct efx_nic *efx)
>  	kfree(efx->xdp_tx_queues);
>  }
>  
> +static inline int efx_alloc_xdp_tx_queues(struct efx_nic *efx)
> +{
> +	if (efx->xdp_tx_queue_count) {
> +		EFX_WARN_ON_PARANOID(efx->xdp_tx_queues);
> +		efx->xdp_tx_queues = kcalloc(efx->xdp_tx_queue_count,
> +					     sizeof(*efx->xdp_tx_queues),
> +					     GFP_KERNEL);

efx_set_channels() can be called multiple times. In that case
the previous memory allocated is not freed and thus it is leaked.

Martin

> +		if (!efx->xdp_tx_queues)
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
>  static int efx_set_xdp_tx_queue(struct efx_nic *efx, int xdp_queue_number,
>  				struct efx_tx_queue *tx_queue)
>  {
> @@ -789,44 +802,29 @@ static void efx_set_xdp_channels(struct efx_nic *efx)
>  	int xdp_queue_number = 0;
>  	int rc;
>  
> -	/* We need to mark which channels really have RX and TX
> -	 * queues, and adjust the TX queue numbers if we have separate
> -	 * RX-only and TX-only channels.
> -	 */
>  	efx_for_each_channel(channel, efx) {
>  		if (channel->channel < efx->tx_channel_offset)
>  			continue;
>  
>  		if (efx_channel_is_xdp_tx(channel)) {
>  			efx_for_each_channel_tx_queue(tx_queue, channel) {
> -				tx_queue->queue = next_queue++;
>  				rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
>  							  tx_queue);
>  				if (rc == 0)
>  					xdp_queue_number++;
>  			}
> -		} else {
> -			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 HW %u\n",
> -					  channel->channel, tx_queue->label,
> -					  tx_queue->queue);
> -			}
> +		} else if (efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_BORROWED) {
>  
>  			/* If XDP is borrowing queues from net stack, it must
>  			 * use the queue with no csum offload, which is the
>  			 * first one of the channel
>  			 * (note: tx_queue_by_type is not initialized yet)
>  			 */
> -			if (efx->xdp_txq_queues_mode ==
> -			    EFX_XDP_TX_QUEUES_BORROWED) {
> -				tx_queue = &channel->tx_queue[0];
> -				rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
> -							  tx_queue);
> -				if (rc == 0)
> -					xdp_queue_number++;
> -			}
> +			tx_queue = &channel->tx_queue[0];
> +			rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
> +						  tx_queue);
> +			if (rc == 0)
> +				xdp_queue_number++;
>  		}
>  	}
>  	WARN_ON(efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_DEDICATED &&
> @@ -952,31 +950,38 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
>  
>  int efx_set_channels(struct efx_nic *efx)
>  {
> +	struct efx_tx_queue *tx_queue;
>  	struct efx_channel *channel;
> +	unsigned int queue_num = 0;
>  	int rc;
>  
>  	efx->tx_channel_offset =
>  		efx_separate_tx_channels ?
>  		efx->n_channels - efx->n_tx_channels : 0;
>  
> -	if (efx->xdp_tx_queue_count) {
> -		EFX_WARN_ON_PARANOID(efx->xdp_tx_queues);
> -
> -		/* Allocate array for XDP TX queue lookup. */
> -		efx->xdp_tx_queues = kcalloc(efx->xdp_tx_queue_count,
> -					     sizeof(*efx->xdp_tx_queues),
> -					     GFP_KERNEL);
> -		if (!efx->xdp_tx_queues)
> -			return -ENOMEM;
> -	}
> -
> +	/* We need to mark which channels really have RX and TX queues, and
> +	 * adjust the TX queue numbers if we have separate RX/TX only channels.
> +	 */
>  	efx_for_each_channel(channel, efx) {
>  		if (channel->channel < efx->n_rx_channels)
>  			channel->rx_queue.core_index = channel->channel;
>  		else
>  			channel->rx_queue.core_index = -1;
> +
> +		if (channel->channel >= efx->tx_channel_offset) {
> +			efx_for_each_channel_tx_queue(tx_queue, channel) {
> +				tx_queue->queue = queue_num++;
> +				netif_dbg(efx, drv, efx->net_dev,
> +					  "Channel %u TXQ %u is HW %u\n",
> +					  channel->channel, tx_queue->label,
> +					  tx_queue->queue);
> +			}
> +		}
>  	}
>  
> +	rc = efx_alloc_xdp_tx_queues(efx);
> +	if (rc)
> +		return rc;
>  	efx_set_xdp_channels(efx);
>  
>  	rc = netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
> -- 
> 2.34.1