[PATCH net-next v1 1/3] net: dsa: add get_pause_stats support

[PATCH net-next v1 1/3] net: dsa: add get_pause_stats support

[Oleksij Rempel]

Add support for pause stats

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/net/dsa.h |  2 ++
 net/dsa/slave.c   | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 14f07275852b..c8d7696cb2bf 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -890,6 +890,8 @@ struct dsa_switch_ops {
 				      struct ethtool_eth_ctrl_stats *ctrl_stats);
 	void	(*get_stats64)(struct dsa_switch *ds, int port,
 				   struct rtnl_link_stats64 *s);
+	void	(*get_pause_stats)(struct dsa_switch *ds, int port,
+				   struct ethtool_pause_stats *pause_stats);
 	void	(*self_test)(struct dsa_switch *ds, int port,
 			     struct ethtool_test *etest, u64 *data);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2e1ac638d135..b2710ee46644 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1097,6 +1097,16 @@ static int dsa_slave_set_link_ksettings(struct net_device *dev,
 	return phylink_ethtool_ksettings_set(dp->pl, cmd);
 }
 
+static void dsa_slave_get_pause_stats(struct net_device *dev,
+				  struct ethtool_pause_stats *pause_stats)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
+
+	if (ds->ops->get_pause_stats)
+		ds->ops->get_pause_stats(ds, dp->index, pause_stats);
+}
+
 static void dsa_slave_get_pauseparam(struct net_device *dev,
 				     struct ethtool_pauseparam *pause)
 {
@@ -2087,6 +2097,7 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_eee		= dsa_slave_get_eee,
 	.get_link_ksettings	= dsa_slave_get_link_ksettings,
 	.set_link_ksettings	= dsa_slave_set_link_ksettings,
+	.get_pause_stats	= dsa_slave_get_pause_stats,
 	.get_pauseparam		= dsa_slave_get_pauseparam,
 	.set_pauseparam		= dsa_slave_set_pauseparam,
 	.get_rxnfc		= dsa_slave_get_rxnfc,
-- 
2.30.2

[PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Oleksij Rempel]

Add support for pause stats and fix rx_packets/tx_packets calculation.

Pause packets are counted by raw.rx64byte/raw.tx64byte counters, so
subtract it from main rx_packets/tx_packets counters.

tx_/rx_bytes are not affected.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index fb3fe74abfe6..82412f54c432 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -231,6 +231,7 @@ struct ar9331_sw_port {
 	int idx;
 	struct delayed_work mib_read;
 	struct rtnl_link_stats64 stats;
+	struct ethtool_pause_stats pause_stats;
 	struct spinlock stats_lock;
 };
 
@@ -606,6 +607,7 @@ static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
 static void ar9331_read_stats(struct ar9331_sw_port *port)
 {
 	struct ar9331_sw_priv *priv = ar9331_sw_port_to_priv(port);
+	struct ethtool_pause_stats *pstats = &port->pause_stats;
 	struct rtnl_link_stats64 *stats = &port->stats;
 	struct ar9331_sw_stats_raw raw;
 	int ret;
@@ -625,9 +627,11 @@ static void ar9331_read_stats(struct ar9331_sw_port *port)
 	stats->tx_bytes += raw.txbyte;
 
 	stats->rx_packets += raw.rx64byte + raw.rx128byte + raw.rx256byte +
-		raw.rx512byte + raw.rx1024byte + raw.rx1518byte + raw.rxmaxbyte;
+		raw.rx512byte + raw.rx1024byte + raw.rx1518byte +
+		raw.rxmaxbyte - raw.rxpause;
 	stats->tx_packets += raw.tx64byte + raw.tx128byte + raw.tx256byte +
-		raw.tx512byte + raw.tx1024byte + raw.tx1518byte + raw.txmaxbyte;
+		raw.tx512byte + raw.tx1024byte + raw.tx1518byte +
+		raw.txmaxbyte - raw.txpause;
 
 	stats->rx_length_errors += raw.rxrunt + raw.rxfragment + raw.rxtoolong;
 	stats->rx_crc_errors += raw.rxfcserr;
@@ -646,6 +650,9 @@ static void ar9331_read_stats(struct ar9331_sw_port *port)
 	stats->multicast += raw.rxmulti;
 	stats->collisions += raw.txcollision;
 
+	pstats->tx_pause_frames += raw.txpause;
+	pstats->rx_pause_frames += raw.rxpause;
+
 	spin_unlock(&port->stats_lock);
 }
 
@@ -670,6 +677,17 @@ static void ar9331_get_stats64(struct dsa_switch *ds, int port,
 	spin_unlock(&p->stats_lock);
 }
 
+static void ar9331_get_pause_stats(struct dsa_switch *ds, int port,
+				   struct ethtool_pause_stats *pause_stats)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct ar9331_sw_port *p = &priv->port[port];
+
+	spin_lock(&p->stats_lock);
+	memcpy(pause_stats, &p->pause_stats, sizeof(*pause_stats));
+	spin_unlock(&p->stats_lock);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -679,6 +697,7 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.phylink_mac_link_down	= ar9331_sw_phylink_mac_link_down,
 	.phylink_mac_link_up	= ar9331_sw_phylink_mac_link_up,
 	.get_stats64		= ar9331_get_stats64,
+	.get_pause_stats	= ar9331_get_pause_stats,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.30.2

[PATCH net-next v1 3/3] net: dsa: microchip: add pause stats support

[Oleksij Rempel]

Add support for pause specific stats.

Tested on ksz9477.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz9477.c    |  1 +
 drivers/net/dsa/microchip/ksz_common.c | 19 +++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  3 +++
 3 files changed, 23 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index ab40b700cf1a..8c31fb0fe1fd 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1351,6 +1351,7 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
 	.port_mirror_add	= ksz9477_port_mirror_add,
 	.port_mirror_del	= ksz9477_port_mirror_del,
 	.get_stats64		= ksz_get_stats64,
+	.get_pause_stats	= ksz_get_pause_stats,
 	.port_change_mtu	= ksz9477_change_mtu,
 	.port_max_mtu		= ksz9477_max_mtu,
 };
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 9ca8c8d7740f..1022affb45aa 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -461,12 +461,14 @@ EXPORT_SYMBOL_GPL(ksz_phylink_get_caps);
 
 void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 {
+	struct ethtool_pause_stats *pstats;
 	struct rtnl_link_stats64 *stats;
 	struct ksz_stats_raw *raw;
 	struct ksz_port_mib *mib;
 
 	mib = &dev->ports[port].mib;
 	stats = &mib->stats64;
+	pstats = &mib->pause_stats;
 	raw = (struct ksz_stats_raw *)mib->counters;
 
 	spin_lock(&mib->stats64_lock);
@@ -498,6 +500,9 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 	stats->multicast = raw->rx_mcast;
 	stats->collisions = raw->tx_total_col;
 
+	pstats->tx_pause_frames = raw->tx_pause;
+	pstats->rx_pause_frames = raw->rx_pause;
+
 	spin_unlock(&mib->stats64_lock);
 }
 EXPORT_SYMBOL_GPL(ksz_r_mib_stats64);
@@ -516,6 +521,20 @@ void ksz_get_stats64(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL_GPL(ksz_get_stats64);
 
+void ksz_get_pause_stats(struct dsa_switch *ds, int port,
+			 struct ethtool_pause_stats *pause_stats)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port_mib *mib;
+
+	mib = &dev->ports[port].mib;
+
+	spin_lock(&mib->stats64_lock);
+	memcpy(pause_stats, &mib->pause_stats, sizeof(*pause_stats));
+	spin_unlock(&mib->stats64_lock);
+}
+EXPORT_SYMBOL_GPL(ksz_get_pause_stats);
+
 void ksz_get_strings(struct dsa_switch *ds, int port,
 		     u32 stringset, uint8_t *buf)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 8500eaedad67..5d81faf81c1b 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -25,6 +25,7 @@ struct ksz_port_mib {
 	u8 cnt_ptr;
 	u64 *counters;
 	struct rtnl_link_stats64 stats64;
+	struct ethtool_pause_stats pause_stats;
 	struct spinlock stats64_lock;
 };
 
@@ -200,6 +201,8 @@ void ksz_init_mib_timer(struct ksz_device *dev);
 void ksz_r_mib_stats64(struct ksz_device *dev, int port);
 void ksz_get_stats64(struct dsa_switch *ds, int port,
 		     struct rtnl_link_stats64 *s);
+void ksz_get_pause_stats(struct dsa_switch *ds, int port,
+			 struct ethtool_pause_stats *pause_stats);
 void ksz_phylink_get_caps(struct dsa_switch *ds, int port,
 			  struct phylink_config *config);
 extern const struct ksz_chip_data ksz_switch_chips[];
-- 
2.30.2

Re: [PATCH net-next v1 3/3] net: dsa: microchip: add pause stats support

[Vladimir Oltean]

Hi Oleksij,

On Fri, Jun 24, 2022 at 02:59:02PM +0200, Oleksij Rempel wrote:
> Add support for pause specific stats.
> 
> Tested on ksz9477.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

This conflicts with Arun's work:

Applying: net: dsa: add get_pause_stats support
Applying: net: dsa: ar9331: add support for pause stats
Applying: net: dsa: microchip: add pause stats support
Using index info to reconstruct a base tree...
M       drivers/net/dsa/microchip/ksz9477.c
M       drivers/net/dsa/microchip/ksz_common.c
M       drivers/net/dsa/microchip/ksz_common.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/net/dsa/microchip/ksz_common.h
CONFLICT (content): Merge conflict in drivers/net/dsa/microchip/ksz_common.h
Auto-merging drivers/net/dsa/microchip/ksz_common.c
CONFLICT (content): Merge conflict in drivers/net/dsa/microchip/ksz_common.c
Auto-merging drivers/net/dsa/microchip/ksz9477.c
CONFLICT (content): Merge conflict in drivers/net/dsa/microchip/ksz9477.c
error: Failed to merge in the changes.
Patch failed at 0003 net: dsa: microchip: add pause stats support

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Vladimir Oltean]

On Fri, Jun 24, 2022 at 02:59:01PM +0200, Oleksij Rempel wrote:
> Add support for pause stats and fix rx_packets/tx_packets calculation.
> 
> Pause packets are counted by raw.rx64byte/raw.tx64byte counters, so
> subtract it from main rx_packets/tx_packets counters.
> 
> tx_/rx_bytes are not affected.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index fb3fe74abfe6..82412f54c432 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -606,6 +607,7 @@ static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
>  static void ar9331_read_stats(struct ar9331_sw_port *port)
>  {
>  	struct ar9331_sw_priv *priv = ar9331_sw_port_to_priv(port);
> +	struct ethtool_pause_stats *pstats = &port->pause_stats;
>  	struct rtnl_link_stats64 *stats = &port->stats;
>  	struct ar9331_sw_stats_raw raw;
>  	int ret;
> @@ -625,9 +627,11 @@ static void ar9331_read_stats(struct ar9331_sw_port *port)
>  	stats->tx_bytes += raw.txbyte;
>  
>  	stats->rx_packets += raw.rx64byte + raw.rx128byte + raw.rx256byte +
> -		raw.rx512byte + raw.rx1024byte + raw.rx1518byte + raw.rxmaxbyte;
> +		raw.rx512byte + raw.rx1024byte + raw.rx1518byte +
> +		raw.rxmaxbyte - raw.rxpause;
>  	stats->tx_packets += raw.tx64byte + raw.tx128byte + raw.tx256byte +
> -		raw.tx512byte + raw.tx1024byte + raw.tx1518byte + raw.txmaxbyte;
> +		raw.tx512byte + raw.tx1024byte + raw.tx1518byte +
> +		raw.txmaxbyte - raw.txpause;

Is there an authoritative source who is able to tell whether rtnl_link_stats64 ::
rx_packets and tx_packets should count PAUSE frames or not?

>  
>  	stats->rx_length_errors += raw.rxrunt + raw.rxfragment + raw.rxtoolong;
>  	stats->rx_crc_errors += raw.rxfcserr;
> @@ -646,6 +650,9 @@ static void ar9331_read_stats(struct ar9331_sw_port *port)
>  	stats->multicast += raw.rxmulti;
>  	stats->collisions += raw.txcollision;
>  
> +	pstats->tx_pause_frames += raw.txpause;
> +	pstats->rx_pause_frames += raw.rxpause;
> +
>  	spin_unlock(&port->stats_lock);
>  }

Re: [PATCH net-next v1 1/3] net: dsa: add get_pause_stats support

[Vladimir Oltean]

On Fri, Jun 24, 2022 at 02:59:00PM +0200, Oleksij Rempel wrote:
> Add support for pause stats
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Oleksij Rempel]

On Sat, Jun 25, 2022 at 01:03:17AM +0300, Vladimir Oltean wrote:
> On Fri, Jun 24, 2022 at 02:59:01PM +0200, Oleksij Rempel wrote:
> > Add support for pause stats and fix rx_packets/tx_packets calculation.
> > 
> > Pause packets are counted by raw.rx64byte/raw.tx64byte counters, so
> > subtract it from main rx_packets/tx_packets counters.
> > 
> > tx_/rx_bytes are not affected.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/qca/ar9331.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> > index fb3fe74abfe6..82412f54c432 100644
> > --- a/drivers/net/dsa/qca/ar9331.c
> > +++ b/drivers/net/dsa/qca/ar9331.c
> > @@ -606,6 +607,7 @@ static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
> >  static void ar9331_read_stats(struct ar9331_sw_port *port)
> >  {
> >  	struct ar9331_sw_priv *priv = ar9331_sw_port_to_priv(port);
> > +	struct ethtool_pause_stats *pstats = &port->pause_stats;
> >  	struct rtnl_link_stats64 *stats = &port->stats;
> >  	struct ar9331_sw_stats_raw raw;
> >  	int ret;
> > @@ -625,9 +627,11 @@ static void ar9331_read_stats(struct ar9331_sw_port *port)
> >  	stats->tx_bytes += raw.txbyte;
> >  
> >  	stats->rx_packets += raw.rx64byte + raw.rx128byte + raw.rx256byte +
> > -		raw.rx512byte + raw.rx1024byte + raw.rx1518byte + raw.rxmaxbyte;
> > +		raw.rx512byte + raw.rx1024byte + raw.rx1518byte +
> > +		raw.rxmaxbyte - raw.rxpause;
> >  	stats->tx_packets += raw.tx64byte + raw.tx128byte + raw.tx256byte +
> > -		raw.tx512byte + raw.tx1024byte + raw.tx1518byte + raw.txmaxbyte;
> > +		raw.tx512byte + raw.tx1024byte + raw.tx1518byte +
> > +		raw.txmaxbyte - raw.txpause;
> 
> Is there an authoritative source who is able to tell whether rtnl_link_stats64 ::
> rx_packets and tx_packets should count PAUSE frames or not?

Yes, it will be interesting to know how to proceed with it. For example
KSZ switch do count pause frame Bytes together will other frames. At
same time, atheros switch do not count pause frame bytes at all.

To make things worse, i can manually send pause frame of any size, so it
will not be accounted by HW. What ever decision we will made, i will
need to calculate typical pause frame size and hope it will fit for 90%
of cases.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Jakub Kicinski]

On Sun, 26 Jun 2022 19:10:08 +0200 Oleksij Rempel wrote:
> > Is there an authoritative source who is able to tell whether rtnl_link_stats64 ::
> > rx_packets and tx_packets should count PAUSE frames or not?  
> 
> Yes, it will be interesting to know how to proceed with it.

I'm curious as well, AFAIK most drivers do not count pause to ifc stats.

> For example KSZ switch do count pause frame Bytes together will other
> frames. At same time, atheros switch do not count pause frame bytes
> at all.
> 
> To make things worse, i can manually send pause frame of any size, so
> it will not be accounted by HW. What ever decision we will made, i
> will need to calculate typical pause frame size and hope it will fit
> for 90% of cases.

:(

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Vladimir Oltean]

On Mon, Jun 27, 2022 at 09:15:21AM -0700, Jakub Kicinski wrote:
> On Sun, 26 Jun 2022 19:10:08 +0200 Oleksij Rempel wrote:
> > > Is there an authoritative source who is able to tell whether rtnl_link_stats64 ::
> > > rx_packets and tx_packets should count PAUSE frames or not?  
> > 
> > Yes, it will be interesting to know how to proceed with it.
> 
> I'm curious as well, AFAIK most drivers do not count pause to ifc stats.

How do you know? Just because they manually bump stats->tx_bytes and
stats->tx_packets during ndo_start_xmit?

That would be a good assumption, but what if a network driver populates
struct rtnl_link_stats64 entirely based on counters reported by hardware,
including {rx,tx}_{packets,bytes}?

Personally I can't really find a reason why not count pause frames if
you can. And in the same note, why go to the extra lengths of hiding
them as Oleksij does. For example, the ocelot/felix switches do count
PAUSE frames as packets/bytes, both on rx and tx.

> > For example KSZ switch do count pause frame Bytes together will other
> > frames. At same time, atheros switch do not count pause frame bytes
> > at all.
> > 
> > To make things worse, i can manually send pause frame of any size, so
> > it will not be accounted by HW. What ever decision we will made, i
> > will need to calculate typical pause frame size and hope it will fit
> > for 90% of cases.
> 
> :(

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Jakub Kicinski]

On Mon, 27 Jun 2022 23:02:38 +0300 Vladimir Oltean wrote:
> > > Yes, it will be interesting to know how to proceed with it.  
> > 
> > I'm curious as well, AFAIK most drivers do not count pause to ifc stats.  
> 
> How do you know? Just because they manually bump stats->tx_bytes and
> stats->tx_packets during ndo_start_xmit?
> 
> That would be a good assumption, but what if a network driver populates
> struct rtnl_link_stats64 entirely based on counters reported by hardware,
> including {rx,tx}_{packets,bytes}?

Yeah, a lot of drivers use SW stats. What matters is where the packets
get counted, even if device does the counting it may be in/before or
after the MAC. Modern NICs generally don't use MAC-level stats for the
interface because of virtualization.

> Personally I can't really find a reason why not count pause frames if
> you can. And in the same note, why go to the extra lengths of hiding
> them as Oleksij does. For example, the ocelot/felix switches do count
> PAUSE frames as packets/bytes, both on rx and tx.

Yeah, the corrections are always iffy. I understand the doubts, and we
can probably leave things "under-specified" until someone with a strong
preference comes along. But I hope that the virt example makes it clear
that neither of the choices is better (SR-IOV NICs would have to start
adding the pause if we declare rtnl stats as inclusive).

I can see advantages to both counting (they are packets) and not
counting those frames (Linux doesn't see them, they get "invented" 
by HW).

Stats are hard.

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Andrew Lunn]

> Yeah, the corrections are always iffy. I understand the doubts, and we
> can probably leave things "under-specified" until someone with a strong
> preference comes along. But I hope that the virt example makes it clear
> that neither of the choices is better (SR-IOV NICs would have to start
> adding the pause if we declare rtnl stats as inclusive).
> 
> I can see advantages to both counting (they are packets) and not
> counting those frames (Linux doesn't see them, they get "invented" 
> by HW).
> 
> Stats are hard.

I doubt we can define it either way. I once submitted a patch for one
driver to make it ignore CRC bytes. It then gave the exact same counts
as another hardware i was using, making the testing i was doing
simpler.

The patch got rejected simply because we have both, with CRC and
without CRC, neither is correct, neither is wrong.

So i would keep it KISS, pause frames can be included, but i would not
go to extra effort to include them, or to exclude them.

   Andrew

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Oleksij Rempel]

On Tue, Jun 28, 2022 at 09:22:53AM +0200, Andrew Lunn wrote:
> > Yeah, the corrections are always iffy. I understand the doubts, and we
> > can probably leave things "under-specified" until someone with a strong
> > preference comes along. But I hope that the virt example makes it clear
> > that neither of the choices is better (SR-IOV NICs would have to start
> > adding the pause if we declare rtnl stats as inclusive).
> > 
> > I can see advantages to both counting (they are packets) and not
> > counting those frames (Linux doesn't see them, they get "invented" 
> > by HW).
> > 
> > Stats are hard.
> 
> I doubt we can define it either way. I once submitted a patch for one
> driver to make it ignore CRC bytes. It then gave the exact same counts
> as another hardware i was using, making the testing i was doing
> simpler.
> 
> The patch got rejected simply because we have both, with CRC and
> without CRC, neither is correct, neither is wrong.
> 
> So i would keep it KISS, pause frames can be included, but i would not
> go to extra effort to include them, or to exclude them.

After I started investigating this topic, I was really frustrated. It is
has hard to find what is wrong: my patch is not working and flow
controller is not triggered? Or every HW/driver implements counters in
some own way. Same is about byte counts: for same packet with different
NICs i have at least 3 different results: 50, 64 and 68.
It makes testing and validation a nightmare. 

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Jakub Kicinski]

On Tue, 28 Jun 2022 10:45:04 +0200 Oleksij Rempel wrote:
> After I started investigating this topic, I was really frustrated. It is
> has hard to find what is wrong: my patch is not working and flow
> controller is not triggered? Or every HW/driver implements counters in
> some own way. Same is about byte counts: for same packet with different
> NICs i have at least 3 different results: 50, 64 and 68.
> It makes testing and validation a nightmare. 

Yeah, I was gonna mention QA in my reply. The very practical reason I've
gone no-CRC, no-flow control in the driver stats in the past was that it
made it possible to test the counters are correct and the match far end.
I mean SW matches HW, and they both match between sender/receiver
(testing NIC-switch-NIC if either link does flow control the counters
on NICs won't match).

Re: [PATCH net-next v1 2/3] net: dsa: ar9331: add support for pause stats

[Oleksij Rempel]

On Tue, Jun 28, 2022 at 09:10:27AM -0700, Jakub Kicinski wrote:
> On Tue, 28 Jun 2022 10:45:04 +0200 Oleksij Rempel wrote:
> > After I started investigating this topic, I was really frustrated. It is
> > has hard to find what is wrong: my patch is not working and flow
> > controller is not triggered? Or every HW/driver implements counters in
> > some own way. Same is about byte counts: for same packet with different
> > NICs i have at least 3 different results: 50, 64 and 68.
> > It makes testing and validation a nightmare. 
> 
> Yeah, I was gonna mention QA in my reply. The very practical reason I've
> gone no-CRC, no-flow control in the driver stats in the past was that it
> made it possible to test the counters are correct and the match far end.
> I mean SW matches HW, and they both match between sender/receiver
> (testing NIC-switch-NIC if either link does flow control the counters
> on NICs won't match).

Hm, may be it make sense to provide extra information on what the HW
counters do actually count? For example set flags, caps, for HW counting
pause frames in the main counter. I do not know if there are other use
cases where data is transferred but not counted except for FCS.

In case someone will hit a switch counting pause frames (like KSZ9477 do),
it will be better to know about it from user space. Instead of making
source code archeology.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |