Re: [PATCH net-next 08/12] amd-xgbe: Add ethtool show/set channels support

[Tom Lendacky]

On 5/22/2018 8:29 AM, Edward Cree wrote:
> On 22/05/18 14:24, Tom Lendacky wrote:
>> The amd-xgbe driver is not designed to allocate separate IRQs for Rx and
>> Tx.  In general, there is one IRQ for a channel of which Tx and Rx are
>> shared.  You can have more Tx channels than Rx channels or vice-versa, but
>> the min() of those numbers will be a combined Tx/Rx with the excess being
>> Tx or Rx only: e.g. combined 0 tx 8 rx 10 results in 8 combined channels
>> plus two Rx only channels.
> If you cannot allocate the channels requested by the user, surely the correct
>  thing is not to fudge it into something similar, but rather to return an
>  error from the ethtool set_channels() op.

Ok, another vote on changing the logic.  I'll update it and submit a v2.

Thanks,
Tom

> 
> -Ed
> 

Re: [PATCH net-next 08/12] amd-xgbe: Add ethtool show/set channels support

[Edward Cree]

On 22/05/18 14:24, Tom Lendacky wrote:
> The amd-xgbe driver is not designed to allocate separate IRQs for Rx and
> Tx.  In general, there is one IRQ for a channel of which Tx and Rx are
> shared.  You can have more Tx channels than Rx channels or vice-versa, but
> the min() of those numbers will be a combined Tx/Rx with the excess being
> Tx or Rx only: e.g. combined 0 tx 8 rx 10 results in 8 combined channels
> plus two Rx only channels.
If you cannot allocate the channels requested by the user, surely the correct
 thing is not to fudge it into something similar, but rather to return an
 error from the ethtool set_channels() op.

-Ed

Re: [PATCH net-next 08/12] amd-xgbe: Add ethtool show/set channels support

[Tom Lendacky]

On 5/22/2018 12:35 AM, Jakub Kicinski wrote:
> On Mon, 21 May 2018 16:59:37 -0500, Tom Lendacky wrote:
>> +rx = combined + channels->rx_count;
>> +tx = combined + channels->tx_count;
>> +netdev_notice(netdev, "final channel count assignment: combined=%u, 
>> rx-only=%u, tx-only=%u\n",
>> +  min(rx, tx), rx - min(rx, tx), tx - min(rx, tx));
> 
> If user requests combined 0 rx 8 tx 8 they will end up with combined 8
> rx 0 tx 0.  Is that expected?

Yes, which is the reason that I issue the final channel count message. I
debated on how to do all this and looked at other drivers as well as the
ethtool man page and decided on this logic.

> 
> The man page clearly sayeth:
> 
>-L --set-channels
>   Changes the numbers of channels of the specified network device.
> 
>rx N   Changes the number of channels with only receive queues.
> 
>tx N   Changes the number of channels with only transmit queues.
> 
>other N
>   Changes the number of channels used only for other  purposes
>   e.g. link interrupts or SR-IOV co-ordination.
> 
>combined N
>   Changes the number of multi-purpose channels.
> 
> Note the use of word *only*.  There are drivers in tree which adhere to
> this interpretation and dutifully allocate separate IRQs if RX and TX
> channels are requested separately.

The amd-xgbe driver is not designed to allocate separate IRQs for Rx and
Tx.  In general, there is one IRQ for a channel of which Tx and Rx are
shared.  You can have more Tx channels than Rx channels or vice-versa, but
the min() of those numbers will be a combined Tx/Rx with the excess being
Tx or Rx only: e.g. combined 0 tx 8 rx 10 results in 8 combined channels
plus two Rx only channels.

I thought this was the most reasonable way to do this, please let me know
if there's a strong objection to this.

Thanks,
Tom

> 
> Which is not to claim that majority of existing drivers adhere to this
> wording :)
> 

Re: [PATCH net-next 08/12] amd-xgbe: Add ethtool show/set channels support

[Jakub Kicinski]

On Mon, 21 May 2018 16:59:37 -0500, Tom Lendacky wrote:
> + rx = combined + channels->rx_count;
> + tx = combined + channels->tx_count;
> + netdev_notice(netdev, "final channel count assignment: combined=%u, 
> rx-only=%u, tx-only=%u\n",
> +   min(rx, tx), rx - min(rx, tx), tx - min(rx, tx));

If user requests combined 0 rx 8 tx 8 they will end up with combined 8
rx 0 tx 0.  Is that expected?

The man page clearly sayeth:

   -L --set-channels
  Changes the numbers of channels of the specified network device.

   rx N   Changes the number of channels with only receive queues.

   tx N   Changes the number of channels with only transmit queues.

   other N
  Changes the number of channels used only for other  purposes
  e.g. link interrupts or SR-IOV co-ordination.

   combined N
  Changes the number of multi-purpose channels.

Note the use of word *only*.  There are drivers in tree which adhere to
this interpretation and dutifully allocate separate IRQs if RX and TX
channels are requested separately.

Which is not to claim that majority of existing drivers adhere to this
wording :)

[PATCH net-next 08/12] amd-xgbe: Add ethtool show/set channels support

[Tom Lendacky]

Add ethtool support to show and set the device channel configuration.
Changing the channel configuration will result in a device restart.

Signed-off-by: Tom Lendacky 
---
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c |   25 +
 drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c |  131 ++
 drivers/net/ethernet/amd/xgbe/xgbe.h |4 +
 3 files changed, 160 insertions(+)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c 
b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 397e3a0..24f1053 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -1329,6 +1329,17 @@ static int xgbe_alloc_memory(struct xgbe_prv_data *pdata)
struct net_device *netdev = pdata->netdev;
int ret;
 
+   if (pdata->new_tx_ring_count) {
+   pdata->tx_ring_count = pdata->new_tx_ring_count;
+   pdata->tx_q_count = pdata->tx_ring_count;
+   pdata->new_tx_ring_count = 0;
+   }
+
+   if (pdata->new_rx_ring_count) {
+   pdata->rx_ring_count = pdata->new_rx_ring_count;
+   pdata->new_rx_ring_count = 0;
+   }
+
/* Calculate the Rx buffer size before allocating rings */
pdata->rx_buf_size = xgbe_calc_rx_buf_size(netdev, netdev->mtu);
 
@@ -1482,6 +1493,20 @@ static void xgbe_stopdev(struct work_struct *work)
netdev_alert(pdata->netdev, "device stopped\n");
 }
 
+void xgbe_full_restart_dev(struct xgbe_prv_data *pdata)
+{
+   /* If not running, "restart" will happen on open */
+   if (!netif_running(pdata->netdev))
+   return;
+
+   xgbe_stop(pdata);
+
+   xgbe_free_memory(pdata);
+   xgbe_alloc_memory(pdata);
+
+   xgbe_start(pdata);
+}
+
 void xgbe_restart_dev(struct xgbe_prv_data *pdata)
 {
/* If not running, "restart" will happen on open */
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c 
b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
index d12f982..d26fd95 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
@@ -705,6 +705,135 @@ static int xgbe_set_ringparam(struct net_device *netdev,
return 0;
 }
 
+static void xgbe_get_channels(struct net_device *netdev,
+ struct ethtool_channels *channels)
+{
+   struct xgbe_prv_data *pdata = netdev_priv(netdev);
+   unsigned int rx, tx, combined;
+
+   /* Calculate maximums allowed:
+*   - Take into account the number of available IRQs
+*   - Do not take into account the number of online CPUs so that
+* the user can over-subscribe if desired
+*   - Tx is additionally limited by the number of hardware queues
+*/
+   rx = min(pdata->hw_feat.rx_ch_cnt, pdata->rx_max_channel_count);
+   rx = min(rx, pdata->channel_irq_count);
+   tx = min(pdata->hw_feat.tx_ch_cnt, pdata->tx_max_channel_count);
+   tx = min(tx, pdata->channel_irq_count);
+   tx = min(tx, pdata->tx_max_q_count);
+
+   combined = min(rx, tx);
+
+   channels->max_combined = combined;
+   channels->max_rx = rx;
+   channels->max_tx = tx;
+
+   /* Current running settings */
+   rx = pdata->rx_ring_count;
+   tx = pdata->tx_ring_count;
+
+   combined = min(rx, tx);
+   rx -= combined;
+   tx -= combined;
+
+   channels->combined_count = combined;
+   channels->rx_count = rx;
+   channels->tx_count = tx;
+}
+
+static void xgbe_print_set_channels_input(struct net_device *netdev,
+ struct ethtool_channels *channels)
+{
+   netdev_err(netdev, "channel inputs: combined=%u, rx-only=%u, 
tx-only=%u\n",
+  channels->combined_count, channels->rx_count,
+  channels->tx_count);
+}
+
+static int xgbe_set_channels(struct net_device *netdev,
+struct ethtool_channels *channels)
+{
+   struct xgbe_prv_data *pdata = netdev_priv(netdev);
+   unsigned int rx, tx, combined;
+
+   /* Calculate maximums allowed:
+*   - Take into account the number of available IRQs
+*   - Do not take into account the number of online CPUs so that
+* the user can over-subscribe if desired
+*   - Tx is additionally limited by the number of hardware queues
+*/
+   rx = min(pdata->hw_feat.rx_ch_cnt, pdata->rx_max_channel_count);
+   rx = min(rx, pdata->channel_irq_count);
+   tx = min(pdata->hw_feat.tx_ch_cnt, pdata->tx_max_channel_count);
+   tx = min(tx, pdata->tx_max_q_count);
+   tx = min(tx, pdata->channel_irq_count);
+
+   combined = min(rx, tx);
+
+   /* Should not be setting other count */
+   if (channels->other_count) {
+   netdev_err(netdev,
+  "other channel count must be zero\n");
+   return -EINVAL;
+   }
+
+   /* Require at least one Rx and Tx