Re: [patch net-next v6_repost 2/3] net: core: add SW stats to if_stats_msg

On 8/23/16, 7:52 AM, Jiri Pirko wrote:

Tue, Aug 23, 2016 at 04:46:37PM CEST, roopa@cumulusnetworks.com wrote:

quoted

On 8/23/16, 12:26 AM, Jiri Pirko wrote:

quoted

Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 23 Aug 2016 08:53:18 +0200

quoted

Anyway I think that next level of nesting is not necessary. On
contrary, it is wrong. The current level is extensible, mixed and
flagged already. I don't see any reason why not to add whatever kind of
stats here. What makes IFLA_STATS_LINK_SW_64 or for example
IFLA_STATS_LINK_HW_ACL so special it has to be nested in some other
attr? I would understand it it would be values of one family, but that
is not the case.

First, I agree with Roopa.  If we want to put this stuff out
there is should be bucketed together in a nested attribute with
other similar stats specifications.

Well I still don't think that IFLA_STATS_LINK_SW_64 and
IFLA_STATS_LINK_HW_ACL are related. You cannot put it under *DRIVER*
nest as IFLA_STATS_LINK_SW_64 are core stats.

not sure i understand, why is this core stats ?.
should a new logical device implement  IFLA_STATS_LINK_64 or IFLA_STATS_LINK_SW_64 ?
any other users ?.

quoted

 So we can put them under
*MISC* nest attr. But that is exactly purpose of the top-level here.
/me confused

By design top level is for higher level grouping of stats (that also helps us maintain a lean higher
level filter space). They are mainly categories of stats. for example we have a nested link
XSTATS attribute..which are again a break down of stats already counted in IFLA_STATS_LINK_64.
That's why I think we can group this into another kind of breakdown stats.

I give up. What name do you suggest for the nested attribute?

how about the below ?
IFLA_STATS_LINK_OFFLOAD_XSTATS [
        IFLA_OFFLOAD_XSTATS_SW_HIT or IFLA_OFFLOAD_XSTATS_CPU_HIT or whatever you want to call it ?
]

reasons: we need an offload driver stats bucket anyways (for the hw driver extended stats.
similar to ethtool stats). keeping the name generic will help cover switchdev and other hw offload uses.
If there is a need for a second level filter in the future, we can always
add a IFLA_OFFLOAD_XSTATS_FILTER attribute when that becomes necessary.

I agree with dave that this is essentially a filter on the existing stats and can be implemented as
a flag someplace. But, the best way I can see that filter implemented in the current code/api
is with a nested attribute like above.

Thanks Jiri!