| Message ID | 1471612650-4508-3-git-send-email-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 8/19/16, 6:17 AM, Jiri Pirko wrote: > From: Nogah Frankel <nogahf@mellanox.com> > > Add a nested attribute of SW stats to if_stats_msg > under IFLA_STATS_LINK_SW_64. > > Signed-off-by: Nogah Frankel <nogahf@mellanox.com> > Reviewed-by: Ido Schimmel <idosch@mellanox.com> > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > include/uapi/linux/if_link.h | 1 + > net/core/rtnetlink.c | 21 +++++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index a1b5202..1c9b808 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -825,6 +825,7 @@ enum { > IFLA_STATS_LINK_64, > IFLA_STATS_LINK_XSTATS, > IFLA_STATS_LINK_XSTATS_SLAVE, > + IFLA_STATS_LINK_SW_64, > hate to sound like a broken record here... sorry, but like I have been saying throughout this patch series, i don't think I can ack a new attribute for so called 'software stats' at the same level as existing IFLA_STATS_LINK_64. It just adds ambiguity for existing stats and confusion for future stats. Today's IFLA_STATS64 or IFLA_STATS_LINK_64 provides aggregate stats and historically its been 'HW only' or 'SW only' or 'HW + SW'. It depends on the driver. logical devices provide pure 'SW only' stats here. There is no real reason to qualify them now. The user/app has only cared about and will continue to care about only aggregate stats. That requirement has never changed. Everything else is breakdown for debug-ability.., hence should be in this second bucket I have been talking about (these are traditionally available via ethtool today). I am not arguing against the value of the stats this patch series provides. But, since the beginning of the stats api, I have always talked about a nested extensible attribute for drivers who wish to break their stats down. so, I have only been requesting to put this so called 'software stats' attribute in a new nested attribute which can be extended for other such specially qualified stats. And, I thought we had agreed on such an attribute. I mention such a nested attribute here again on your previous post: http://marc.info/?l=linux-netdev&m=147085641703885&w=2 Thanks, Roopa
Tue, Aug 23, 2016 at 07:51:57AM CEST, roopa@cumulusnetworks.com wrote: >On 8/19/16, 6:17 AM, Jiri Pirko wrote: >> From: Nogah Frankel <nogahf@mellanox.com> >> >> Add a nested attribute of SW stats to if_stats_msg >> under IFLA_STATS_LINK_SW_64. >> >> Signed-off-by: Nogah Frankel <nogahf@mellanox.com> >> Reviewed-by: Ido Schimmel <idosch@mellanox.com> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> include/uapi/linux/if_link.h | 1 + >> net/core/rtnetlink.c | 21 +++++++++++++++++++++ >> 2 files changed, 22 insertions(+) >> >> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h >> index a1b5202..1c9b808 100644 >> --- a/include/uapi/linux/if_link.h >> +++ b/include/uapi/linux/if_link.h >> @@ -825,6 +825,7 @@ enum { >> IFLA_STATS_LINK_64, >> IFLA_STATS_LINK_XSTATS, >> IFLA_STATS_LINK_XSTATS_SLAVE, >> + IFLA_STATS_LINK_SW_64, >> >hate to sound like a broken record here... > >sorry, but like I have been saying throughout this patch series, >i don't think I can ack a new attribute for so called 'software stats' at the >same level as existing IFLA_STATS_LINK_64. It just adds ambiguity for existing stats and >confusion for future stats. > >Today's IFLA_STATS64 or IFLA_STATS_LINK_64 provides aggregate stats >and historically its been 'HW only' or 'SW only' or 'HW + SW'. It depends on the driver. logical devices >provide pure 'SW only' stats here. There is no real reason to qualify them now. The user/app >has only cared about and will continue to care about only aggregate stats. That requirement >has never changed. > >Everything else is breakdown for debug-ability.., hence should be in this second bucket I >have been talking about (these are traditionally available via ethtool today). > >I am not arguing against the value of the stats this patch series provides. But, since the beginning >of the stats api, I have always talked about a nested extensible attribute for drivers who wish to >break their stats down. so, I have only been requesting to put this so called 'software stats' attribute >in a new nested attribute which can be extended for other such specially qualified stats. > >And, I thought we had agreed on such an attribute. I mention such a nested attribute >here again on your previous post: http://marc.info/?l=linux-netdev&m=147085641703885&w=2 And Nogah replied. 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.
From: Jiri Pirko <jiri@resnulli.us> Date: Tue, 23 Aug 2016 08:53:18 +0200 > 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. Second, the more I think about this what you're providing isn't actually a new statistic type. It's a filter. So why don't we just provide a filter specification that gets passed down into the driver. And the user can ask for "SW stats" or whatever using that.
Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote: >From: Jiri Pirko <jiri@resnulli.us> >Date: Tue, 23 Aug 2016 08:53:18 +0200 > >> 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. So we can put them under *MISC* nest attr. But that is exactly purpose of the top-level here. /me confused > >Second, the more I think about this what you're providing isn't >actually a new statistic type. > >It's a filter. > >So why don't we just provide a filter specification that gets passed >down into the driver. And the user can ask for "SW stats" or whatever >using that. for this particular stats (sw stats) you can look at it that way. The question is that in case we handle it the filter way instead of multiattr way. Is it convenient for user to do 2 netlink calls instead of one? I tend to like the one-call-flag-what-you-want approach better.
On 8/23/16, 12:26 AM, Jiri Pirko wrote: > Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote: >> From: Jiri Pirko <jiri@resnulli.us> >> Date: Tue, 23 Aug 2016 08:53:18 +0200 >> >>> 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 ?. > 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. thanks, Roopa
Tue, Aug 23, 2016 at 04:46:37PM CEST, roopa@cumulusnetworks.com wrote: >On 8/23/16, 12:26 AM, Jiri Pirko wrote: >> Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote: >>> From: Jiri Pirko <jiri@resnulli.us> >>> Date: Tue, 23 Aug 2016 08:53:18 +0200 >>> >>>> 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 ?. > > >> 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? Thanks.
On 8/23/16, 7:52 AM, Jiri Pirko wrote: > Tue, Aug 23, 2016 at 04:46:37PM CEST, roopa@cumulusnetworks.com wrote: >> On 8/23/16, 12:26 AM, Jiri Pirko wrote: >>> Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote: >>>> From: Jiri Pirko <jiri@resnulli.us> >>>> Date: Tue, 23 Aug 2016 08:53:18 +0200 >>>> >>>>> 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 ?. >> >> >>> 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!
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index a1b5202..1c9b808 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -825,6 +825,7 @@ enum { IFLA_STATS_LINK_64, IFLA_STATS_LINK_XSTATS, IFLA_STATS_LINK_XSTATS_SLAVE, + IFLA_STATS_LINK_SW_64, __IFLA_STATS_MAX, }; diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 189cc78..910f802 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3583,6 +3583,21 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev, dev_get_stats(dev, sp); } + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_SW_64, *idxattr)) { + if (dev_have_sw_stats(dev)) { + struct rtnl_link_stats64 *sp; + + attr = nla_reserve_64bit(skb, IFLA_STATS_LINK_SW_64, + sizeof(struct rtnl_link_stats64), + IFLA_STATS_UNSPEC); + if (!attr) + goto nla_put_failure; + + sp = nla_data(attr); + dev_get_sw_stats(dev, sp); + } + } + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_XSTATS, *idxattr)) { const struct rtnl_link_ops *ops = dev->rtnl_link_ops; @@ -3644,6 +3659,7 @@ nla_put_failure: static const struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = { [IFLA_STATS_LINK_64] = { .len = sizeof(struct rtnl_link_stats64) }, + [IFLA_STATS_LINK_SW_64] = { .len = sizeof(struct rtnl_link_stats64) }, }; static size_t if_nlmsg_stats_size(const struct net_device *dev, @@ -3685,6 +3701,11 @@ static size_t if_nlmsg_stats_size(const struct net_device *dev, } } + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_SW_64, 0)) { + if (dev_have_sw_stats(dev)) + size += nla_total_size_64bit(sizeof(struct rtnl_link_stats64)); + } + return size; }