| Message ID | 20221205052904.2834962-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bde55dd9ccda7a3f5f735d89e855691362745248 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1,1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Dec 05, 2022 at 06:29:04AM +0100, Oleksij Rempel wrote: > +void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port) > +{ > + struct ethtool_pause_stats *pstats; > + struct rtnl_link_stats64 *stats; > + struct ksz88xx_stats_raw *raw; > + struct ksz_port_mib *mib; > + > + mib = &dev->ports[port].mib; > + stats = &mib->stats64; > + pstats = &mib->pause_stats; > + raw = (struct ksz88xx_stats_raw *)mib->counters; > + > + spin_lock(&mib->stats64_lock); > + > + stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast + > + raw->rx_pause; > + stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast + > + raw->tx_pause; > + > + /* HW counters are counting bytes + FCS which is not acceptable > + * for rtnl_link_stats64 interface > + */ > + stats->rx_bytes = raw->rx + raw->rx_hi - stats->rx_packets * ETH_FCS_LEN; > + stats->tx_bytes = raw->tx + raw->tx_hi - stats->tx_packets * ETH_FCS_LEN; What are rx_hi, tx_hi compared to rx, tx? > + > + stats->rx_length_errors = raw->rx_undersize + raw->rx_fragments + > + raw->rx_oversize; > + > + stats->rx_crc_errors = raw->rx_crc_err; > + stats->rx_frame_errors = raw->rx_align_err; > + stats->rx_dropped = raw->rx_discards; > + stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors + > + stats->rx_frame_errors + stats->rx_dropped; > + > + stats->tx_window_errors = raw->tx_late_col; > + stats->tx_fifo_errors = raw->tx_discards; > + stats->tx_aborted_errors = raw->tx_exc_col; > + stats->tx_errors = stats->tx_window_errors + stats->tx_fifo_errors + > + stats->tx_aborted_errors; > + > + 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; FWIW, ksz_get_pause_stats() can sleep, just ksz_get_stats64() can't. So the pause stats don't need to be periodically read (unless you want to do that to prevent 32-bit overflows). > + > + spin_unlock(&mib->stats64_lock); > +}
On Mon, 5 Dec 2022 06:29:04 +0100 Oleksij Rempel wrote: > + stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast + > + raw->rx_pause; > + stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast + > + raw->tx_pause; FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include pause frames, normally. Otherwise one can't maintain those stats in SW (and per-ring stats, if any, don't add up to the full link stats). But if you have a good reason to do this - I won't nack..
On Tue, Dec 06, 2022 at 07:08:01PM +0200, Vladimir Oltean wrote: > On Mon, Dec 05, 2022 at 06:29:04AM +0100, Oleksij Rempel wrote: > > +void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port) > > +{ > > + struct ethtool_pause_stats *pstats; > > + struct rtnl_link_stats64 *stats; > > + struct ksz88xx_stats_raw *raw; > > + struct ksz_port_mib *mib; > > + > > + mib = &dev->ports[port].mib; > > + stats = &mib->stats64; > > + pstats = &mib->pause_stats; > > + raw = (struct ksz88xx_stats_raw *)mib->counters; > > + > > + spin_lock(&mib->stats64_lock); > > + > > + stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast + > > + raw->rx_pause; > > + stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast + > > + raw->tx_pause; > > + > > + /* HW counters are counting bytes + FCS which is not acceptable > > + * for rtnl_link_stats64 interface > > + */ > > + stats->rx_bytes = raw->rx + raw->rx_hi - stats->rx_packets * ETH_FCS_LEN; > > + stats->tx_bytes = raw->tx + raw->tx_hi - stats->tx_packets * ETH_FCS_LEN; > > What are rx_hi, tx_hi compared to rx, tx? rx, tx are packets with normal priority and rx_hi, tx_hi are packets with high prio. > > + > > + stats->rx_length_errors = raw->rx_undersize + raw->rx_fragments + > > + raw->rx_oversize; > > + > > + stats->rx_crc_errors = raw->rx_crc_err; > > + stats->rx_frame_errors = raw->rx_align_err; > > + stats->rx_dropped = raw->rx_discards; > > + stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors + > > + stats->rx_frame_errors + stats->rx_dropped; > > + > > + stats->tx_window_errors = raw->tx_late_col; > > + stats->tx_fifo_errors = raw->tx_discards; > > + stats->tx_aborted_errors = raw->tx_exc_col; > > + stats->tx_errors = stats->tx_window_errors + stats->tx_fifo_errors + > > + stats->tx_aborted_errors; > > + > > + 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; > > FWIW, ksz_get_pause_stats() can sleep, just ksz_get_stats64() can't. So > the pause stats don't need to be periodically read (unless you want to > do that to prevent 32-bit overflows). KSZ driver is using worker to read stats periodically. Since all needed locks are already taken, I copy pause stats as well. Otherwise it will need some different locking, which will make things look different but do not reduce CPU load. Regards, Oleksij
On Tue, Dec 06, 2022 at 11:41:33AM -0800, Jakub Kicinski wrote: > On Mon, 5 Dec 2022 06:29:04 +0100 Oleksij Rempel wrote: > > + stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast + > > + raw->rx_pause; > > + stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast + > > + raw->tx_pause; > > FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include > pause frames, normally. Otherwise one can't maintain those stats in SW > (and per-ring stats, if any, don't add up to the full link stats). > But if you have a good reason to do this - I won't nack.. Pause frames are accounted by rx/tx_bytes by HW. Since pause frames may have different size, it is not possible to correct byte counters, so I need to add them to the packet counters. Regards, Oleksij
On Wed, 7 Dec 2022 07:16:30 +0100 Oleksij Rempel wrote: > > FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include > > pause frames, normally. Otherwise one can't maintain those stats in SW > > (and per-ring stats, if any, don't add up to the full link stats). > > But if you have a good reason to do this - I won't nack.. > > Pause frames are accounted by rx/tx_bytes by HW. Since pause frames may > have different size, it is not possible to correct byte counters, so I > need to add them to the packet counters. I have embarrassed myself with my lack of understanding of pause frames before but nonetheless - are you sure? I thought they are always 64B. Quick look at the standard seems to agree: 31C.3.1 Receive state diagram (INITIATE MAC CONTROL FUNCTION) for EXTENSION operation shows a 64 octet frame. Sending long pause frames seems self-defeating as we presumably want the receiver to react ASAP.
On Wed, Dec 07, 2022 at 03:48:26PM -0800, Jakub Kicinski wrote: > On Wed, 7 Dec 2022 07:16:30 +0100 Oleksij Rempel wrote: > > > FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include > > > pause frames, normally. Otherwise one can't maintain those stats in SW > > > (and per-ring stats, if any, don't add up to the full link stats). > > > But if you have a good reason to do this - I won't nack.. > > > > Pause frames are accounted by rx/tx_bytes by HW. Since pause frames may > > have different size, it is not possible to correct byte counters, so I > > need to add them to the packet counters. > > I have embarrassed myself with my lack of understanding of pause frames > before but nonetheless - are you sure? I thought they are always 64B. > Quick look at the standard seems to agree: > > 31C.3.1 Receive state diagram (INITIATE MAC CONTROL FUNCTION) for > EXTENSION operation > > shows a 64 octet frame. > > Sending long pause frames seems self-defeating as we presumably want > the receiver to react ASAP. I tested it by sending correct and malformed pause frames manually with mausezahn. Since it is possible to send and receive pause frames manually, it is good to count all bytes in use, otherwise we may have bogus or malicious stealth traffic without possibility to measure it. Regards, Oleksij
On Thu, 8 Dec 2022 06:55:12 +0100 Oleksij Rempel wrote: > I tested it by sending correct and malformed pause frames manually with > mausezahn. Since it is possible to send and receive pause frames > manually, it is good to count all bytes in use, otherwise we may have > bogus or malicious stealth traffic without possibility to measure it.
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Mon, 5 Dec 2022 06:29:04 +0100 you wrote: > Add stats64 support for ksz8xxx series of switches. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz_common.c | 87 ++++++++++++++++++++++++++ > drivers/net/dsa/microchip/ksz_common.h | 1 + > 2 files changed, 88 insertions(+) Here is the summary with links: - [net-next,v1,1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches https://git.kernel.org/netdev/net-next/c/bde55dd9ccda You are awesome, thank you!
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index f39b041765fb..423f944cc34c 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -70,6 +70,43 @@ struct ksz_stats_raw { u64 tx_discards; }; +struct ksz88xx_stats_raw { + u64 rx; + u64 rx_hi; + u64 rx_undersize; + u64 rx_fragments; + u64 rx_oversize; + u64 rx_jabbers; + u64 rx_symbol_err; + u64 rx_crc_err; + u64 rx_align_err; + u64 rx_mac_ctrl; + u64 rx_pause; + u64 rx_bcast; + u64 rx_mcast; + u64 rx_ucast; + u64 rx_64_or_less; + u64 rx_65_127; + u64 rx_128_255; + u64 rx_256_511; + u64 rx_512_1023; + u64 rx_1024_1522; + u64 tx; + u64 tx_hi; + u64 tx_late_col; + u64 tx_pause; + u64 tx_bcast; + u64 tx_mcast; + u64 tx_ucast; + u64 tx_deferred; + u64 tx_total_col; + u64 tx_exc_col; + u64 tx_single_col; + u64 tx_mult_col; + u64 rx_discards; + u64 tx_discards; +}; + static const struct ksz_mib_names ksz88xx_mib_names[] = { { 0x00, "rx" }, { 0x01, "rx_hi" }, @@ -156,6 +193,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = { .w_phy = ksz8_w_phy, .r_mib_cnt = ksz8_r_mib_cnt, .r_mib_pkt = ksz8_r_mib_pkt, + .r_mib_stat64 = ksz88xx_r_mib_stats64, .freeze_mib = ksz8_freeze_mib, .port_init_cnt = ksz8_port_init_cnt, .fdb_dump = ksz8_fdb_dump, @@ -1583,6 +1621,55 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port) spin_unlock(&mib->stats64_lock); } +void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port) +{ + struct ethtool_pause_stats *pstats; + struct rtnl_link_stats64 *stats; + struct ksz88xx_stats_raw *raw; + struct ksz_port_mib *mib; + + mib = &dev->ports[port].mib; + stats = &mib->stats64; + pstats = &mib->pause_stats; + raw = (struct ksz88xx_stats_raw *)mib->counters; + + spin_lock(&mib->stats64_lock); + + stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast + + raw->rx_pause; + stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast + + raw->tx_pause; + + /* HW counters are counting bytes + FCS which is not acceptable + * for rtnl_link_stats64 interface + */ + stats->rx_bytes = raw->rx + raw->rx_hi - stats->rx_packets * ETH_FCS_LEN; + stats->tx_bytes = raw->tx + raw->tx_hi - stats->tx_packets * ETH_FCS_LEN; + + stats->rx_length_errors = raw->rx_undersize + raw->rx_fragments + + raw->rx_oversize; + + stats->rx_crc_errors = raw->rx_crc_err; + stats->rx_frame_errors = raw->rx_align_err; + stats->rx_dropped = raw->rx_discards; + stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors + + stats->rx_frame_errors + stats->rx_dropped; + + stats->tx_window_errors = raw->tx_late_col; + stats->tx_fifo_errors = raw->tx_discards; + stats->tx_aborted_errors = raw->tx_exc_col; + stats->tx_errors = stats->tx_window_errors + stats->tx_fifo_errors + + stats->tx_aborted_errors; + + 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); +} + static void ksz_get_stats64(struct dsa_switch *ds, int port, struct rtnl_link_stats64 *s) { diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index cb27f5a180c7..055d61ff3fb8 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -345,6 +345,7 @@ void ksz_switch_remove(struct ksz_device *dev); void ksz_init_mib_timer(struct ksz_device *dev); void ksz_r_mib_stats64(struct ksz_device *dev, int port); +void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port); void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state); bool ksz_get_gbit(struct ksz_device *dev, int port); phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
Add stats64 support for ksz8xxx series of switches. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz_common.c | 87 ++++++++++++++++++++++++++ drivers/net/dsa/microchip/ksz_common.h | 1 + 2 files changed, 88 insertions(+)