| Message ID | 20230208044536.10961-1-muhammad.husaini.zulkifli@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [net-next,v1] igc: Add transmission overrun counter | expand |
On 2/7/2023 8:45 PM, Muhammad Husaini Zulkifli wrote: > Add transmission overrun counter as per defined by IEEE 802.1Q Bridges. > The Intel Discrete I225/6 does not have HW counters that can determine > whether a packet is still being transmitted after the gate has been closed. > But I225/I226 have a mechanism to not transmit a packets if the gate > open time is insufficient for the packet transmission by setting the > Strict_End bit. Software counters have been created for each queues in > response to the IEEE specification. Thus, we can assume that overrun > counter is always "0" when setting this bit. Can you explain why this is needed? It doesn't seem to make sense to add a driver counter to always report 0. If it's needed for spec or tools, I would think it should be added to something higher like netdev stats, not driver specific stats. > User can get this counter by using below command: > "ethtool -S <interface> | grep qbv_tx_overrun" > > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
HI Tony, > -----Original Message----- > From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Sent: Thursday, 16 February, 2023 3:23 AM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; > intel-wired-lan@osuosl.org; Gomes, Vinicius <vinicius.gomes@intel.com> > Cc: Neftin, Sasha <sasha.neftin@intel.com>; naamax.meir@linux.intel.com > Subject: Re: [PATCH net-next v1] igc: Add transmission overrun counter > > > > On 2/7/2023 8:45 PM, Muhammad Husaini Zulkifli wrote: > > Add transmission overrun counter as per defined by IEEE 802.1Q Bridges. > > The Intel Discrete I225/6 does not have HW counters that can determine > > whether a packet is still being transmitted after the gate has been closed. > > But I225/I226 have a mechanism to not transmit a packets if the gate > > open time is insufficient for the packet transmission by setting the > > Strict_End bit. Software counters have been created for each queues in > > response to the IEEE specification. Thus, we can assume that overrun > > counter is always "0" when setting this bit. > > Can you explain why this is needed? It doesn't seem to make sense to add a > driver counter to always report 0. If it's needed for spec or tools, I would > think it should be added to something higher like netdev stats, not driver > specific stats. We need this as part of AVNU Certification Test Case. The reason why value always reported as zero is because we set the IGC_TXQCTL_STRICT_END bit. I225/6 does not have HW counter to really Know if the packet is outside the qbv windows even though we unset the IGC_TXQCTL_STRICT_END. It much simpler to just add this counter as part of Driver code as of now and pass through the igc_stats where it can be show in Using ethtool command as well. > > > User can get this counter by using below command: > > "ethtool -S <interface> | grep qbv_tx_overrun" > > > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com>
Hi Jakub and Vinicius, I would appreciate any early feedback or thoughts on this patch submission. History of the discussion: https://lore.kernel.org/intel-wired-lan/SJ1PR11MB6180D0D59EB01AD1E8FE4991B8A39@SJ1PR11MB6180.namprd11.prod.outlook.com/T/#r8db595a7b40067d2315def91d41c9695454d6c9f Thanks > -----Original Message----- > From: Zulkifli, Muhammad Husaini > Sent: Thursday, 16 February, 2023 7:41 AM > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; intel-wired- > lan@osuosl.org; Gomes, Vinicius <vinicius.gomes@intel.com> > Cc: Neftin, Sasha <sasha.neftin@intel.com>; naamax.meir@linux.intel.com > Subject: RE: [PATCH net-next v1] igc: Add transmission overrun counter > > HI Tony, > > > -----Original Message----- > > From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > > Sent: Thursday, 16 February, 2023 3:23 AM > > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; > > intel-wired-lan@osuosl.org; Gomes, Vinicius <vinicius.gomes@intel.com> > > Cc: Neftin, Sasha <sasha.neftin@intel.com>; > > naamax.meir@linux.intel.com > > Subject: Re: [PATCH net-next v1] igc: Add transmission overrun counter > > > > > > > > On 2/7/2023 8:45 PM, Muhammad Husaini Zulkifli wrote: > > > Add transmission overrun counter as per defined by IEEE 802.1Q Bridges. > > > The Intel Discrete I225/6 does not have HW counters that can > > > determine whether a packet is still being transmitted after the gate has > been closed. > > > But I225/I226 have a mechanism to not transmit a packets if the gate > > > open time is insufficient for the packet transmission by setting the > > > Strict_End bit. Software counters have been created for each queues > > > in response to the IEEE specification. Thus, we can assume that > > > overrun counter is always "0" when setting this bit. > > > > Can you explain why this is needed? It doesn't seem to make sense to > > add a driver counter to always report 0. If it's needed for spec or > > tools, I would think it should be added to something higher like > > netdev stats, not driver specific stats. > > We need this as part of AVNU Certification Test Case. > The reason why value always reported as zero is because we set the > IGC_TXQCTL_STRICT_END bit. I225/6 does not have HW counter to really > Know if the packet is outside the qbv windows even though we unset the > IGC_TXQCTL_STRICT_END. It much simpler to just add this counter as part of > Driver code as of now and pass through the igc_stats where it can be show > in Using ethtool command as well. > > > > > > User can get this counter by using below command: > > > "ethtool -S <interface> | grep qbv_tx_overrun" > > > > > > Signed-off-by: Muhammad Husaini Zulkifli > > > <muhammad.husaini.zulkifli@intel.com>
On Sat, 18 Feb 2023 06:19:31 +0000 Zulkifli, Muhammad Husaini wrote: > Hi Jakub and Vinicius, > > I would appreciate any early feedback or thoughts on this patch submission. > > History of the discussion: > https://lore.kernel.org/intel-wired-lan/SJ1PR11MB6180D0D59EB01AD1E8FE4991B8A39@SJ1PR11MB6180.namprd11.prod.outlook.com/T/#r8db595a7b40067d2315def91d41c9695454d6c9f Tony asks very good questions. Driver specific counter always reported as 0 would be odd for Linux. The counter is of no practical importance.
On 2/20/2023 2:20 PM, Jakub Kicinski wrote: > On Sat, 18 Feb 2023 06:19:31 +0000 Zulkifli, Muhammad Husaini wrote: >> Hi Jakub and Vinicius, >> >> I would appreciate any early feedback or thoughts on this patch submission. >> >> History of the discussion: >> https://lore.kernel.org/intel-wired-lan/SJ1PR11MB6180D0D59EB01AD1E8FE4991B8A39@SJ1PR11MB6180.namprd11.prod.outlook.com/T/#r8db595a7b40067d2315def91d41c9695454d6c9f > > Tony asks very good questions. Driver specific counter always reported > as 0 would be odd for Linux. The counter is of no practical importance. Maybe too obvious a question, but it looks like your patch, Muhammad, forgot to add the code that increments the counter? If you add that, then the patch makes sense.
On 2/21/2023 3:11 PM, Jesse Brandeburg wrote: > On 2/20/2023 2:20 PM, Jakub Kicinski wrote: >> On Sat, 18 Feb 2023 06:19:31 +0000 Zulkifli, Muhammad Husaini wrote: >>> Hi Jakub and Vinicius, >>> >>> I would appreciate any early feedback or thoughts on this patch submission. >>> >>> History of the discussion: >>> https://lore.kernel.org/intel-wired-lan/SJ1PR11MB6180D0D59EB01AD1E8FE4991B8A39@SJ1PR11MB6180.namprd11.prod.outlook.com/T/#r8db595a7b40067d2315def91d41c9695454d6c9f >> >> Tony asks very good questions. Driver specific counter always reported >> as 0 would be odd for Linux. The counter is of no practical importance. > > Maybe too obvious a question, but it looks like your patch, Muhammad, > forgot to add the code that increments the counter? > > If you add that, then the patch makes sense. Oops! it was pointed out to me that your patch had some comment in the middle of the code saying that you'll never increment this value. But I still don't get it, since your commit message said: > Add transmission overrun counter as per defined by IEEE 802.1Q Bridges. > The Intel Discrete I225/6 does not have HW counters that can determine > whether a packet is still being transmitted after the gate has been closed. > But I225/I226 have a mechanism to not transmit a packets if the gate > open time is insufficient for the packet transmission by setting the > Strict_End bit. Software counters have been created for each queues in > response to the IEEE specification. Thus, we can assume that overrun > counter is always "0" when setting this bit. Generally if a counter isn't reported it's presumed to be zero. I think you need to do a better job explaining why userspace needs an "always zero" counter, and can't just be told that the counter is not there (so is obviously zero) and if this app requires that every vendor implement a per-queue stat to track this item, the code to track the stat may as well be added to core kernel instead of for each driver independently.
Hello, > -----Original Message----- > From: Brandeburg, Jesse <jesse.brandeburg@intel.com> > Sent: Wednesday, 22 February, 2023 7:58 AM > To: Jakub Kicinski <kuba@kernel.org>; Zulkifli, Muhammad Husaini > <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Nguyen, Anthony L > <anthony.l.nguyen@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH net-next v1] igc: Add transmission > overrun counter > > On 2/21/2023 3:11 PM, Jesse Brandeburg wrote: > > On 2/20/2023 2:20 PM, Jakub Kicinski wrote: > >> On Sat, 18 Feb 2023 06:19:31 +0000 Zulkifli, Muhammad Husaini wrote: > >>> Hi Jakub and Vinicius, > >>> > >>> I would appreciate any early feedback or thoughts on this patch > submission. > >>> > >>> History of the discussion: > >>> https://lore.kernel.org/intel-wired- > lan/SJ1PR11MB6180D0D59EB01AD1E8F > >>> > E4991B8A39@SJ1PR11MB6180.namprd11.prod.outlook.com/T/#r8db595a7b > 4006 > >>> 7d2315def91d41c9695454d6c9f > >> > >> Tony asks very good questions. Driver specific counter always > >> reported as 0 would be odd for Linux. The counter is of no practical > importance. > > > > Maybe too obvious a question, but it looks like your patch, Muhammad, > > forgot to add the code that increments the counter? > > > > If you add that, then the patch makes sense. > > Oops! it was pointed out to me that your patch had some comment in the > middle of the code saying that you'll never increment this value. > > But I still don't get it, since your commit message said: > > > Add transmission overrun counter as per defined by IEEE 802.1Q Bridges. > > The Intel Discrete I225/6 does not have HW counters that can determine > > whether a packet is still being transmitted after the gate has been closed. > > But I225/I226 have a mechanism to not transmit a packets if the gate > > open time is insufficient for the packet transmission by setting the > > Strict_End bit. Software counters have been created for each queues in > > response to the IEEE specification. Thus, we can assume that overrun > > counter is always "0" when setting this bit. > > Generally if a counter isn't reported it's presumed to be zero. > > I think you need to do a better job explaining why userspace needs an "always > zero" counter, and can't just be told that the counter is not there (so is > obviously zero) AVNu TSN Test certification conformance test required this counter to indicate in the Userapp that whether there is a packets being transmitted on specific queue when the qbv window size is not sufficient. Then for i226 case, our counter will always be "zero" due to the HW mechanism to not transmit the packet if the gate time is not sufficient. > > and if this app requires that every vendor implement a per-queue stat to track > this item, the code to track the stat may as well be added to core kernel > instead of for each driver independently. > We are thinking to have a per-queue stat since it will use by other vendor as well For the certification. I will refactor the code and re submit again for the review.
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 9db93c1f97679..a8c7a978d4335 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -99,6 +99,7 @@ struct igc_ring { u32 start_time; u32 end_time; + u64 qbv_tx_overrun; /* CBS parameters */ bool cbs_enable; /* indicates if CBS is enabled */ diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 0e2cb00622d1a..34a893171b209 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -68,6 +68,10 @@ static const struct igc_stats igc_gstrings_stats[] = { IGC_STAT("tx_lpi_counter", stats.tlpic), IGC_STAT("rx_lpi_counter", stats.rlpic), IGC_STAT("qbv_config_change_errors", qbv_config_change_errors), + IGC_STAT("qbv_tx_overrun_q0", stats.qbv_tx_overrun_q0), + IGC_STAT("qbv_tx_overrun_q1", stats.qbv_tx_overrun_q1), + IGC_STAT("qbv_tx_overrun_q2", stats.qbv_tx_overrun_q2), + IGC_STAT("qbv_tx_overrun_q3", stats.qbv_tx_overrun_q3), }; #define IGC_NETDEV_STAT(_net_stat) { \ diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h index 88680e3d613dd..ce3ba19eef601 100644 --- a/drivers/net/ethernet/intel/igc/igc_hw.h +++ b/drivers/net/ethernet/intel/igc/igc_hw.h @@ -273,6 +273,10 @@ struct igc_hw_stats { u64 o2bspc; u64 b2ospc; u64 b2ogprc; + u64 qbv_tx_overrun_q0; + u64 qbv_tx_overrun_q1; + u64 qbv_tx_overrun_q2; + u64 qbv_tx_overrun_q3; }; struct net_device *igc_get_hw_dev(struct igc_hw *hw); diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 0cc327294dfb5..8b6cdb7d4eff2 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -4926,6 +4926,12 @@ void igc_update_stats(struct igc_adapter *adapter) adapter->stats.mgptc += rd32(IGC_MGTPTC); adapter->stats.mgprc += rd32(IGC_MGTPRC); adapter->stats.mgpdc += rd32(IGC_MGTPDC); + + /* SW overrun counter */ + adapter->stats.qbv_tx_overrun_q0 = adapter->tx_ring[0]->qbv_tx_overrun; + adapter->stats.qbv_tx_overrun_q1 = adapter->tx_ring[1]->qbv_tx_overrun; + adapter->stats.qbv_tx_overrun_q2 = adapter->tx_ring[2]->qbv_tx_overrun; + adapter->stats.qbv_tx_overrun_q3 = adapter->tx_ring[3]->qbv_tx_overrun; } /** @@ -6039,6 +6045,7 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) ring->start_time = 0; ring->end_time = NSEC_PER_SEC; + ring->qbv_tx_overrun = 0; } return 0; diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index b38c1c7569a0b..2593a1517af0a 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -135,6 +135,12 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) txqctl |= IGC_TXQCTL_STRICT_CYCLE | IGC_TXQCTL_STRICT_END; + /* Counters will always be zero if Strict_End bit is set. + * Condition to check for the IGC_TXQCTL_STRICT_END must be add + * in the future if Strict_End bit is not set. + */ + ring->qbv_tx_overrun = 0; + if (ring->launchtime_enable) txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
Add transmission overrun counter as per defined by IEEE 802.1Q Bridges. The Intel Discrete I225/6 does not have HW counters that can determine whether a packet is still being transmitted after the gate has been closed. But I225/I226 have a mechanism to not transmit a packets if the gate open time is insufficient for the packet transmission by setting the Strict_End bit. Software counters have been created for each queues in response to the IEEE specification. Thus, we can assume that overrun counter is always "0" when setting this bit. User can get this counter by using below command: "ethtool -S <interface> | grep qbv_tx_overrun" Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> --- drivers/net/ethernet/intel/igc/igc.h | 1 + drivers/net/ethernet/intel/igc/igc_ethtool.c | 4 ++++ drivers/net/ethernet/intel/igc/igc_hw.h | 4 ++++ drivers/net/ethernet/intel/igc/igc_main.c | 7 +++++++ drivers/net/ethernet/intel/igc/igc_tsn.c | 6 ++++++ 5 files changed, 22 insertions(+)