| Message ID | 20230601005925.30473-1-muhammad.husaini.zulkifli@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net,v3] igc: Add TransmissionOverrun counter | expand |
Hi, > -----Original Message----- > From: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Sent: Thursday, 1 June, 2023 8:59 AM > To: intel-wired-lan@osuosl.org > Cc: Gomes, Vinicius <vinicius.gomes@intel.com>; Zulkifli, Muhammad > Husaini <muhammad.husaini.zulkifli@intel.com>; Neftin, Sasha > <sasha.neftin@intel.com>; tee.min.tan@linux.intel.com; > vladimir.oltean@nxp.com; naamax.meir@linux.intel.com; Nguyen, Anthony L > <anthony.l.nguyen@intel.com> > Subject: [PATCH iwl-net v3] igc: Add TransmissionOverrun counter Sorry I think I accidentally copy paste wrong subject prefix when generate this V3 patch. It should go to "net-next". My bad. > > Add TransmissionOverrun as per defined by IEEE 802.1Q Bridges. > TransmissionOverrun counter shall be incremented if the implementation > detects that a frame from a given queue is still being transmitted by the MAC > when that gate-close event for that queue occurs. > > This counter is utilised by the Certification conformance test to inform the > user application whether any packets are currently being transmitted on a > particular queue during a gate-close event. > > Intel Discrete 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. Thus, it is expected for this counter to be always zero at this > moment. > > Inspired from enetc_taprio_stats() and enetc_taprio_tc_stats(), now driver > also report the tx_overruns counter per traffic class. > > User can get this counter by using below command: > 1) ethtool -S <interface> | grep Transmit_overruns > 2) tc -s qdisc show dev <interface> root > 3) tc -s class show dev <interface> > > Signed-off-by: Muhammad Husaini Zulkifli > <muhammad.husaini.zulkifli@intel.com> > > --- > V2 -> V3: Included new infra xstats to report back the counter to qdisc > V1 -> V2: Change per-queue stats. Driver still remains adding the > statistic independently. > --- > --- > drivers/net/ethernet/intel/igc/igc.h | 1 + > drivers/net/ethernet/intel/igc/igc_ethtool.c | 4 ++- > drivers/net/ethernet/intel/igc/igc_main.c | 38 ++++++++++++++++++++ > drivers/net/ethernet/intel/igc/igc_tsn.c | 10 ++++++ > 4 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h > b/drivers/net/ethernet/intel/igc/igc.h > index cb5751fab03c9..2a13e62b75d60 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -46,6 +46,7 @@ struct igc_tx_queue_stats { > u64 bytes; > u64 restart_queue; > u64 restart_queue2; > + u64 tx_overruns; > }; > > struct igc_rx_queue_stats { > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c > b/drivers/net/ethernet/intel/igc/igc_ethtool.c > index 0e2cb00622d1a..6a10ae1474fc5 100644 > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > @@ -112,7 +112,7 @@ static const char > igc_gstrings_test[][ETH_GSTRING_LEN] = { > (sizeof(igc_gstrings_net_stats) / sizeof(struct igc_stats)) #define > IGC_RX_QUEUE_STATS_LEN \ > (sizeof(struct igc_rx_queue_stats) / sizeof(u64)) -#define > IGC_TX_QUEUE_STATS_LEN 3 /* packets, bytes, restart_queue */ > +#define IGC_TX_QUEUE_STATS_LEN 4 /* packets, bytes, restart_queue, > +tx_overruns */ > #define IGC_QUEUE_STATS_LEN \ > ((((struct igc_adapter *)netdev_priv(netdev))->num_rx_queues * \ > IGC_RX_QUEUE_STATS_LEN) + \ > @@ -781,6 +781,7 @@ static void igc_ethtool_get_strings(struct net_device > *netdev, u32 stringset, > ethtool_sprintf(&p, "tx_queue_%u_packets", i); > ethtool_sprintf(&p, "tx_queue_%u_bytes", i); > ethtool_sprintf(&p, "tx_queue_%u_restart", i); > + ethtool_sprintf(&p, > "tx_queue_%u_Transmit_overruns", i); > } > for (i = 0; i < adapter->num_rx_queues; i++) { > ethtool_sprintf(&p, "rx_queue_%u_packets", i); @@ - > 850,6 +851,7 @@ static void igc_ethtool_get_stats(struct net_device > *netdev, > restart2 = ring->tx_stats.restart_queue2; > } while (u64_stats_fetch_retry(&ring->tx_syncp2, start)); > data[i + 2] += restart2; > + data[i + 3] = ring->tx_stats.tx_overruns; > > i += IGC_TX_QUEUE_STATS_LEN; > } > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > b/drivers/net/ethernet/intel/igc/igc_main.c > index aa9f23b7f0c1a..056925a7bcdd8 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -6109,11 +6109,43 @@ static int igc_tsn_clear_schedule(struct > igc_adapter *adapter) > ring->start_time = 0; > ring->end_time = NSEC_PER_SEC; > ring->max_sdu = 0; > + ring->tx_stats.tx_overruns = 0; > } > > return 0; > } > > +static void igc_taprio_stats(struct net_device *dev, > + struct tc_taprio_qopt_stats *stats) { > + struct igc_adapter *adapter = netdev_priv(dev); > + u64 tx_overruns = 0; > + int i; > + > + for (i = 0; i < adapter->num_tx_queues; i++) { > + struct igc_ring *tx_ring = adapter->tx_ring[i]; > + > + tx_overruns += tx_ring->tx_stats.tx_overruns; > + } > + > + stats->tx_overruns = tx_overruns; > +} > + > +static void igc_taprio_tc_stats(struct net_device *dev, > + struct tc_taprio_qopt_tc_stats *tc_stats) { > + struct tc_taprio_qopt_stats *stats = &tc_stats->stats; > + struct igc_adapter *adapter = netdev_priv(dev); > + struct igc_ring *tx_ring; > + int tc = tc_stats->tc; > + int txq; > + > + txq = dev->tc_to_txq[tc].offset; > + tx_ring = adapter->tx_ring[txq]; > + > + stats->tx_overruns = tx_ring->tx_stats.tx_overruns; } > + > static int igc_save_qbv_schedule(struct igc_adapter *adapter, > struct tc_taprio_qopt_offload *qopt) { @@ - > 6130,6 +6162,12 @@ static int igc_save_qbv_schedule(struct igc_adapter > *adapter, > case TAPRIO_CMD_DESTROY: > adapter->qbv_enable = false; > break; > + case TAPRIO_CMD_STATS: > + igc_taprio_stats(adapter->netdev, &qopt->stats); > + return 0; > + case TAPRIO_CMD_TC_STATS: > + igc_taprio_tc_stats(adapter->netdev, &qopt->tc_stats); > + return 0; > default: > return -EOPNOTSUPP; > } > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c > b/drivers/net/ethernet/intel/igc/igc_tsn.c > index 6b299b83e7ef2..342530d11aae9 100644 > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > @@ -136,6 +136,16 @@ static int igc_tsn_enable_offload(struct igc_adapter > *adapter) > txqctl |= IGC_TXQCTL_STRICT_CYCLE | > IGC_TXQCTL_STRICT_END; > > + /* If it notices that a frame from a particular queue is still > + * being transmitted by MAC, tx_overruns shall be increased. > + * But currently driver setting Strict_End bit which indicate > + * that packet from the queue can be transmitted only if they > + * are expected to be completed before the windows of the > + * queue is ended. Thus, this counter will always be zero > when > + * Strict_End is set. > + */ > + ring->tx_stats.tx_overruns = 0; > + > if (ring->launchtime_enable) > txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT; > > -- > 2.17.1
Hi Husaini, On Thu, Jun 01, 2023 at 08:59:25AM +0800, Muhammad Husaini Zulkifli wrote: > Add TransmissionOverrun as per defined by IEEE 802.1Q Bridges. > TransmissionOverrun counter shall be incremented if the implementation > detects that a frame from a given queue is still being transmitted by > the MAC when that gate-close event for that queue occurs. > > This counter is utilised by the Certification conformance test to > inform the user application whether any packets are currently being > transmitted on a particular queue during a gate-close event. > > Intel Discrete 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. Thus, it is expected for this counter to be always > zero at this moment. > > Inspired from enetc_taprio_stats() and enetc_taprio_tc_stats(), now > driver also report the tx_overruns counter per traffic class. > > User can get this counter by using below command: > 1) ethtool -S <interface> | grep Transmit_overruns Is the unstructured ethtool -S still a viable reporting mechanism if a standardized place for reporting the counter exists? > 2) tc -s qdisc show dev <interface> root > 3) tc -s class show dev <interface> > > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > > --- > V2 -> V3: Included new infra xstats to report back the counter to qdisc > V1 -> V2: Change per-queue stats. Driver still remains adding the > statistic independently. > --- > --- > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c > index 6b299b83e7ef2..342530d11aae9 100644 > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > @@ -136,6 +136,16 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) > txqctl |= IGC_TXQCTL_STRICT_CYCLE | > IGC_TXQCTL_STRICT_END; > > + /* If it notices that a frame from a particular queue is still > + * being transmitted by MAC, tx_overruns shall be increased. > + * But currently driver setting Strict_End bit which indicate > + * that packet from the queue can be transmitted only if they > + * are expected to be completed before the windows of the > + * queue is ended. Thus, this counter will always be zero when > + * Strict_End is set. > + */ > + ring->tx_stats.tx_overruns = 0; What is the purpose of keeping a constant (0) in a variable replicated per TX queue? It is a waste of space. Also, if IGC_TXQCTL_STRICT_END wasn't set, how would the window overruns be counted? > + > if (ring->launchtime_enable) > txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT; > > -- > 2.17.1 >
Hi Vladimir, > -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Tuesday, 6 June, 2023 10:03 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Gomes, Vinicius <vinicius.gomes@intel.com>; > Neftin, Sasha <sasha.neftin@intel.com>; tee.min.tan@linux.intel.com; > naamax.meir@linux.intel.com; Nguyen, Anthony L > <anthony.l.nguyen@intel.com> > Subject: Re: [PATCH iwl-net v3] igc: Add TransmissionOverrun counter > > Hi Husaini, > > On Thu, Jun 01, 2023 at 08:59:25AM +0800, Muhammad Husaini Zulkifli > wrote: > > Add TransmissionOverrun as per defined by IEEE 802.1Q Bridges. > > TransmissionOverrun counter shall be incremented if the implementation > > detects that a frame from a given queue is still being transmitted by > > the MAC when that gate-close event for that queue occurs. > > > > This counter is utilised by the Certification conformance test to > > inform the user application whether any packets are currently being > > transmitted on a particular queue during a gate-close event. > > > > Intel Discrete 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. Thus, it is expected for this counter to > > be always zero at this moment. > > > > Inspired from enetc_taprio_stats() and enetc_taprio_tc_stats(), now > > driver also report the tx_overruns counter per traffic class. > > > > User can get this counter by using below command: > > 1) ethtool -S <interface> | grep Transmit_overruns > > Is the unstructured ethtool -S still a viable reporting mechanism if a > standardized place for reporting the counter exists? This can be remove. I will remove the ethtool reporting mechanism. > > > 2) tc -s qdisc show dev <interface> root > > 3) tc -s class show dev <interface> > > > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com> > > > > --- > > V2 -> V3: Included new infra xstats to report back the counter to > > qdisc > > V1 -> V2: Change per-queue stats. Driver still remains adding the > > statistic independently. > > --- > > --- > > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c > > b/drivers/net/ethernet/intel/igc/igc_tsn.c > > index 6b299b83e7ef2..342530d11aae9 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > > @@ -136,6 +136,16 @@ static int igc_tsn_enable_offload(struct > igc_adapter *adapter) > > txqctl |= IGC_TXQCTL_STRICT_CYCLE | > > IGC_TXQCTL_STRICT_END; > > > > + /* If it notices that a frame from a particular queue is still > > + * being transmitted by MAC, tx_overruns shall be increased. > > + * But currently driver setting Strict_End bit which indicate > > + * that packet from the queue can be transmitted only if they > > + * are expected to be completed before the windows of the > > + * queue is ended. Thus, this counter will always be zero when > > + * Strict_End is set. > > + */ > > + ring->tx_stats.tx_overruns = 0; > > What is the purpose of keeping a constant (0) in a variable replicated per TX > queue? It is a waste of space. Purpose is for the Certification conformance test. This is used to notify the application (automated) if any packets are currently being transmitted on a certain queue during a gate-close event. Since we set the Strict_end bit, the value will always be zero. > > Also, if IGC_TXQCTL_STRICT_END wasn't set, how would the window overruns > be counted? Currently, there is no log or statistics about packets taking advantage of STRICT_END not being set. Thanks, Husaini > > > > > if (ring->launchtime_enable) > > txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT; > > > > -- > > 2.17.1 > >
On Fri, Jun 09, 2023 at 05:10:24PM +0000, Zulkifli, Muhammad Husaini wrote: > > What is the purpose of keeping a constant (0) in a variable replicated per TX > > queue? It is a waste of space. > > Purpose is for the Certification conformance test. > This is used to notify the application (automated) if any packets are currently > being transmitted on a certain queue during a gate-close event. I agree with you, but that wasn't the question. The question was "why bother keeping a constant (0) in memory storage whose lifetime is the driver's lifetime, when you can just report those constants in the taprio callbacks, bypassing the driver storage, as long as you're not going to read them from hardware anyway". > Since we set the Strict_end bit, the value will always be zero. Both statements are true ("we set the Strict_end bit" and "the value will always be zero"), but the deduction is false. That's not why the value is zero, the value is zero because no one sets it to any other value :) If the counter actually monitored a physical event, then I'd have agreed with your statement. > > Also, if IGC_TXQCTL_STRICT_END wasn't set, how would the window overruns > > be counted? > > Currently, there is no log or statistics about packets taking advantage of STRICT_END > not being set. You may have missed my question here. Does the hardware have any reporting mechanism for TX overruns? Irrespective of what the current driver has or does not have. I imagine the hardware should have something like this, since IGC_TXQCTL_STRICT_END is an opt-in feature, and there should be something non-zero to count when that bit isn't set. If it does, wouldn't it be better to query the hardware for this information, regardless of the STRICT_END bit, rather than just assume it will be zero? Due to things like congestion on memory, I suppose it might be possible for the NIC to start the DMA transfer of a frame in time for its timely transmission, but nonetheless the frame transmission could still overrun the window. Only the hardware could know if that was the case or not (or a hardware TX timestamp adjusted for the frame length). So what I'm saying is that there's value in reading this from value even if you think that STRICT_END should be all that's needed. For context, on NXP LS1028A, the fact that the hardware (ENETC) doesn't report a TransmitOverrun counter is an erratum, even though ENETC also has logic to stop sending packets early enough so as to not overrun the window. It's a bit of a leap of faith for the software driver to politically report a counter as zero and for higher layers to trust that as first-hand evidence, and this is why, at least in ENETC, I chose to not report TransmitOverrun rather than be confidently wrong about it.
Dear Vladimir, > On Fri, Jun 09, 2023 at 05:10:24PM +0000, Zulkifli, Muhammad Husaini wrote: > > > What is the purpose of keeping a constant (0) in a variable > > > replicated per TX queue? It is a waste of space. > > > > Purpose is for the Certification conformance test. > > This is used to notify the application (automated) if any packets are > > currently being transmitted on a certain queue during a gate-close event. > > I agree with you, but that wasn't the question. The question was "why bother > keeping a constant (0) in memory storage whose lifetime is the driver's > lifetime, when you can just report those constants in the taprio callbacks, > bypassing the driver storage, as long as you're not going to read them from > hardware anyway". Yeah. You are right. I could just report "0" in the taprio callback. Thanks for the suggestion 😊 > > > Since we set the Strict_end bit, the value will always be zero. > > Both statements are true ("we set the Strict_end bit" and "the value will > always be zero"), but the deduction is false. That's not why the value is zero, > the value is zero because no one sets it to any other value :) If the counter > actually monitored a physical event, then I'd have agreed with your > statement. It's not like no one changed it. This value should be obtained straight from the HW statistic count register. But yeah... I'll get to that in my subsequent comment. > > > > Also, if IGC_TXQCTL_STRICT_END wasn't set, how would the window > > > overruns be counted? > > > > Currently, there is no log or statistics about packets taking > > advantage of STRICT_END not being set. > > You may have missed my question here. Sorry my bad. > > Does the hardware have any reporting mechanism for TX overruns? To be honest, no. ☹ > Irrespective of what the current driver has or does not have. > I imagine the hardware should have something like this, since > IGC_TXQCTL_STRICT_END is an opt-in feature, and there should be > something non-zero to count when that bit isn't set. There is no HW statistic count for transmission overrun. I agreed with you, when this bit is not set, there should be non-zero value. When setting TSN mode, we now make the STRICT_END flag mandatory for QBV operation in the driver code. So that the transmission is only enabled inside the QBV time windows and obeying the STRICT_END parameter of the queue. So we can expect "0" packet overrun when this bit is enabled in our case. > > If it does, wouldn't it be better to query the hardware for this information, > regardless of the STRICT_END bit, rather than just assume it will be zero? > > Due to things like congestion on memory, I suppose it might be possible for > the NIC to start the DMA transfer of a frame in time for its timely > transmission, but nonetheless the frame transmission could still overrun the > window. Only the hardware could know if that was the case or not (or a > hardware TX timestamp adjusted for the frame length). So what I'm saying is > that there's value in reading this from value even if you think that > STRICT_END should be all that's needed. Yeah. Only HW could know for sure if the transmission is overrun out of QBV Window or not.....But HW does not give any HW statistic register value for this. > > For context, on NXP LS1028A, the fact that the hardware (ENETC) doesn't > report a TransmitOverrun counter is an erratum, even though ENETC also has > logic to stop sending packets early enough so as to not overrun the window. > > It's a bit of a leap of faith for the software driver to politically report a counter > as zero and for higher layers to trust that as first-hand evidence, and this is > why, at least in ENETC, I chose to not report TransmitOverrun rather than be > confidently wrong about it. I see what you're saying. However, because I225 HW has this STRIC_END bit option, we can at least guarantee that HW is handling it where packets from the queue can be transmitted only if they are expected to be completed before the window of the Queue is ended. Thanks, Husaini
On Mon, Jun 12, 2023 at 03:50:19AM +0000, Zulkifli, Muhammad Husaini wrote: > There is no HW statistic count for transmission overrun. > I agreed with you, when this bit is not set, there should be non-zero value. > > When setting TSN mode, we now make the STRICT_END flag mandatory for QBV > operation in the driver code. So that the transmission is only enabled inside the QBV > time windows and obeying the STRICT_END parameter of the queue. So we can expect > "0" packet overrun when this bit is enabled in our case. (...) > > Yeah. Only HW could know for sure if the transmission is overrun out of QBV > Window or not.....But HW does not give any HW statistic register value for this. (...) > > I see what you're saying. However, because I225 HW has this STRIC_END bit option, > we can at least guarantee that HW is handling it where packets from the queue can be > transmitted only if they are expected to be completed before the window of the Queue > is ended. Are the people doing the certification testing aware of the fact that your hardware does not monitor and report transmission overruns, and thus, that the counter of "0" that the driver reports may be confidently wrong? Are they ok with this?
Dear Vladimir, > -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Monday, 12 June, 2023 10:28 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Gomes, Vinicius <vinicius.gomes@intel.com>; > Neftin, Sasha <sasha.neftin@intel.com>; tee.min.tan@linux.intel.com; > naamax.meir@linux.intel.com; Nguyen, Anthony L > <anthony.l.nguyen@intel.com> > Subject: Re: [PATCH iwl-net v3] igc: Add TransmissionOverrun counter > > On Mon, Jun 12, 2023 at 03:50:19AM +0000, Zulkifli, Muhammad Husaini > wrote: > > There is no HW statistic count for transmission overrun. > > I agreed with you, when this bit is not set, there should be non-zero value. > > > > When setting TSN mode, we now make the STRICT_END flag mandatory for > > QBV operation in the driver code. So that the transmission is only > > enabled inside the QBV time windows and obeying the STRICT_END > > parameter of the queue. So we can expect "0" packet overrun when this bit is > enabled in our case. > (...) > > > > Yeah. Only HW could know for sure if the transmission is overrun out > > of QBV Window or not.....But HW does not give any HW statistic register value > for this. > (...) > > > > I see what you're saying. However, because I225 HW has this STRIC_END > > bit option, we can at least guarantee that HW is handling it where > > packets from the queue can be transmitted only if they are expected to > > be completed before the window of the Queue is ended. > > Are the people doing the certification testing aware of the fact that your > hardware does not monitor and report transmission overruns, and thus, that > the counter of "0" that the driver reports may be confidently wrong? Are they > ok with this? No, they aren't aware of it. They will just query this counter to determine if the value has increased or not. The test plan does not rely entirely on the counter. On the receive side, it will validate all frames for the traffic class involved. Any transmitted frame that is not received or any of the received frame does not fulfil the frameEndTime criteria will cause the test plan to fail before reading the overrun counter again. Since our HW with STRICT_END bit able to fulfill the criteria, we can assume that Counter "0" is valid here. Unless HW fails to meet the framEndtime criteria and reports a "0" value... However, in this scenario, it will eventually fail the test plan.
On Tue, Jun 13, 2023 at 03:38:54PM +0000, Zulkifli, Muhammad Husaini wrote: > No, they aren't aware of it. They will just query this counter to determine if the > value has increased or not. The test plan does not rely entirely on the counter. > On the receive side, it will validate all frames for the traffic class involved. > Any transmitted frame that is not received or any of the received frame > does not fulfil the frameEndTime criteria will cause the test plan to fail before > reading the overrun counter again. Since our HW with STRICT_END bit able to > fulfill the criteria, we can assume that Counter "0" is valid here. > > Unless HW fails to meet the framEndtime criteria and reports a "0" value... > However, in this scenario, it will eventually fail the test plan. Ok, I take back this objection then, only the other one remains in that case, with unnecessarily keeping this constant counter in the driver's ring data structures.
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Tuesday, 13 June, 2023 11:41 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Gomes, Vinicius <vinicius.gomes@intel.com>; > Neftin, Sasha <sasha.neftin@intel.com>; tee.min.tan@linux.intel.com; > naamax.meir@linux.intel.com; Nguyen, Anthony L > <anthony.l.nguyen@intel.com> > Subject: Re: [PATCH iwl-net v3] igc: Add TransmissionOverrun counter > > On Tue, Jun 13, 2023 at 03:38:54PM +0000, Zulkifli, Muhammad Husaini > wrote: > > No, they aren't aware of it. They will just query this counter to > > determine if the value has increased or not. The test plan does not rely entirely > on the counter. > > On the receive side, it will validate all frames for the traffic class involved. > > Any transmitted frame that is not received or any of the received > > frame does not fulfil the frameEndTime criteria will cause the test > > plan to fail before reading the overrun counter again. Since our HW > > with STRICT_END bit able to fulfill the criteria, we can assume that Counter "0" > is valid here. > > > > Unless HW fails to meet the framEndtime criteria and reports a "0" value... > > However, in this scenario, it will eventually fail the test plan. > > Ok, I take back this objection then, only the other one remains in that case, with > unnecessarily keeping this constant counter in the driver's ring data structures. Sure. I will remove that one and resend again. Thanks 😊
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index cb5751fab03c9..2a13e62b75d60 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -46,6 +46,7 @@ struct igc_tx_queue_stats { u64 bytes; u64 restart_queue; u64 restart_queue2; + u64 tx_overruns; }; struct igc_rx_queue_stats { diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 0e2cb00622d1a..6a10ae1474fc5 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -112,7 +112,7 @@ static const char igc_gstrings_test[][ETH_GSTRING_LEN] = { (sizeof(igc_gstrings_net_stats) / sizeof(struct igc_stats)) #define IGC_RX_QUEUE_STATS_LEN \ (sizeof(struct igc_rx_queue_stats) / sizeof(u64)) -#define IGC_TX_QUEUE_STATS_LEN 3 /* packets, bytes, restart_queue */ +#define IGC_TX_QUEUE_STATS_LEN 4 /* packets, bytes, restart_queue, tx_overruns */ #define IGC_QUEUE_STATS_LEN \ ((((struct igc_adapter *)netdev_priv(netdev))->num_rx_queues * \ IGC_RX_QUEUE_STATS_LEN) + \ @@ -781,6 +781,7 @@ static void igc_ethtool_get_strings(struct net_device *netdev, u32 stringset, ethtool_sprintf(&p, "tx_queue_%u_packets", i); ethtool_sprintf(&p, "tx_queue_%u_bytes", i); ethtool_sprintf(&p, "tx_queue_%u_restart", i); + ethtool_sprintf(&p, "tx_queue_%u_Transmit_overruns", i); } for (i = 0; i < adapter->num_rx_queues; i++) { ethtool_sprintf(&p, "rx_queue_%u_packets", i); @@ -850,6 +851,7 @@ static void igc_ethtool_get_stats(struct net_device *netdev, restart2 = ring->tx_stats.restart_queue2; } while (u64_stats_fetch_retry(&ring->tx_syncp2, start)); data[i + 2] += restart2; + data[i + 3] = ring->tx_stats.tx_overruns; i += IGC_TX_QUEUE_STATS_LEN; } diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index aa9f23b7f0c1a..056925a7bcdd8 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6109,11 +6109,43 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) ring->start_time = 0; ring->end_time = NSEC_PER_SEC; ring->max_sdu = 0; + ring->tx_stats.tx_overruns = 0; } return 0; } +static void igc_taprio_stats(struct net_device *dev, + struct tc_taprio_qopt_stats *stats) +{ + struct igc_adapter *adapter = netdev_priv(dev); + u64 tx_overruns = 0; + int i; + + for (i = 0; i < adapter->num_tx_queues; i++) { + struct igc_ring *tx_ring = adapter->tx_ring[i]; + + tx_overruns += tx_ring->tx_stats.tx_overruns; + } + + stats->tx_overruns = tx_overruns; +} + +static void igc_taprio_tc_stats(struct net_device *dev, + struct tc_taprio_qopt_tc_stats *tc_stats) +{ + struct tc_taprio_qopt_stats *stats = &tc_stats->stats; + struct igc_adapter *adapter = netdev_priv(dev); + struct igc_ring *tx_ring; + int tc = tc_stats->tc; + int txq; + + txq = dev->tc_to_txq[tc].offset; + tx_ring = adapter->tx_ring[txq]; + + stats->tx_overruns = tx_ring->tx_stats.tx_overruns; +} + static int igc_save_qbv_schedule(struct igc_adapter *adapter, struct tc_taprio_qopt_offload *qopt) { @@ -6130,6 +6162,12 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, case TAPRIO_CMD_DESTROY: adapter->qbv_enable = false; break; + case TAPRIO_CMD_STATS: + igc_taprio_stats(adapter->netdev, &qopt->stats); + return 0; + case TAPRIO_CMD_TC_STATS: + igc_taprio_tc_stats(adapter->netdev, &qopt->tc_stats); + return 0; default: return -EOPNOTSUPP; } diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index 6b299b83e7ef2..342530d11aae9 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -136,6 +136,16 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) txqctl |= IGC_TXQCTL_STRICT_CYCLE | IGC_TXQCTL_STRICT_END; + /* If it notices that a frame from a particular queue is still + * being transmitted by MAC, tx_overruns shall be increased. + * But currently driver setting Strict_End bit which indicate + * that packet from the queue can be transmitted only if they + * are expected to be completed before the windows of the + * queue is ended. Thus, this counter will always be zero when + * Strict_End is set. + */ + ring->tx_stats.tx_overruns = 0; + if (ring->launchtime_enable) txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
Add TransmissionOverrun as per defined by IEEE 802.1Q Bridges. TransmissionOverrun counter shall be incremented if the implementation detects that a frame from a given queue is still being transmitted by the MAC when that gate-close event for that queue occurs. This counter is utilised by the Certification conformance test to inform the user application whether any packets are currently being transmitted on a particular queue during a gate-close event. Intel Discrete 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. Thus, it is expected for this counter to be always zero at this moment. Inspired from enetc_taprio_stats() and enetc_taprio_tc_stats(), now driver also report the tx_overruns counter per traffic class. User can get this counter by using below command: 1) ethtool -S <interface> | grep Transmit_overruns 2) tc -s qdisc show dev <interface> root 3) tc -s class show dev <interface> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> --- V2 -> V3: Included new infra xstats to report back the counter to qdisc V1 -> V2: Change per-queue stats. Driver still remains adding the statistic independently. --- --- drivers/net/ethernet/intel/igc/igc.h | 1 + drivers/net/ethernet/intel/igc/igc_ethtool.c | 4 ++- drivers/net/ethernet/intel/igc/igc_main.c | 38 ++++++++++++++++++++ drivers/net/ethernet/intel/igc/igc_tsn.c | 10 ++++++ 4 files changed, 52 insertions(+), 1 deletion(-)