Patchwork [RFC,v2,10/10] ixgbe: Add support for set_channels ethtool operation

login
register
mail settings
Submitter Alexander Duyck
Date Jan. 10, 2013, 6:58 p.m.
Message ID <20130110185846.29578.94872.stgit@ahduyck-cp1.jf.intel.com>
Download mbox | patch
Permalink /patch/211120/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Alexander Duyck - Jan. 10, 2013, 6:58 p.m.
This change adds support for the ethtool set_channels operation.

Since the ixgbe driver has to support DCB as well as the other modes the
assumption I made here is that the number of channels in DCB modes refers
to the number of queues per traffic class, not the number of queues total.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |    2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |   40 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |    7 +++-
 3 files changed, 46 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Jan. 16, 2013, 4:19 p.m.
On Thu, 2013-01-10 at 10:58 -0800, Alexander Duyck wrote:
> This change adds support for the ethtool set_channels operation.
> 
> Since the ixgbe driver has to support DCB as well as the other modes the
> assumption I made here is that the number of channels in DCB modes refers
> to the number of queues per traffic class, not the number of queues total.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

In DCB mode are there separate IRQs for the different classes?

[...]
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -2775,6 +2775,45 @@ static void ixgbe_get_channels(struct net_device *dev,
>  	ch->combined_count = adapter->ring_feature[RING_F_FDIR].indices;
>  }
>  
> +static int ixgbe_set_channels(struct net_device *dev,
> +			      struct ethtool_channels *ch)
> +{
> +	struct ixgbe_adapter *adapter = netdev_priv(dev);
> +	unsigned int count = ch->combined_count;
> +
> +	/* verify they are not requesting separate vectors */
> +	if (ch->rx_count || ch->tx_count)
> +		return -EINVAL;
> +
> +	/* ignore other_count since it is not changeable */
[...]

Please do return an error if the command specifies a change to
other_count.

Ben.
Waskiewicz Jr, Peter P - Jan. 16, 2013, 4:30 p.m.
On Wed, 2013-01-16 at 16:19 +0000, Ben Hutchings wrote:
> On Thu, 2013-01-10 at 10:58 -0800, Alexander Duyck wrote:

> > This change adds support for the ethtool set_channels operation.

> > 

> > Since the ixgbe driver has to support DCB as well as the other modes the

> > assumption I made here is that the number of channels in DCB modes refers

> > to the number of queues per traffic class, not the number of queues total.

> > 

> > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

> 

> In DCB mode are there separate IRQs for the different classes?


Yes.  The Rx packet buffer is split into multiple packet buffers, one
for each online class.  After that, it's just queues assigned to the
packet buffers, and interrupts assigned however you want them to be.

Cheers,
-PJ
Alexander Duyck - Jan. 17, 2013, 12:35 a.m.
On 01/16/2013 08:19 AM, Ben Hutchings wrote:
> On Thu, 2013-01-10 at 10:58 -0800, Alexander Duyck wrote:
>> This change adds support for the ethtool set_channels operation.
>>
>> Since the ixgbe driver has to support DCB as well as the other modes the
>> assumption I made here is that the number of channels in DCB modes refers
>> to the number of queues per traffic class, not the number of queues total.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> In DCB mode are there separate IRQs for the different classes?
> 
> [...]
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> @@ -2775,6 +2775,45 @@ static void ixgbe_get_channels(struct net_device *dev,
>>  	ch->combined_count = adapter->ring_feature[RING_F_FDIR].indices;
>>  }
>>  
>> +static int ixgbe_set_channels(struct net_device *dev,
>> +			      struct ethtool_channels *ch)
>> +{
>> +	struct ixgbe_adapter *adapter = netdev_priv(dev);
>> +	unsigned int count = ch->combined_count;
>> +
>> +	/* verify they are not requesting separate vectors */
>> +	if (ch->rx_count || ch->tx_count)
>> +		return -EINVAL;
>> +
>> +	/* ignore other_count since it is not changeable */
> [...]
> 
> Please do return an error if the command specifies a change to
> other_count.
> 
> Ben.
> 

I will update the patch to return an error if other_count is not equal
to NON_Q_VECTORS.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Jan. 23, 2013, 9:20 p.m.
On Wed, 2013-01-16 at 16:30 +0000, Waskiewicz Jr, Peter P wrote:
> On Wed, 2013-01-16 at 16:19 +0000, Ben Hutchings wrote:
> > On Thu, 2013-01-10 at 10:58 -0800, Alexander Duyck wrote:
> > > This change adds support for the ethtool set_channels operation.
> > > 
> > > Since the ixgbe driver has to support DCB as well as the other modes the
> > > assumption I made here is that the number of channels in DCB modes refers
> > > to the number of queues per traffic class, not the number of queues total.
> > > 
> > > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> > 
> > In DCB mode are there separate IRQs for the different classes?
> 
> Yes.  The Rx packet buffer is split into multiple packet buffers, one
> for each online class.  After that, it's just queues assigned to the
> packet buffers, and interrupts assigned however you want them to be.

Right, I think we've been through this before.  And I can see how it
would be more useful for users to specify number of RX queues per
priority level.  But that's not what was specified...

I'm afraid the 'channels' ethtool operations have turned into a mess...
I can't see how to get to a reasonable generic definition of what they
should do.

Ben.
Alexander Duyck - Jan. 23, 2013, 10:31 p.m.
On 01/23/2013 01:20 PM, Ben Hutchings wrote:
> On Wed, 2013-01-16 at 16:30 +0000, Waskiewicz Jr, Peter P wrote:
>> On Wed, 2013-01-16 at 16:19 +0000, Ben Hutchings wrote:
>>> On Thu, 2013-01-10 at 10:58 -0800, Alexander Duyck wrote:
>>>> This change adds support for the ethtool set_channels operation.
>>>>
>>>> Since the ixgbe driver has to support DCB as well as the other modes the
>>>> assumption I made here is that the number of channels in DCB modes refers
>>>> to the number of queues per traffic class, not the number of queues total.
>>>>
>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>> In DCB mode are there separate IRQs for the different classes?
>> Yes.  The Rx packet buffer is split into multiple packet buffers, one
>> for each online class.  After that, it's just queues assigned to the
>> packet buffers, and interrupts assigned however you want them to be.
> Right, I think we've been through this before.  And I can see how it
> would be more useful for users to specify number of RX queues per
> priority level.  But that's not what was specified...
>
> I'm afraid the 'channels' ethtool operations have turned into a mess...
> I can't see how to get to a reasonable generic definition of what they
> should do.
>
> Ben.

Actually it looks like most of the drivers (I looked at bnx, bnx2x, tg3,
and qlcnic) are using the set_queues call in a similar way.  What they
end up doing is using the value and plugging it into their TSS/RSS
fields in their private structures.  From what I can tell in
bnx2x_setup_tc they may do exactly the same thing we are currently doing
for DCB since they use the BNX2X_NUM_ETH_QUEUES value that they set in
their set_channels call to set the number of queues they use per traffic
class.  I would say the usage is actually pretty consistent between
bnx2x and ixgbe based on this, even if it isn't exactly correct.

For now I would say all of the drivers are using the set_channels
operation to specify the number of Tx/Rx queues, or queue pairs per
traffic class.  So for non-DCB NICs this means it is setting exactly
that number of queues, and for DCB capable nics it means num_tcs times
the specified number of queues.

Thanks,

Alex








--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Fastabend - Jan. 23, 2013, 11:48 p.m.
On 1/23/2013 2:31 PM, Alexander Duyck wrote:
> On 01/23/2013 01:20 PM, Ben Hutchings wrote:
>> On Wed, 2013-01-16 at 16:30 +0000, Waskiewicz Jr, Peter P wrote:
>>> On Wed, 2013-01-16 at 16:19 +0000, Ben Hutchings wrote:
>>>> On Thu, 2013-01-10 at 10:58 -0800, Alexander Duyck wrote:
>>>>> This change adds support for the ethtool set_channels operation.
>>>>>
>>>>> Since the ixgbe driver has to support DCB as well as the other modes the
>>>>> assumption I made here is that the number of channels in DCB modes refers
>>>>> to the number of queues per traffic class, not the number of queues total.
>>>>>
>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>>> In DCB mode are there separate IRQs for the different classes?
>>> Yes.  The Rx packet buffer is split into multiple packet buffers, one
>>> for each online class.  After that, it's just queues assigned to the
>>> packet buffers, and interrupts assigned however you want them to be.
>> Right, I think we've been through this before.  And I can see how it
>> would be more useful for users to specify number of RX queues per
>> priority level.  But that's not what was specified...
>>
>> I'm afraid the 'channels' ethtool operations have turned into a mess...
>> I can't see how to get to a reasonable generic definition of what they
>> should do.
>>
>> Ben.
>
> Actually it looks like most of the drivers (I looked at bnx, bnx2x, tg3,
> and qlcnic) are using the set_queues call in a similar way.  What they
> end up doing is using the value and plugging it into their TSS/RSS
> fields in their private structures.  From what I can tell in
> bnx2x_setup_tc they may do exactly the same thing we are currently doing
> for DCB since they use the BNX2X_NUM_ETH_QUEUES value that they set in
> their set_channels call to set the number of queues they use per traffic
> class.  I would say the usage is actually pretty consistent between
> bnx2x and ixgbe based on this, even if it isn't exactly correct.
>
> For now I would say all of the drivers are using the set_channels
> operation to specify the number of Tx/Rx queues, or queue pairs per
> traffic class.  So for non-DCB NICs this means it is setting exactly
> that number of queues, and for DCB capable nics it means num_tcs times
> the specified number of queues.
>
> Thanks,
>
> Alex
>

Would it perhaps be cleaner to let set_channels set the absolute
number of tx/rx queue pairs regardless of number of tcs. Then
you could use the mqprio interface to divide those queues into
classes.

struct tc_mqprio_qopt {
         __u8    num_tc;
         __u8    prio_tc_map[TC_QOPT_BITMASK + 1];
         __u8    hw;
         __u16   count[TC_QOPT_MAX_QUEUE];
         __u16   offset[TC_QOPT_MAX_QUEUE];
};

The ndo_setup_tc op could pass the tc_mqprio_qopt structure to the
driver instead of just the number of tcs. This would allow changing
the number of queues per class depending on the traffic type expected
and set_channels then is consistent regardless of number of TCs.

Thanks,
John
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 590f042..a431628 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -695,8 +695,8 @@  extern bool ixgbe_verify_lesm_fw_enabled_82599(struct ixgbe_hw *hw);
 extern void ixgbe_set_rx_mode(struct net_device *netdev);
 #ifdef CONFIG_IXGBE_DCB
 extern void ixgbe_set_rx_drop_en(struct ixgbe_adapter *adapter);
-extern int ixgbe_setup_tc(struct net_device *dev, u8 tc);
 #endif
+extern int ixgbe_setup_tc(struct net_device *dev, u8 tc);
 extern void ixgbe_tx_ctxtdesc(struct ixgbe_ring *, u32, u32, u32, u32);
 extern void ixgbe_do_reset(struct net_device *netdev);
 #ifdef CONFIG_IXGBE_HWMON
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 688effc..31998a3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -2775,6 +2775,45 @@  static void ixgbe_get_channels(struct net_device *dev,
 	ch->combined_count = adapter->ring_feature[RING_F_FDIR].indices;
 }
 
+static int ixgbe_set_channels(struct net_device *dev,
+			      struct ethtool_channels *ch)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(dev);
+	unsigned int count = ch->combined_count;
+
+	/* verify they are not requesting separate vectors */
+	if (ch->rx_count || ch->tx_count)
+		return -EINVAL;
+
+	/* ignore other_count since it is not changeable */
+
+	/* verify we have at least one channel requested */
+	if (!count)
+		return -EINVAL;
+
+	/* verify the number of channels does not exceed hardware limits */
+	if (count > ixgbe_max_channels(adapter))
+		return -EINVAL;
+
+	/* update feature limits from largest to smallest supported values */
+	adapter->ring_feature[RING_F_FDIR].limit = count;
+
+	/* cap RSS limit at 16 */
+	if (count > IXGBE_MAX_RSS_INDICES)
+		count = IXGBE_MAX_RSS_INDICES;
+	adapter->ring_feature[RING_F_RSS].limit = count;
+
+#ifdef IXGBE_FCOE
+	/* cap FCoE limit at 8 */
+	if (count > IXGBE_FCRETA_SIZE)
+		count = IXGBE_FCRETA_SIZE;
+	adapter->ring_feature[RING_F_FCOE].limit = count;
+
+#endif
+	/* use setup TC to update any traffic class queue mapping */
+	return ixgbe_setup_tc(dev, netdev_get_num_tc(dev));
+}
+
 static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.get_settings           = ixgbe_get_settings,
 	.set_settings           = ixgbe_set_settings,
@@ -2805,6 +2844,7 @@  static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.set_rxnfc		= ixgbe_set_rxnfc,
 	.get_ts_info		= ixgbe_get_ts_info,
 	.get_channels		= ixgbe_get_channels,
+	.set_channels		= ixgbe_set_channels,
 };
 
 void ixgbe_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 4c0bcd8..798bbf6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6796,6 +6796,7 @@  static void ixgbe_set_prio_tc_map(struct ixgbe_adapter *adapter)
 	}
 }
 
+#endif /* CONFIG_IXGBE_DCB */
 /**
  * ixgbe_setup_tc - configure net_device for multiple traffic classes
  *
@@ -6821,6 +6822,7 @@  int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 		ixgbe_close(dev);
 	ixgbe_clear_interrupt_scheme(adapter);
 
+#ifdef CONFIG_IXGBE_DCB
 	if (tc) {
 		netdev_set_num_tc(dev, tc);
 		ixgbe_set_prio_tc_map(adapter);
@@ -6843,15 +6845,16 @@  int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 		adapter->dcb_cfg.pfc_mode_enable = false;
 	}
 
-	ixgbe_init_interrupt_scheme(adapter);
 	ixgbe_validate_rtr(adapter, tc);
+
+#endif /* CONFIG_IXGBE_DCB */
+	ixgbe_init_interrupt_scheme(adapter);
 	if (netif_running(dev))
 		ixgbe_open(dev);
 
 	return 0;
 }
 
-#endif /* CONFIG_IXGBE_DCB */
 void ixgbe_do_reset(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);