Message-ID: <87sf4e8htq.fsf@waldekranz.com>
Date: Thu, 07 Dec 2023 10:47:45 +0100
From: Tobias Waldekranz <tobias@...dekranz.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
 f.fainelli@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter
 group support

On ons, dec 06, 2023 at 21:55, Vladimir Oltean <olteanv@...il.com> wrote:
> On Wed, Dec 06, 2023 at 09:27:29AM +0100, Tobias Waldekranz wrote:
>> > I just noticed that this doesn't populate the TX counters, just RX.
>> >
>> > I haven't tried it, but I think the Histogram Mode bits (11:10) of the
>> > Stats Operation Register might be able to control what gets reported for
>> > the Set 4 of counters. Currently AFAICS, the driver always sets it to
>> > MV88E6XXX_G1_STATS_OP_HIST_RX_TX, aka what gets reported to
>> > "rx-rmon-etherStatsPkts64to64Octets" is actually an RX+TX counter.
>> 
>> You have a keen eye! Yes, that is what's happening.
>
> It would be nice if my failure-prone keen eye had the safety net of a
> selftest that catches this kind of stuff. After all, the ethtool
> counters were standardized in order for us to be able to expect standard
> behavior out of them, and for nonconformities to stand out easily.
>
> Do you think (bearing in mind that the questions below might make the
> rest irrelevant) that you could look into creating a minimal test in
> tools/testing/selftests/net/forwarding and symlinking it to
> tools/testing/selftests/drivers/net/dsa? You can start from
> ethtool_std_stats_get() and take inspiration from the way in which it is
> used by ethtool_mm.sh.

I'll give it the old college try.

>> > What's the story behind this?
>> 
>> I think the story starts, and ends, with this value being the hardware
>> default.
>
> I do hope that is where the story actually ends.
>
> But the 88E6097 documentation I have suggests that the Histogram Mode
> bits are reserved to the value of 3 (RX+TX), which suggests that this
> cannot be written to any other value.

I'm pretty sure that is just typo in the documentation. Every other
field in the documentation marked "RES" has the description "Reserved
for future use" - except for this one. My guess is that the author meant
to type "RWS to 0x3", since that is the convetion for all other
multi-bit fields with a non-zero reset value. W is right next to E on
most keyboards, which makes it even more likely.

In section 4.3.8, we find this:

    The counters are designed to support:
    - RFC 2819 – RMON MIB (this RFC obsoletes 1757 which obsoletes 1271)
    ....

At the bottom of that page there is this note for "Set 4" (which is the
histogram group):

    **Note**
    The Set 4 counters can be configured to be ingress only, egress
    only, or both.

These sentences have remained unchanged since at least 6095, up to and
including 6393X.

>> Seeing as the hardware only has a single set of histogram counters,
>
> "Seeing" means you tested this? calling chip->info->ops->stats_set_histogram()
> at runtime, and seeing if the previously hidden histogram counters are
> reset to zero, or if they show retroactively counted packets?

I've now tested it on a 6393X (the only chip I can get my hands on at
the moment). On this device, the bits have moved to Global1:0x1c, but
the description is the same. It behaves like the documentation would
suggest: There is a single set of histogram counters - the user gets to
choose if they collect stats from rx, tx, or both:

We start out with the default of counting rx+tx, we've seen 400 packets
before the test starts:

    root@...ix-06-01-00:~# mdio f212a2* mvls 0 g1:0x1c
    0x07c0
    root@...ix-06-01-00:~# ethtool -S x10 | grep hist_512
         hist_512_1023bytes: 400

Send 10 pings to our neighbor with a large payload, both rx and tx are
counted (20):

    root@...ix-06-01-00:~# ping -L ff02::1%x10 -s 512 -c 10 -i 0.1 -q
    PING ff02::1%x10(ff02::1%x10) 512 data bytes

    --- ff02::1%x10 ping statistics ---
    10 packets transmitted, 10 received, 0% packet loss, time 934ms
    rtt min/avg/max/mdev = 0.150/0.172/0.313/0.046 ms
    root@...ix-06-01-00:~# ethtool -S x10 | grep hist_512
         hist_512_1023bytes: 420

Limit to tx only:

    root@...ix-06-01-00:~# mdio f212a2* mvls 0 g1:0x1c 0x0740

Same test again now only increments the (same) counter by 10:

    root@...ix-06-01-00:~# ping -L ff02::1%x10 -s 512 -c 10 -i 0.1 -q
    PING ff02::1%x10(ff02::1%x10) 512 data bytes

    --- ff02::1%x10 ping statistics ---
    10 packets transmitted, 10 received, 0% packet loss, time 934ms
    rtt min/avg/max/mdev = 0.148/0.173/0.323/0.050 ms
    root@...ix-06-01-00:~# ethtool -S x10 | grep hist_512
         hist_512_1023bytes: 430


> I searched through the git logs, but it's not exactly clear that this
> was tried and doesn't work.
>
>> it seems to me like we have to prioritize between:
>> 
>> 1. Keeping Rx+Tx: Backwards-compatible, but we can't export any histogram via
>>    the standard RMON group.
>> 
>> 2. Move to Rx-only: We can export them via the RMON group, but we change
>>    the behavior of the "native" counters.
>> 
>> 3. Move to Tx-only: We can export them via the RMON group, but we change
>>    the behavior of the "native" counters.
>> 
>> Looking at RFC2819, which lays out the original RMON MIB, we find this
>> description:
>> 
>>     etherStatsPkts64Octets OBJECT-TYPE
>>         SYNTAX     Counter32
>>         UNITS      "Packets"
>>         MAX-ACCESS read-only
>>         STATUS     current
>>         DESCRIPTION
>>             "The total number of packets (including bad
>>             packets) received that were 64 octets in length
>>             (excluding framing bits but including FCS octets)."
>>         ::= { etherStatsEntry 14 }
>> 
>> In my opinion, this gives (2) a clear edge over (3), so we're down to
>> choosing between (1) and (2). Personally, I lean towards (2), as I think
>> it is more useful because:
>> 
>> - Most people will tend to assume that the histogram counters refers to
>>   those defined in RFC2819 anyway
>> 
>> - It means we can deliver _something_ rather than nothing to someone
>>   building an operating system, who is looking for a hardware
>>   independent way of providing diagnostics
>
> If the "reserved to 3" thing is true, then both (2) and (3) become
> practically non-options, at least for 88E6097. The waters would be
> further muddied if the driver were to make some chips use one
> Histogram Mode, and other chips a different one. It implies that as
> a user, you would need to know what switch family you have, before
> you know what "ethtool -S lan0 | grep hist_64bytes" is counting!

I agree that having different chips work differently would be a
nightmare. Especially since this could mean that "lan1" might behave
differently than "lan0" if they are attached to different chips on the
same system.

Fortunately though, it looks like (2) is still on the table. Do you
agree with that assessment? If yes, do you think is the right way
forward?

Andrew, what's your opinion?

Powered by blists - more mailing lists