[Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Muhammad Husaini Zulkifli]

Add TransmissionOverrun as defined by IEEE Std 802.1Q.
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.

Drivers are now adding the statistics independently because different
hardware controllers may support different hardware queues.

User can get this counter by using below command:
"ethtool -S <interface> | grep TransmissionOverrun"

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

---
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    |  1 +
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 10 ++++++++++
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 34aebf00a5123..a3edae4ef9de8 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -45,6 +45,7 @@ struct igc_tx_queue_stats {
 	u64 bytes;
 	u64 restart_queue;
 	u64 restart_queue2;
+	u64 transmission_overrun;
 };
 
 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..3ab2555d674e1 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, transmission_overrun */
 #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_TransmissionOverrun", 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.transmission_overrun;
 
 		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 ba49728be919a..d252f2ad8e700 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6051,6 +6051,7 @@ 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.transmission_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 94a2b0dfb54d4..b9c07fb7e6aa5 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, TransmissionOverrun 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.transmission_overrun = 0;
+
 		if (ring->launchtime_enable)
 			txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
 
-- 
2.17.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Tony Nguyen]

On 4/10/2023 10:55 PM, Muhammad Husaini Zulkifli wrote:
> Add TransmissionOverrun as defined by IEEE Std 802.1Q.
> 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.

This still nets to adding driver statistics that always reports 0. My 
initial reaction is since it's an IEEE stat and part of a certification 
test, it should go higher than driver level since other drivers running 
the test would need the same statistic? However, I'm not sure how that 
fits in since you're adding per-queue tracking.

Also, not a fan of the camel case naming.

Jakub - are you ok with this 0 driver stat or did you have a thought of 
where you'd like to see it?

Thanks,
Tony

> Drivers are now adding the statistics independently because different
> hardware controllers may support different hardware queues.
> 
> User can get this counter by using below command:
> "ethtool -S <interface> | grep TransmissionOverrun"
> 
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> ---
> 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    |  1 +
>   drivers/net/ethernet/intel/igc/igc_tsn.c     | 10 ++++++++++
>   4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 34aebf00a5123..a3edae4ef9de8 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -45,6 +45,7 @@ struct igc_tx_queue_stats {
>   	u64 bytes;
>   	u64 restart_queue;
>   	u64 restart_queue2;
> +	u64 transmission_overrun;
>   };
>   
>   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..3ab2555d674e1 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, transmission_overrun */
>   #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_TransmissionOverrun", 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.transmission_overrun;
>   
>   		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 ba49728be919a..d252f2ad8e700 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6051,6 +6051,7 @@ 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.transmission_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 94a2b0dfb54d4..b9c07fb7e6aa5 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, TransmissionOverrun 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.transmission_overrun = 0;
> +
>   		if (ring->launchtime_enable)
>   			txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
>   
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Jakub Kicinski]

On Tue, 18 Apr 2023 16:51:17 -0700 Tony Nguyen wrote:
> On 4/10/2023 10:55 PM, Muhammad Husaini Zulkifli wrote:
> > Add TransmissionOverrun as defined by IEEE Std 802.1Q.
> > 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.  
> 
> This still nets to adding driver statistics that always reports 0. My 
> initial reaction is since it's an IEEE stat and part of a certification 
> test, it should go higher than driver level since other drivers running 
> the test would need the same statistic? However, I'm not sure how that 
> fits in since you're adding per-queue tracking.
> 
> Also, not a fan of the camel case naming.
> 
> Jakub - are you ok with this 0 driver stat or did you have a thought of 
> where you'd like to see it?

Seems like something that should be reported back to the qdisc which
configured the gate.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Zulkifli, Muhammad Husaini]

Hi Jakub,

> On Tue, 18 Apr 2023 16:51:17 -0700 Tony Nguyen wrote:
> > On 4/10/2023 10:55 PM, Muhammad Husaini Zulkifli wrote:
> > > Add TransmissionOverrun as defined by IEEE Std 802.1Q.
> > > 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.
> >
> > This still nets to adding driver statistics that always reports 0. My
> > initial reaction is since it's an IEEE stat and part of a
> > certification test, it should go higher than driver level since other
> > drivers running the test would need the same statistic? However, I'm
> > not sure how that fits in since you're adding per-queue tracking.
> >
> > Also, not a fan of the camel case naming.
> >
> > Jakub - are you ok with this 0 driver stat or did you have a thought
> > of where you'd like to see it?
> 
> Seems like something that should be reported back to the qdisc which
> configured the gate.

May I know which qdisc stats that we need to reported out? Is it gnet_stats_queue?
IMHO, gnet_stats_queue stats all used by SW QBV and not by HW QBV.

Thanks,
Husaini



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Jakub Kicinski]

On Tue, 2 May 2023 01:47:30 +0000 Zulkifli, Muhammad Husaini wrote:
> > Seems like something that should be reported back to the qdisc which
> > configured the gate.  
> 
> May I know which qdisc stats that we need to reported out? Is it gnet_stats_queue?
> IMHO, gnet_stats_queue stats all used by SW QBV and not by HW QBV.

Yeah, definitely not gnet_stats_queue, the qdisc used to configure 
the gates can have its dedicated super special stats.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Zulkifli, Muhammad Husaini]

Hi Jakub,

Sorry for the late response.

> On Tue, 2 May 2023 01:47:30 +0000 Zulkifli, Muhammad Husaini wrote:
> > > Seems like something that should be reported back to the qdisc which
> > > configured the gate.
> >
> > May I know which qdisc stats that we need to reported out? Is it
> gnet_stats_queue?
> > IMHO, gnet_stats_queue stats all used by SW QBV and not by HW QBV.
> 
> Yeah, definitely not gnet_stats_queue, the qdisc used to configure the gates can
> have its dedicated super special stats.

I'm not sure how this super special stat will be link to qdisc. 
I'm include Vladimir in this discussion to obtain his thoughts, 
as I feel this will be required for other vendors as well if we can come up 
with a generic approach.

Hi Vladimir,

Do you have any input about this TransmissionOverrun counter?

Thanks,
Husaini
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Vladimir Oltean]

On Sun, May 28, 2023 at 10:10:18AM +0000, Zulkifli, Muhammad Husaini wrote:
> Hi Jakub,
> 
> Sorry for the late response.
> 
> > On Tue, 2 May 2023 01:47:30 +0000 Zulkifli, Muhammad Husaini wrote:
> > > > Seems like something that should be reported back to the qdisc which
> > > > configured the gate.
> > >
> > > May I know which qdisc stats that we need to reported out? Is it
> > gnet_stats_queue?
> > > IMHO, gnet_stats_queue stats all used by SW QBV and not by HW QBV.
> > 
> > Yeah, definitely not gnet_stats_queue, the qdisc used to configure the gates can
> > have its dedicated super special stats.
> 
> I'm not sure how this super special stat will be link to qdisc. 
> I'm include Vladimir in this discussion to obtain his thoughts, 
> as I feel this will be required for other vendors as well if we can come up 
> with a generic approach.
> 
> Hi Vladimir,
> 
> Do you have any input about this TransmissionOverrun counter?
> 
> Thanks,
> Husaini

Thanks for the question. I guess Jakub is talking about qdisc xstats
(root and per class) which can be reported through the TCA_STATS_APP
attribute of the TCA_STATS2 nest. In fact I've started working on a
prototype of this (some infra will be required) and I'm currently
debugging it :)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Vladimir Oltean]

On Mon, May 29, 2023 at 04:59:49PM +0300, Vladimir Oltean wrote:
> On Sun, May 28, 2023 at 10:10:18AM +0000, Zulkifli, Muhammad Husaini wrote:
> > Hi Jakub,
> > 
> > Sorry for the late response.
> > 
> > > On Tue, 2 May 2023 01:47:30 +0000 Zulkifli, Muhammad Husaini wrote:
> > > > > Seems like something that should be reported back to the qdisc which
> > > > > configured the gate.
> > > >
> > > > May I know which qdisc stats that we need to reported out? Is it
> > > gnet_stats_queue?
> > > > IMHO, gnet_stats_queue stats all used by SW QBV and not by HW QBV.
> > > 
> > > Yeah, definitely not gnet_stats_queue, the qdisc used to configure the gates can
> > > have its dedicated super special stats.
> > 
> > I'm not sure how this super special stat will be link to qdisc. 
> > I'm include Vladimir in this discussion to obtain his thoughts, 
> > as I feel this will be required for other vendors as well if we can come up 
> > with a generic approach.
> > 
> > Hi Vladimir,
> > 
> > Do you have any input about this TransmissionOverrun counter?
> > 
> > Thanks,
> > Husaini
> 
> Thanks for the question. I guess Jakub is talking about qdisc xstats
> (root and per class) which can be reported through the TCA_STATS_APP
> attribute of the TCA_STATS2 nest. In fact I've started working on a
> prototype of this (some infra will be required) and I'm currently
> debugging it :)

Okay, debugging done, here's what I've got:
https://github.com/vladimiroltean/linux/commits/sch-taprio-stats
https://github.com/vladimiroltean/iproute2/commits/taprio-xstats

Jakub, is this what you had in mind?

$ tc -d -s qdisc show dev eth0 root
Warning: sch_taprio: Size table not specified, frame length estimations may be inaccurate.
qdisc taprio 800e: root tc 8 map 0 1 2 3 4 5 6 7 0 0 0 0 0 0 0 0
queues offset 0 count 1 offset 1 count 1 offset 2 count 1 offset 3 count 1 offset 4 count 1 offset 5 count 1 offset 6 count 1 offset 7 count 1
 flags 0x2      base-time 0 cycle-time 1000000 cycle-time-extension 0
        index 0 cmd S gatemask 0x20 interval 200000
        index 1 cmd S gatemask 0xdf interval 800000
max-sdu 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
fp E E E E E E E E E E E E E E E E

 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new

$ tc -d -s class show dev eth0
class taprio 800e:1 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new
class taprio 800e:2 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new
class taprio 800e:3 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new
class taprio 800e:4 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new
class taprio 800e:5 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new
class taprio 800e:6 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new
class taprio 800e:7 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new
class taprio 800e:8 root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0
 Window drops: 0 <----------------- this is new

ENETC (on which I tested) doesn't overrun the taprio time gates, so it
has no reason to implement the TransmitOverrun counter that's being
discussed here. But it was a good opportunity to move a (non-standard)
counter from ethtool -S to the Qdisc layer :)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Jakub Kicinski]

On Mon, 29 May 2023 20:33:57 +0300 Vladimir Oltean wrote:
> Okay, debugging done, here's what I've got:
> https://github.com/vladimiroltean/linux/commits/sch-taprio-stats
> https://github.com/vladimiroltean/iproute2/commits/taprio-xstats
> 
> Jakub, is this what you had in mind?

yup, 👍️👍️
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Zulkifli, Muhammad Husaini]

> On Mon, May 29, 2023 at 04:59:49PM +0300, Vladimir Oltean wrote:
> > On Sun, May 28, 2023 at 10:10:18AM +0000, Zulkifli, Muhammad Husaini
> wrote:
> > > Hi Jakub,
> > >
> > > Sorry for the late response.
> > >
> > > > On Tue, 2 May 2023 01:47:30 +0000 Zulkifli, Muhammad Husaini wrote:
> > > > > > Seems like something that should be reported back to the qdisc
> > > > > > which configured the gate.
> > > > >
> > > > > May I know which qdisc stats that we need to reported out? Is it
> > > > gnet_stats_queue?
> > > > > IMHO, gnet_stats_queue stats all used by SW QBV and not by HW QBV.
> > > >
> > > > Yeah, definitely not gnet_stats_queue, the qdisc used to configure
> > > > the gates can have its dedicated super special stats.
> > >
> > > I'm not sure how this super special stat will be link to qdisc.
> > > I'm include Vladimir in this discussion to obtain his thoughts, as I
> > > feel this will be required for other vendors as well if we can come
> > > up with a generic approach.
> > >
> > > Hi Vladimir,
> > >
> > > Do you have any input about this TransmissionOverrun counter?
> > >
> > > Thanks,
> > > Husaini
> >
> > Thanks for the question. I guess Jakub is talking about qdisc xstats
> > (root and per class) which can be reported through the TCA_STATS_APP
> > attribute of the TCA_STATS2 nest. In fact I've started working on a
> > prototype of this (some infra will be required) and I'm currently
> > debugging it :)
> 
> Okay, debugging done, here's what I've got:
> https://github.com/vladimiroltean/linux/commits/sch-taprio-stats
> https://github.com/vladimiroltean/iproute2/commits/taprio-xstats
> 
> Jakub, is this what you had in mind?
> 
> $ tc -d -s qdisc show dev eth0 root
> Warning: sch_taprio: Size table not specified, frame length estimations may be
> inaccurate.
> qdisc taprio 800e: root tc 8 map 0 1 2 3 4 5 6 7 0 0 0 0 0 0 0 0 queues offset 0
> count 1 offset 1 count 1 offset 2 count 1 offset 3 count 1 offset 4 count 1 offset
> 5 count 1 offset 6 count 1 offset 7 count 1
>  flags 0x2      base-time 0 cycle-time 1000000 cycle-time-extension 0
>         index 0 cmd S gatemask 0x20 interval 200000
>         index 1 cmd S gatemask 0xdf interval 800000 max-sdu 0 0 0 0 0 0 0 0 0 0 0
> 0 0 0 0 0 fp E E E E E E E E E E E E E E E E
> 
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p
> requeues 0  Window drops: 0 <----------------- this is new
> 
> $ tc -d -s class show dev eth0
> class taprio 800e:1 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p
> requeues 0  Window drops: 0 <----------------- this is new class taprio 800e:2 root
> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p requeues
> 0  Window drops: 0 <----------------- this is new class taprio 800e:3 root  Sent 0
> bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p requeues 0
> Window drops: 0 <----------------- this is new class taprio 800e:4 root  Sent 0
> bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p requeues 0
> Window drops: 0 <----------------- this is new class taprio 800e:5 root  Sent 0
> bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p requeues 0
> Window drops: 0 <----------------- this is new class taprio 800e:6 root  Sent 0
> bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p requeues 0
> Window drops: 0 <----------------- this is new class taprio 800e:7 root  Sent 0
> bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p requeues 0
> Window drops: 0 <----------------- this is new class taprio 800e:8 root  Sent 0
> bytes 0 pkt (dropped 0, overlimits 0 requeues 0)  backlog 0b 0p requeues 0
> Window drops: 0 <----------------- this is new
> 
> ENETC (on which I tested) doesn't overrun the taprio time gates, so it has no
> reason to implement the TransmitOverrun counter that's being discussed here.
> But it was a good opportunity to move a (non-standard) counter from ethtool -S
> to the Qdisc layer :)

Vladimir.. That was a very quick and great idea :)
Yes, it appears that exposing counters to the Qdisc layer is a good idea.
Could you kindly include(cc) me in the patch series if you decide to upstream 
this approach later? On top of that, I'll add my own changes.

Thanks,
Husaini
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igc: Add TransmissionOverrun counter

[Vladimir Oltean]

On Tue, May 30, 2023 at 02:56:30AM +0000, Zulkifli, Muhammad Husaini wrote:
> Vladimir.. That was a very quick and great idea :)
> Yes, it appears that exposing counters to the Qdisc layer is a good idea.
> Could you kindly include(cc) me in the patch series if you decide to upstream 
> this approach later? On top of that, I'll add my own changes.

Ok, I'll send the patch set to net-next now.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan