| Message ID | 20130110185846.29578.94872.stgit@ahduyck-cp1.jf.intel.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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
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.
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
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
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);
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