netdev - Re: [net 02/11] net/mlx5e: Fix multicast counter not up-to-date in "ip -s"

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]

Date:   Tue, 7 Jul 2020 01:51:21 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "kuba@...nel.org" <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...lanox.com>,
        Ron Diskin <rondi@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [net 02/11] net/mlx5e: Fix multicast counter not up-to-date in
 "ip -s"

On Mon, 2020-07-06 at 12:57 -0700, Jakub Kicinski wrote:
> On Mon, 6 Jul 2020 19:40:50 +0000 Saeed Mahameed wrote:
> > > I don't really feel too strongly, I'm just trying to get the
> > > details
> > > because I feel like the situation is going to be increasingly
> > > common.
> > > It'd be quite sad if drivers had to reimplement all stats in sw.
> > 
> > Depends on HW, our HW/FW supports providing stats per
> > (Vport/function).
> > which means if a packet got lost between the NIC and the netdev
> > queue,
> > it will be counted as rx-packet/mcast, although we have a private
> > counter to show this drop in ethtool but will be counted in rx
> > counter
> > in netdev stats, if we used hw stats.
> > 
> > so this is why i always prefer SW stats for netdev reported stats,
> > all
> > we need to count in SW {rx,tx} X {packets, bytes} + rx mcast
> > packets.
> 
> If that was indeed the intention it'd had been done in the core, not
> each driver separately..
> 

this is why it depends on the HW. unfortunately in our case HW stats !=
SW stats.

> > This gives more flexibility and correctness, any given HW can
> > create
> > multiple netdevs on the same function, we need the netdev stats to
> > reflect traffic that only went through that netdev.
> > 
> > > I thought it would be entirely reasonable for the driver to read
> > > the
> > > stats from a delayed work every 1/2 HZ and cache that. We do have
> > > a
> > > knob in ethtool IRQ coalescing settings for stats writeback
> > > frequency.
> > 
> > Some customers didn't like this since for drivers that implement
> > this
> > their CPU power utilization will be slightly higher on idle.
> 
> Other customers may dislike the per packet cycles.
> 
> I don't really mind, I just found the commit message to be lacking 
> for a fix, which this supposedly is.
> 

Yes commit message could use some improvement.

I think it is even worse than i thought, we removed  the background
refreshing of stats due to the FW events support to updates some
counters. Multicast counter can't be refreshed by FW events since it is
a data path counters. 

> Also looks like you report the total number of mcast packets in
> ethtool
> -S, which should be identical to ip -s? If so please remove that.

why ? it is ok to report the same counter both in ehttool and netdev
stats.