netdev - Re: [patch net-next 1/4] netdevice: add SW statistics ndo

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:	Sat, 14 May 2016 14:49:17 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, nogahf@...lanox.com,
	idosch@...lanox.com, eladr@...lanox.com, yotamg@...lanox.com,
	ogerlitz@...lanox.com, nikolay@...ulusnetworks.com,
	linville@...driver.com, tgraf@...g.ch, gospo@...ulusnetworks.com,
	sfeldma@...il.com, sd@...asysnail.net, eranbe@...lanox.com,
	ast@...mgrid.com, edumazet@...gle.com, hannes@...essinduktion.org
Subject: Re: [patch net-next 1/4] netdevice: add SW statistics ndo

Fri, May 13, 2016 at 08:47:48PM CEST, roopa@...ulusnetworks.com wrote:
>On 5/12/16, 11:03 PM, Jiri Pirko wrote:
>> Thu, May 12, 2016 at 11:10:08PM CEST, roopa@...ulusnetworks.com wrote:
>>> On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>>>> From: Nogah Frankel <nogahf@...lanox.com>
>>>>
>>>> Till now we had a ndo statistics function that returned SW statistics.
>>>> We want to change the "basic" statistics to return HW statistics if
>>>> available.
>>>> In this case we need to expose a new ndo to return the SW statistics.
>>>> Add a new ndo declaration to get SW statistics
>>>> Add a function that gets SW statistics if a competible ndo exist
>>>>
>>>> Signed-off-by: Nogah Frankel <nogahf@...lanox.com>
>>>> Reviewed-by: Ido Schimmel <idosch@...lanox.com>
>>>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>>>> ---
>>>>
>>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>>> (logical or physical). HW stats is queried and added to the SW stats.
>> I'm not sure I follow. HW stats already contain SW stats. Because on
>> slow path every packet that is not offloaded and goes through kernel is
>> counted into HW stats as well (because it goes through HW port). 
>yes, correct... we don't want to double count those. But since these stats are
>generally queried from hw, I am calling them HW stats.
>you will not really maintain a software counter for this. But, the driver can maintain its own
> counters for rx and tx errors etc and I call these SW stats. They are counted at the driver.
>
>> If you
>> do HW stats + SW stats, what you get makes no sense. Am I missing something?
>If you go by my definition of HW and SW stats above, on a ndo_get_stats64() call,
>you will add the SW counters + HW counters and return. In my definition, the pkts
>that was rx'ed or tx'ed successfully are always in the HW count.
>
>> Btw, looking at enic_get_stats, looks exactly what we introduce for
>> mlxsw in this patchset.
>
>In enic_get_stats, the ones counted in software are the ones taken from 'enic->'
>     net_stats->rx_over_errors = enic->rq_truncated_pkts;
>     net_stats->rx_crc_errors = enic->rq_bad_fcs;
>
>>
>> With this patchset, we only allow user to se the actual stats for
>> slow-path aka SW stats.
>hmm...ok. But i am not sure how many will use this new attribute.
>When you do 'ip -s link show' you really want all counters on that port
>hardware or software does not matter at that point.
>
>My suggestion to move this to ethtool like attribute is because that is an existing
> way to break down your stats which ever way you want. And the best part is it can be
>customized (say rx_pkts_cpu_saw)

I bevieve that ethtool is really not a place to expose sw stats. Does
not make sense.