Messages in this thread | | | Subject | Re: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics | From | Florian Fainelli <> | Date | Mon, 13 Jan 2020 10:00:23 -0800 |
| |
Hi Andrew,
On 1/13/20 5:21 AM, Andrew Lunn wrote: > On Sun, Jan 12, 2020 at 08:53:19PM -0800, Florian Fainelli wrote: >> Maintain per MDIO device and MDIO bus statistics comprised of the number >> of transfers/operations, reads and writes and errors. This is useful for >> tracking the per-device and global MDIO bus bandwidth and doing >> optimizations as necessary. > > Hi Florian > > One point for discussion is, is sysfs the right way to do this? > Should we be using ethtool and exporting the statistics like other > statistics? > > The argument against it, is we have devices which are not related to a > network interfaces on MDIO busses. For a PHY we could plumb the per > PHY mdio device statistics into the exiting PHY statistics. But we > also have Ethernet switches on MDIO devices, which don't have an > association to a netdev interface. Broadcom also have some generic PHY > device on MDIO busses, for USB, SATA, etc. And whole bus statistics > don't fit the netdev model at all.
Correct, that was the reasoning, which I should probably put in the commit message.
> > So sysfs does make sense. But i would also suggest we do plumb per PHY > MDIO device statistics into the exiting ethtool call.
It looks like replicating statistics that are already available via another mechanism is kind of frowned upon, see this for an example:
> >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> Documentation/ABI/testing/sysfs-bus-mdio | 34 +++++++ >> drivers/net/phy/mdio_bus.c | 116 +++++++++++++++++++++++ >> drivers/net/phy/mdio_device.c | 1 + >> include/linux/mdio.h | 10 ++ >> include/linux/phy.h | 2 + >> 5 files changed, 163 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-bus-mdio >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-mdio b/Documentation/ABI/testing/sysfs-bus-mdio >> new file mode 100644 >> index 000000000000..a552d92890f1 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-bus-mdio >> @@ -0,0 +1,34 @@ >> +What: /sys/bus/mdio_bus/devices/.../statistics/ >> +Date: January 2020 >> +KernelVersion: 5.6 >> +Contact: netdev@vger.kernel.org >> +Description: >> + This folder contains statistics about MDIO bus transactions. >> + >> +What: /sys/bus/mdio_bus/devices/.../statistics/transfers >> +Date: January 2020 >> +KernelVersion: 5.6 >> +Contact: netdev@vger.kernel.org >> +Description: >> + Total number of transfers for this MDIO bus. >> + >> +What: /sys/bus/mdio_bus/devices/.../statistics/errors >> +Date: January 2020 >> +KernelVersion: 5.6 >> +Contact: netdev@vger.kernel.org >> +Description: >> + Total number of transfer errors for this MDIO bus. >> + >> +What: /sys/bus/mdio_bus/devices/.../statistics/writes >> +Date: January 2020 >> +KernelVersion: 5.6 >> +Contact: netdev@vger.kernel.org >> +Description: >> + Total number of write transactions for this MDIO bus. >> + >> +What: /sys/bus/mdio_bus/devices/.../statistics/reads >> +Date: January 2020 >> +KernelVersion: 5.6 >> +Contact: netdev@vger.kernel.org >> +Description: >> + Total number of read transactions for this MDIO bus. > > Looking at this description, it is not clear we have whole bus and per > device statistics. > >> int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum) >> { >> + struct mdio_device *mdiodev = bus->mdio_map[addr]; >> int retval; >> >> WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock)); >> @@ -555,6 +645,9 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum) >> retval = bus->read(bus, addr, regnum); >> >> trace_mdio_access(bus, 1, addr, regnum, retval, retval); >> + mdiobus_stats_acct(&bus->stats, true, retval); >> + if (mdiodev) >> + mdiobus_stats_acct(&mdiodev->stats, true, retval); >> >> return retval; > > I think for most Ethernet switches, these per device counters are > going to be misleading. The switch often takes up multiple addresses > on the bus, but the switch is represented as a single mdiodev with one > address.
For MDIO switches you would usually have the mdio_device claim the pseudo PHY address and all other MDIO addresses should correspond to built-in PHYs, for which we also have mdio_device instances, is there a case that I am missing?
> So the counters will reflect the transfers on that one > address, not the whole switch. The device tree binding does not have > enough information for us to associated one mdiodev to multiple > addresses. And for some of the Marvell switches, it is a sparse address > map, and i have seen PHY devices in the holes. So in the sysfs > documentation, we should probably add a warning that when used with an > Ethernet switch, the counters are unlikely to be accurate, and should > be interpreted with care.
If the answer to my question above is that we still have reads to addresses for which we do not have mdio_device (which we might very well have), then we could either:
- create <mdio_bus>:<address>/statistics/ folders even for non-existent devices, but just to track the per-address statistics - create <mdio_bus>/<address>/statistics and when a mdio_device instance exists we symbolic link <mdio_bus>:<address>/statistics -> ../<mdio_bus>/<addr>/statistics
Would that work? -- Florian
| |