netdev - Re: [PATCH net-next] net: tighten the definition of interface statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52ad841c-c51d-f606-09e3-e757fc0d193b@gmail.com>
Date:   Thu, 3 Sep 2020 13:48:39 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Edwin Peer <edwin.peer@...adcom.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Julian Wiedmann <jwi@...ux.ibm.com>, andrew@...n.ch,
        mkubecek@...e.cz, dsahern@...il.com,
        Michael Chan <michael.chan@...adcom.com>, saeedm@...lanox.com,
        rmk+kernel@...linux.org.uk
Subject: Re: [PATCH net-next] net: tighten the definition of interface
 statistics



On 9/3/2020 1:45 PM, Jakub Kicinski wrote:
> On Thu, 3 Sep 2020 09:29:22 -0700 Edwin Peer wrote:
>> On Wed, Sep 2, 2020 at 7:03 PM Jakub Kicinski <kuba@...nel.org> wrote:
>>> +Drivers should report all statistics which have a matching member in
>>> +:c:type:`struct rtnl_link_stats64 <rtnl_link_stats64>` exclusively
>>> +via `.ndo_get_stats64`. Reporting such standard stats via ethtool
>>> +or debugfs will not be accepted.
>>
>> Should existing drivers that currently duplicate standard stats in the
>> ethtool list be revised also?
> 
> That's probably considered uAPI land. I've removed the stats from the
> nfp awhile back, and nobody complained, but I'm thinking to leave the
> decision to individual maintainers.
> 
> Funnily enough number of 10G and 40G drivers report tx_heartbeat_errors
> in their ethtool stats (always as 0). Explainer what the statistic
> counts for a contemporary reader:
> 
> http://www.ethermanage.com/ethernet/sqe/sqe.html
> 
>>> + * @rx_packets: Number of good packets received by the interface.
>>> + *   For hardware interfaces counts all good packets seen by the host,
>>> + *   including packets which host had to drop at various stages of processing
>>> + *   (even in the driver).
>>
>> This is perhaps a bit ambiguous. I think you mean to say packets received from
>> the device, but I could also interpret the above to mean received by the device
>> if 'host' is read to be the whole physical machine (ie. including NIC hardware)
>> instead of the part that is apart from the NIC from the NIC's perspective.
> 
> How about:
> 
>    For hardware interfaces counts all good packets received from the
>    device by the host, including packets which host had to drop...
> 
>>> + * @rx_bytes: Number of good incoming bytes, corresponding to @rx_packets.
>>> + * @tx_bytes: Number of good incoming bytes, corresponding to @tx_packets.
>>
>> Including or excluding FCS?
> 
> Good point, no FCS is probably a reasonable expectation.
> 
> I'm not sure what to say about pad. I'm tempted to also mention that
> for tx we shouldn't count pad, no? (IOW Ethernet Frame - FCS - pad)

It depends I would say, if the driver needed to add padding in order to 
get the frame to be transmitted because the Ethernet MAC cannot pad 
automatically then it would seem natural to count the added padding. If 
you implement BQL that is what you will be reporting because that how 
much travels on the wire. What do you think?

> 
>>> + *   For Ethernet devices this counter may be equivalent to:
>>> + *
>>> + *    - 30.3.1.1.21 aMulticastFramesReceivedOK
>>
>> You mention the IEEE standard in your commit message, but I don't think this
>> document properly cites what you are referring to here? It might be an idea to
>> say "IEEE 30.3.1.1.21 aMulticastFramesReceivedOK" here and provide an
>> appropriate citation reference at the end, or perhaps a link.
> 
> How about I replace Ethernet with IEEE 802.3:
> 
>    For IEEE 802.3 devices this counter may be equivalent to:
> 
>     - 30.3.1.1.21 aMulticastFramesReceivedOK
> 

-- 
Florian