[PATCH net-next 0/5] sfc: refactor of efx_set_channels

[PATCH net-next 0/5] sfc: refactor of efx_set_channels

[Íñigo Huguet]

The code configuring RX, TX and XDP_TX queues in efx_set_channels was a
bit messy and difficult to follow. Some of my recent changes to add
support for sharing TX queues for XDP when not enough channels are
available contributed even more to this.

This patch series contains no functional changes, but only a refactor to
make that part of the driver's code clearer and easier to maintain.

Since these changes partially undo commit 059a47f1da93 ("net: sfc: add
missing xdp queue reinitialization"), I've specifically tested against
regression of this change.

Tested: receiving packets with iperf3 and with samples/bpf/xdp_rxq_info.
Then increased ring sizes to force their reallocation and repeated the
tests.

Íñigo Huguet (5):
  sfc: add new helper macros to iterate channels by type
  sfc: separate channel->tx_queue and efx->xdp_tx_queue mappings
  sfc: rename set_channels to set_queues and document it
  sfc: refactor efx_set_xdp_tx_queues
  sfc: move tx_channel_offset calculation to interrupts probe

 drivers/net/ethernet/sfc/ef100_netdev.c |   2 +-
 drivers/net/ethernet/sfc/efx.c          |   2 +-
 drivers/net/ethernet/sfc/efx_channels.c | 155 ++++++++++++------------
 drivers/net/ethernet/sfc/efx_channels.h |   2 +-
 drivers/net/ethernet/sfc/net_driver.h   |  21 ++++
 5 files changed, 99 insertions(+), 83 deletions(-)

-- 
2.34.1

[PATCH net-next 1/5] sfc: add new helper macros to iterate channels by type

[Íñigo Huguet]

Sometimes in the driver it's needed to iterate a subset of the channels
depending on whether it is an rx, tx or xdp channel. Now it's done
iterating over all channels and checking if it's of the desired type,
leading to too much nested and a bit complex to understand code.

Add new iterator macros to allow iterating only over a single type of
channel.

Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/net_driver.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 318db906a154..7f665ba6a082 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -1501,6 +1501,27 @@ efx_get_channel(struct efx_nic *efx, unsigned index)
 	     _channel = (_channel->channel + 1 < (_efx)->n_channels) ?	\
 		     (_efx)->channel[_channel->channel + 1] : NULL)
 
+#define efx_for_each_rx_channel(_channel, _efx)				    \
+	for (_channel = (_efx)->channel[0];				    \
+	     _channel;							    \
+	     _channel = (_channel->channel + 1 < (_efx)->n_rx_channels) ?   \
+		     (_efx)->channel[_channel->channel + 1] : NULL)
+
+#define efx_for_each_tx_channel(_channel, _efx)				    \
+	for (_channel = (_efx)->channel[efx->tx_channel_offset];	    \
+	     _channel;							    \
+	     _channel = (_channel->channel + 1 <			    \
+		     (_efx)->tx_channel_offset + (_efx)->n_tx_channels) ?   \
+		     (_efx)->channel[_channel->channel + 1] : NULL)
+
+#define efx_for_each_xdp_channel(_channel, _efx)			    \
+	for (_channel = ((_efx)->n_xdp_channels > 0) ?			    \
+		     (_efx)->channel[efx->xdp_channel_offset] : NULL;	    \
+	     _channel;							    \
+	     _channel = (_channel->channel + 1 <			    \
+		     (_efx)->xdp_channel_offset + (_efx)->n_xdp_channels) ? \
+		     (_efx)->channel[_channel->channel + 1] : NULL)
+
 /* Iterate over all used channels in reverse */
 #define efx_for_each_channel_rev(_channel, _efx)			\
 	for (_channel = (_efx)->channel[(_efx)->n_channels - 1];	\
-- 
2.34.1

Re: [PATCH net-next 1/5] sfc: add new helper macros to iterate channels by type

[Martin Habets]

Hi Íñigo,

On Tue, May 10, 2022 at 10:44:39AM +0200, Íñigo Huguet wrote:
> Sometimes in the driver it's needed to iterate a subset of the channels
> depending on whether it is an rx, tx or xdp channel. Now it's done
> iterating over all channels and checking if it's of the desired type,
> leading to too much nested and a bit complex to understand code.
> 
> Add new iterator macros to allow iterating only over a single type of
> channel.

We have similar code we'll be upstreaming soon, once we've managed to
split off Siena. The crucial part of that seems to have been done
today.

> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> ---
>  drivers/net/ethernet/sfc/net_driver.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> index 318db906a154..7f665ba6a082 100644
> --- a/drivers/net/ethernet/sfc/net_driver.h
> +++ b/drivers/net/ethernet/sfc/net_driver.h
> @@ -1501,6 +1501,27 @@ efx_get_channel(struct efx_nic *efx, unsigned index)
>  	     _channel = (_channel->channel + 1 < (_efx)->n_channels) ?	\
>  		     (_efx)->channel[_channel->channel + 1] : NULL)
>  
> +#define efx_for_each_rx_channel(_channel, _efx)				    \
> +	for (_channel = (_efx)->channel[0];				    \
> +	     _channel;							    \
> +	     _channel = (_channel->channel + 1 < (_efx)->n_rx_channels) ?   \
> +		     (_efx)->channel[_channel->channel + 1] : NULL)
> +#define efx_for_each_tx_channel(_channel, _efx)				    \
> +	for (_channel = (_efx)->channel[efx->tx_channel_offset];	    \
> +	     _channel;							    \
> +	     _channel = (_channel->channel + 1 <			    \
> +		     (_efx)->tx_channel_offset + (_efx)->n_tx_channels) ?   \
> +		     (_efx)->channel[_channel->channel + 1] : NULL)

We've chosen a different naming conventions here, and we're also removing
the channel array.
Also not every channel has RX queues and not every channel has TX queues.

Sounds like it's time we have another call.
Martin

> +#define efx_for_each_xdp_channel(_channel, _efx)			    \
> +	for (_channel = ((_efx)->n_xdp_channels > 0) ?			    \
> +		     (_efx)->channel[efx->xdp_channel_offset] : NULL;	    \
> +	     _channel;							    \
> +	     _channel = (_channel->channel + 1 <			    \
> +		     (_efx)->xdp_channel_offset + (_efx)->n_xdp_channels) ? \
> +		     (_efx)->channel[_channel->channel + 1] : NULL)
> +
>  /* Iterate over all used channels in reverse */
>  #define efx_for_each_channel_rev(_channel, _efx)			\
>  	for (_channel = (_efx)->channel[(_efx)->n_channels - 1];	\
> -- 
> 2.34.1

Re: [PATCH net-next 1/5] sfc: add new helper macros to iterate channels by type

[Íñigo Huguet]

On Wed, May 11, 2022 at 9:19 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> Hi 婉igo,
>
> On Tue, May 10, 2022 at 10:44:39AM +0200, 婉igo Huguet wrote:
> > Sometimes in the driver it's needed to iterate a subset of the channels
> > depending on whether it is an rx, tx or xdp channel. Now it's done
> > iterating over all channels and checking if it's of the desired type,
> > leading to too much nested and a bit complex to understand code.
> >
> > Add new iterator macros to allow iterating only over a single type of
> > channel.
>
> We have similar code we'll be upstreaming soon, once we've managed to
> split off Siena. The crucial part of that seems to have been done
> today.
>
> > Signed-off-by: 婉igo Huguet <ihuguet@redhat.com>
> > ---
> >  drivers/net/ethernet/sfc/net_driver.h | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> > index 318db906a154..7f665ba6a082 100644
> > --- a/drivers/net/ethernet/sfc/net_driver.h
> > +++ b/drivers/net/ethernet/sfc/net_driver.h
> > @@ -1501,6 +1501,27 @@ efx_get_channel(struct efx_nic *efx, unsigned index)
> >            _channel = (_channel->channel + 1 < (_efx)->n_channels) ?  \
> >                    (_efx)->channel[_channel->channel + 1] : NULL)
> >
> > +#define efx_for_each_rx_channel(_channel, _efx)                                  \
> > +     for (_channel = (_efx)->channel[0];                                 \
> > +          _channel;                                                      \
> > +          _channel = (_channel->channel + 1 < (_efx)->n_rx_channels) ?   \
> > +                  (_efx)->channel[_channel->channel + 1] : NULL)
> > +#define efx_for_each_tx_channel(_channel, _efx)                                  \
> > +     for (_channel = (_efx)->channel[efx->tx_channel_offset];            \
> > +          _channel;                                                      \
> > +          _channel = (_channel->channel + 1 <                            \
> > +                  (_efx)->tx_channel_offset + (_efx)->n_tx_channels) ?   \
> > +                  (_efx)->channel[_channel->channel + 1] : NULL)
>
> We've chosen a different naming conventions here, and we're also removing
> the channel array.
> Also not every channel has RX queues and not every channel has TX queues.
>
> Sounds like it's time we have another call.

I saw you were already upstreaming the siena split, probably it had
been a good idea to wait for it to be merged before sending this.

I'm going to be on PTO the rest of the week and the next one, maybe we
can talk when I'm back, and hopefully you will have made more
progress. Then I can resubmit this series adapted to the new state of
the code, if it's still useful.

> Martin
>
> > +#define efx_for_each_xdp_channel(_channel, _efx)                         \
> > +     for (_channel = ((_efx)->n_xdp_channels > 0) ?                      \
> > +                  (_efx)->channel[efx->xdp_channel_offset] : NULL;       \
> > +          _channel;                                                      \
> > +          _channel = (_channel->channel + 1 <                            \
> > +                  (_efx)->xdp_channel_offset + (_efx)->n_xdp_channels) ? \
> > +                  (_efx)->channel[_channel->channel + 1] : NULL)
> > +
> >  /* Iterate over all used channels in reverse */
> >  #define efx_for_each_channel_rev(_channel, _efx)                     \
> >       for (_channel = (_efx)->channel[(_efx)->n_channels - 1];        \
> > --
> > 2.34.1
>


-- 
Íñigo Huguet

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

[Íñigo Huguet]

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.

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.

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);
+		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

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

[Martin Habets]

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

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

[Íñigo Huguet]

On Wed, May 11, 2022 at 9:52 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> 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.

That would be changes far deeper than this patch series intended to
do. However, I totally agree with you, but maybe it's something better
to be included in the channels rework you're working on? I'm happy to
help on that, if possible.

>
> >
> > 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.

But that mapping is not introduced by me, it already existed, and it's
used in efx_xdp_tx_buffers to have a dedicated TX queue for each CPU,
avoiding locking contention (at least when there are enough channels).

XPS is not an option here, at least not one I can think of, for 2 reasons:
1. When doing XDP_TX action, there doesn't even exist an skb, so there
is no information to be used by XPS
2. XPS create mappings to the normal TX queues, for normal traffic,
not for the XDP ones

In fact, when I sent the patches to allow using normal TX queues for
XDP if there are no free channels for XDP, I already tried to use XPS
to do the CPU mapping, and I didn't find the way to do it, because of
point 1.

I think this is because XDP is still very limited in some things,
hopefully it will improve in the future. AFAIK, all drivers are doing
the same or similar approach of one CPU mapped to one dedicated XDP_TX
queue.

>
> > 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.

This was an already existing bug, that I didn't see, but you're right
we should fix it now. I will submit it to `net`.

>
> 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
>


-- 
Íñigo Huguet

[PATCH net-next 3/5] sfc: rename set_channels to set_queues and document it

[Íñigo Huguet]

Function efx_set_channels had a bit missleading name because it was only
setting some parameters related to queues, but the name suggests that
much more things related to channels are being configured.

Also, function efx_set_xdp_channels has been renamed to
efx_xdp_tx_queues. It was even more missleading because there are cases
where XDP dedicated channels might not exist, but there are still some
xdp_tx queues parameters to configure, for example when sharing tx
queues from normal channels for XDP.

Finally, added some comments as documentation for these functions.

Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ef100_netdev.c |  2 +-
 drivers/net/ethernet/sfc/efx.c          |  2 +-
 drivers/net/ethernet/sfc/efx_channels.c | 16 ++++++++++++----
 drivers/net/ethernet/sfc/efx_channels.h |  2 +-
 4 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
index 67fe44db6b61..fe8253d59782 100644
--- a/drivers/net/ethernet/sfc/ef100_netdev.c
+++ b/drivers/net/ethernet/sfc/ef100_netdev.c
@@ -118,7 +118,7 @@ static int ef100_net_open(struct net_device *net_dev)
 	if (rc)
 		goto fail;
 
-	rc = efx_set_channels(efx);
+	rc = efx_set_queues(efx);
 	if (rc)
 		goto fail;
 
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 5a772354da83..3d8a83b94e0a 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -289,7 +289,7 @@ static int efx_probe_nic(struct efx_nic *efx)
 		if (rc)
 			goto fail1;
 
-		rc = efx_set_channels(efx);
+		rc = efx_set_queues(efx);
 		if (rc)
 			goto fail1;
 
diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
index 8feba80f0a34..1c05063a7215 100644
--- a/drivers/net/ethernet/sfc/efx_channels.c
+++ b/drivers/net/ethernet/sfc/efx_channels.c
@@ -780,6 +780,7 @@ static inline int efx_alloc_xdp_tx_queues(struct efx_nic *efx)
 	return 0;
 }
 
+/* Assign a tx queue to one CPU for XDP_TX action */
 static int efx_set_xdp_tx_queue(struct efx_nic *efx, int xdp_queue_number,
 				struct efx_tx_queue *tx_queue)
 {
@@ -794,7 +795,11 @@ static int efx_set_xdp_tx_queue(struct efx_nic *efx, int xdp_queue_number,
 	return 0;
 }
 
-static void efx_set_xdp_channels(struct efx_nic *efx)
+/* Create the cpu to tx_queue mappings for XDP_TX. Depending on the value of
+ * efx->xdp_txq_queues_mode, it may use dedicated XDP channels or shared queues
+ * with netdev core
+ */
+static void efx_set_xdp_tx_queues(struct efx_nic *efx)
 {
 	struct efx_tx_queue *tx_queue;
 	struct efx_channel *channel;
@@ -915,7 +920,7 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
 		efx_init_napi_channel(efx->channel[i]);
 	}
 
-	efx_set_xdp_channels(efx);
+	efx_set_xdp_tx_queues(efx);
 out:
 	/* Destroy unused channel structures */
 	for (i = 0; i < efx->n_channels; i++) {
@@ -948,7 +953,10 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
 	goto out;
 }
 
-int efx_set_channels(struct efx_nic *efx)
+/* Assign hw queues to each RX and TX queue, create the cpu->tx_queue mapping
+ * for XDP_TX and register the RX and TX queues with netdev core
+ */
+int efx_set_queues(struct efx_nic *efx)
 {
 	struct efx_tx_queue *tx_queue;
 	struct efx_channel *channel;
@@ -982,7 +990,7 @@ int efx_set_channels(struct efx_nic *efx)
 	rc = efx_alloc_xdp_tx_queues(efx);
 	if (rc)
 		return rc;
-	efx_set_xdp_channels(efx);
+	efx_set_xdp_tx_queues(efx);
 
 	rc = netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
 	if (rc)
diff --git a/drivers/net/ethernet/sfc/efx_channels.h b/drivers/net/ethernet/sfc/efx_channels.h
index 64abb99a56b8..148e22415b05 100644
--- a/drivers/net/ethernet/sfc/efx_channels.h
+++ b/drivers/net/ethernet/sfc/efx_channels.h
@@ -35,7 +35,7 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries);
 void efx_set_channel_names(struct efx_nic *efx);
 int efx_init_channels(struct efx_nic *efx);
 int efx_probe_channels(struct efx_nic *efx);
-int efx_set_channels(struct efx_nic *efx);
+int efx_set_queues(struct efx_nic *efx);
 void efx_remove_channel(struct efx_channel *channel);
 void efx_remove_channels(struct efx_nic *efx);
 void efx_fini_channels(struct efx_nic *efx);
-- 
2.34.1

[PATCH net-next 4/5] sfc: refactor efx_set_xdp_tx_queues

[Íñigo Huguet]

Refactor this code to make easier to follow what's going on there and to
show the intent of the code more clearly.

No functional changes.

Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/efx_channels.c | 65 ++++++++++---------------
 1 file changed, 27 insertions(+), 38 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
index 1c05063a7215..f6634faa1ec4 100644
--- a/drivers/net/ethernet/sfc/efx_channels.c
+++ b/drivers/net/ethernet/sfc/efx_channels.c
@@ -781,17 +781,18 @@ static inline int efx_alloc_xdp_tx_queues(struct efx_nic *efx)
 }
 
 /* Assign a tx queue to one CPU for XDP_TX action */
-static int efx_set_xdp_tx_queue(struct efx_nic *efx, int xdp_queue_number,
-				struct efx_tx_queue *tx_queue)
+static inline int efx_set_xdp_tx_queue(struct efx_nic *efx, int cpu,
+				       struct efx_tx_queue *tx_queue)
 {
-	if (xdp_queue_number >= efx->xdp_tx_queue_count)
+	if (cpu >= efx->xdp_tx_queue_count)
 		return -EINVAL;
 
 	netif_dbg(efx, drv, efx->net_dev,
 		  "Channel %u TXQ %u is XDP %u, HW %u\n",
 		  tx_queue->channel->channel, tx_queue->label,
-		  xdp_queue_number, tx_queue->queue);
-	efx->xdp_tx_queues[xdp_queue_number] = tx_queue;
+		  cpu, tx_queue->queue);
+
+	efx->xdp_tx_queues[cpu] = tx_queue;
 	return 0;
 }
 
@@ -803,49 +804,37 @@ static void efx_set_xdp_tx_queues(struct efx_nic *efx)
 {
 	struct efx_tx_queue *tx_queue;
 	struct efx_channel *channel;
-	unsigned int next_queue = 0;
-	int xdp_queue_number = 0;
-	int rc;
-
-	efx_for_each_channel(channel, efx) {
-		if (channel->channel < efx->tx_channel_offset)
-			continue;
-
-		if (efx_channel_is_xdp_tx(channel)) {
+	unsigned int queue_num, cpu;
+
+	cpu = 0;
+	if (efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_BORROWED) {
+		efx_for_each_tx_channel(channel, efx) {
+			/* borrow first channel's queue, with no csum offload */
+			if (efx_set_xdp_tx_queue(efx, cpu, &channel->tx_queue[0]) == 0)
+				cpu++;
+		}
+	} else {
+		efx_for_each_xdp_channel(channel, efx) {
 			efx_for_each_channel_tx_queue(tx_queue, channel) {
-				rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
-							  tx_queue);
-				if (rc == 0)
-					xdp_queue_number++;
+				if (efx_set_xdp_tx_queue(efx, cpu, tx_queue) == 0)
+					cpu++;
 			}
-		} 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)
-			 */
-			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 &&
-		xdp_queue_number != efx->xdp_tx_queue_count);
+		cpu != efx->xdp_tx_queue_count);
 	WARN_ON(efx->xdp_txq_queues_mode != EFX_XDP_TX_QUEUES_DEDICATED &&
-		xdp_queue_number > efx->xdp_tx_queue_count);
+		cpu > efx->xdp_tx_queue_count);
 
 	/* If we have more CPUs than assigned XDP TX queues, assign the already
 	 * existing queues to the exceeding CPUs
 	 */
-	next_queue = 0;
-	while (xdp_queue_number < efx->xdp_tx_queue_count) {
-		tx_queue = efx->xdp_tx_queues[next_queue++];
-		rc = efx_set_xdp_tx_queue(efx, xdp_queue_number, tx_queue);
-		if (rc == 0)
-			xdp_queue_number++;
+	queue_num = 0;
+	while (cpu < efx->xdp_tx_queue_count) {
+		tx_queue = efx->xdp_tx_queues[queue_num++];
+		if (efx_set_xdp_tx_queue(efx, cpu, tx_queue) == 0)
+			cpu++;
 	}
 }
 
-- 
2.34.1

[PATCH net-next 5/5] sfc: move tx_channel_offset calculation to interrupts probe

[Íñigo Huguet]

All parameters related to what channels are used for RX, TX and/or XDP
are calculated in efx_probe_interrupts or its called function
efx_allocate_msix_channels.

tx_channel_offset was recalculated needlessly in efx_set_queues. Remove
this from here since it's more coherent to calculate it only once, in
the same place than the rest of channels parameters. If MSIX is not used,
this value was not set in efx_probe_interrupts, so let's do it now.

The value calculated in efx_set_queues was wrong anyway, because with
the addition of the support for XDP, additional channels had been added
after the TX channels, and efx->n_channels - efx->n_tx_channels didn't
point to the beginning of the TX channels any more.

Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/efx_channels.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
index f6634faa1ec4..b9bbef07bb5e 100644
--- a/drivers/net/ethernet/sfc/efx_channels.c
+++ b/drivers/net/ethernet/sfc/efx_channels.c
@@ -220,14 +220,9 @@ static int efx_allocate_msix_channels(struct efx_nic *efx,
 	n_channels -= efx->n_xdp_channels;
 
 	if (efx_separate_tx_channels) {
-		efx->n_tx_channels =
-			min(max(n_channels / 2, 1U),
-			    efx->max_tx_channels);
-		efx->tx_channel_offset =
-			n_channels - efx->n_tx_channels;
-		efx->n_rx_channels =
-			max(n_channels -
-			    efx->n_tx_channels, 1U);
+		efx->n_tx_channels = min(max(n_channels / 2, 1U), efx->max_tx_channels);
+		efx->tx_channel_offset = n_channels - efx->n_tx_channels;
+		efx->n_rx_channels = max(n_channels - efx->n_tx_channels, 1U);
 	} else {
 		efx->n_tx_channels = min(n_channels, efx->max_tx_channels);
 		efx->tx_channel_offset = 0;
@@ -303,6 +298,7 @@ int efx_probe_interrupts(struct efx_nic *efx)
 		efx->n_channels = 1;
 		efx->n_rx_channels = 1;
 		efx->n_tx_channels = 1;
+		efx->tx_channel_offset = 0;
 		efx->n_xdp_channels = 0;
 		efx->xdp_channel_offset = efx->n_channels;
 		rc = pci_enable_msi(efx->pci_dev);
@@ -323,6 +319,7 @@ int efx_probe_interrupts(struct efx_nic *efx)
 		efx->n_channels = 1 + (efx_separate_tx_channels ? 1 : 0);
 		efx->n_rx_channels = 1;
 		efx->n_tx_channels = 1;
+		efx->tx_channel_offset = 1;
 		efx->n_xdp_channels = 0;
 		efx->xdp_channel_offset = efx->n_channels;
 		efx->legacy_irq = efx->pci_dev->irq;
@@ -952,10 +949,6 @@ int efx_set_queues(struct efx_nic *efx)
 	unsigned int queue_num = 0;
 	int rc;
 
-	efx->tx_channel_offset =
-		efx_separate_tx_channels ?
-		efx->n_channels - efx->n_tx_channels : 0;
-
 	/* 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.
 	 */
-- 
2.34.1

Re: [PATCH net-next 5/5] sfc: move tx_channel_offset calculation to interrupts probe

[Martin Habets]

On Tue, May 10, 2022 at 10:44:43AM +0200, Íñigo Huguet wrote:
> All parameters related to what channels are used for RX, TX and/or XDP
> are calculated in efx_probe_interrupts or its called function
> efx_allocate_msix_channels.
> 
> tx_channel_offset was recalculated needlessly in efx_set_queues. Remove
> this from here since it's more coherent to calculate it only once, in
> the same place than the rest of channels parameters. If MSIX is not used,
> this value was not set in efx_probe_interrupts, so let's do it now.
> 
> The value calculated in efx_set_queues was wrong anyway, because with
> the addition of the support for XDP, additional channels had been added
> after the TX channels, and efx->n_channels - efx->n_tx_channels didn't
> point to the beginning of the TX channels any more.

Apart from the reformatting this fixes a bug in the existing code.
Please submit this bug fix part again as an individual patch.

Martin

> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> ---
>  drivers/net/ethernet/sfc/efx_channels.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> index f6634faa1ec4..b9bbef07bb5e 100644
> --- a/drivers/net/ethernet/sfc/efx_channels.c
> +++ b/drivers/net/ethernet/sfc/efx_channels.c
> @@ -220,14 +220,9 @@ static int efx_allocate_msix_channels(struct efx_nic *efx,
>  	n_channels -= efx->n_xdp_channels;
>  
>  	if (efx_separate_tx_channels) {
> -		efx->n_tx_channels =
> -			min(max(n_channels / 2, 1U),
> -			    efx->max_tx_channels);
> -		efx->tx_channel_offset =
> -			n_channels - efx->n_tx_channels;
> -		efx->n_rx_channels =
> -			max(n_channels -
> -			    efx->n_tx_channels, 1U);
> +		efx->n_tx_channels = min(max(n_channels / 2, 1U), efx->max_tx_channels);
> +		efx->tx_channel_offset = n_channels - efx->n_tx_channels;
> +		efx->n_rx_channels = max(n_channels - efx->n_tx_channels, 1U);
>  	} else {
>  		efx->n_tx_channels = min(n_channels, efx->max_tx_channels);
>  		efx->tx_channel_offset = 0;
> @@ -303,6 +298,7 @@ int efx_probe_interrupts(struct efx_nic *efx)
>  		efx->n_channels = 1;
>  		efx->n_rx_channels = 1;
>  		efx->n_tx_channels = 1;
> +		efx->tx_channel_offset = 0;
>  		efx->n_xdp_channels = 0;
>  		efx->xdp_channel_offset = efx->n_channels;
>  		rc = pci_enable_msi(efx->pci_dev);
> @@ -323,6 +319,7 @@ int efx_probe_interrupts(struct efx_nic *efx)
>  		efx->n_channels = 1 + (efx_separate_tx_channels ? 1 : 0);
>  		efx->n_rx_channels = 1;
>  		efx->n_tx_channels = 1;
> +		efx->tx_channel_offset = 1;
>  		efx->n_xdp_channels = 0;
>  		efx->xdp_channel_offset = efx->n_channels;
>  		efx->legacy_irq = efx->pci_dev->irq;
> @@ -952,10 +949,6 @@ int efx_set_queues(struct efx_nic *efx)
>  	unsigned int queue_num = 0;
>  	int rc;
>  
> -	efx->tx_channel_offset =
> -		efx_separate_tx_channels ?
> -		efx->n_channels - efx->n_tx_channels : 0;
> -
>  	/* 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.
>  	 */
> -- 
> 2.34.1