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 20:46:29 +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

Sat, May 14, 2016 at 05:47:41PM CEST, roopa@...ulusnetworks.com wrote:
>On 5/14/16, 5:49 AM, Jiri Pirko wrote:
>> 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.
>2 things:
>- i was surprised you don't want your ndo_get_stats64 to be a unified view of HW and SW stats

Roopa, please, look at the patch 4/4. That is exactly what we are doing.
We expose HW stats via ndo_get_stats64 and that is of course including
whatever comes through slowpath (non-forwarded in HW).


>- by bringing up ethtool like stats (IFLA_STATS_LINK_HW_EXTENDED) I am just saying
>it has always been a way to breakdown stats. If you don't want to show explicit SW stats there,
>there is always a way to show HW only stats....and now you know the delta between the unified stats
>and the HW only stats is your SW stats.

I think we don/t understand each other. HW stats always include SW
stats. Because whatever goes in or out goes through HW. Therefore, the
"unified stats" you mention are exactly HW stats.

This is fine, Patch 4/4 would do to make this correct. However, I think
it has value for user to know what went via slowpath (non-forwarded in HW).
And that is exacly exposed by the SW stats we try to add.

Is that confusing?