lkml.org 
[lkml]   [2020]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics
From
Date
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

\
 
 \ /
  Last update: 2020-01-13 19:00    [W:0.103 / U:0.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site