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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
> }
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 <[email protected]>
> > ---
> > 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 |
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.
:(
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.
>
> :(
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.
> 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
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 |
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).
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 |