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]
Message-ID: <20160515064726.GB1974@nanopsycho.orion>
Date:	Sun, 15 May 2016 08:47:27 +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

Sun, May 15, 2016 at 06:11:20AM CEST, roopa@...ulusnetworks.com wrote:
>On 5/14/16, 11:46 AM, Jiri Pirko wrote:
>> 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:
>>>>>
>[snip]
>>>>>  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).
>
>Maybe i missed it but i did not think it included any rx or tx err counters counted solely
>by the driver.
>>
>>
>>> - 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?
>
>Its not confusing. I understand what you are doing.
>The only point I was making was that most drivers have unified stats via ndo
>and there are also hw stats via ethtool like api (which will also be part of the stats
>api in the future). And sw only stats can be derived from that...which is the way most

The thing is, they can't be derived from it. That is my whole point.
HW-HW=0


>people do today.
>But that's fine. If you think it will be useful/easier to have a new api/attribute
>for software only stats for some drivers, sure, fine. Lets move on.
>
>