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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Tue, 16 Aug 2016 12:47:00 +0000
From:	Nogah Frankel <nogahf@...lanox.com>
To:	Roopa Prabhu <roopa@...ulusnetworks.com>,
	Jiri Pirko <jiri@...nulli.us>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	Ido Schimmel <idosch@...lanox.com>,
	Elad Raz <eladr@...lanox.com>,
	Yotam Gigi <yotamg@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	"nikolay@...ulusnetworks.com" <nikolay@...ulusnetworks.com>,
	"linville@...driver.com" <linville@...driver.com>,
	"tgraf@...g.ch" <tgraf@...g.ch>,
	"gospo@...ulusnetworks.com" <gospo@...ulusnetworks.com>,
	"sfeldma@...il.com" <sfeldma@...il.com>,
	"sd@...asysnail.net" <sd@...asysnail.net>,
	"Eran Ben Elisha" <eranbe@...lanox.com>,
	"ast@...mgrid.com" <ast@...mgrid.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"hannes@...essinduktion.org" <hannes@...essinduktion.org>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>,
	"dsa@...ulusnetworks.com" <dsa@...ulusnetworks.com>
Subject: RE: [patch net-next v6 2/3] net: core: add SW stats to if_stats_msg

> -----Original Message-----
> From: Roopa Prabhu [mailto:roopa@...ulusnetworks.com]
> Sent: Wednesday, August 10, 2016 9:09 AM
> To: Jiri Pirko <jiri@...nulli.us>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; Nogah Frankel
> <nogahf@...lanox.com>; Ido Schimmel <idosch@...lanox.com>; Elad
> Raz <eladr@...lanox.com>; Yotam Gigi <yotamg@...lanox.com>; Or
> Gerlitz <ogerlitz@...lanox.com>; nikolay@...ulusnetworks.com;
> linville@...driver.com; tgraf@...g.ch; gospo@...ulusnetworks.com;
> sfeldma@...il.com; sd@...asysnail.net; Eran Ben Elisha
> <eranbe@...lanox.com>; ast@...mgrid.com; edumazet@...gle.com;
> hannes@...essinduktion.org; f.fainelli@...il.com;
> dsa@...ulusnetworks.com
> Subject: Re: [patch net-next v6 2/3] net: core: add SW stats to if_stats_msg
> 
> On 8/9/16, 2:25 PM, Jiri Pirko wrote:
> > From: Nogah Frankel <nogahf@...lanox.com>
> >
> > Add a nested attribute of SW stats to if_stats_msg
> > under IFLA_STATS_LINK_SW_64.
> >
> > Signed-off-by: Nogah Frankel <nogahf@...lanox.com>
> > Reviewed-by: Ido Schimmel <idosch@...lanox.com>
> > Signed-off-by: Jiri Pirko <jiri@...lanox.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,
> 
> sorry, don't mean to drag this discussion forever...but...
> 
> IIRC on the switchdev call, I thought we had agreed to just put these kind of
> breakdown
> stats in one more layer of nesting.. which can be extended for everybody.
> 
> IFLA_STATS_LINK_DRIVER_XSTATS   (or some other name) [
>                 IFLA_STATS_LINK_SW_64  (struct rtnl_link_stats64)    <---- you get
> the well defined struct here as you wanted.
>                 IFLA_STATS_LINK_HW_ACL_DROPS  /* u64 */        <----- we can
> keep extending this for other stats people want.
> 
>                 /* just keeps it extensible */
> ]
> 
> because in addition to all other reasons I have mentioned before,
> technically IFLA_STATS_LINK_64 (top-level aggregate stats)
> are also software only stats for say most logical devices etc.
> 
> 
> and instead of adding one ndo for your software only stats now and then
> again for other ethtool
> like hw stats from all drivers, it could also be like the below:
> 
> new ndo_get_hw_stats
> 
> IFLA_STATS_LINK_HW_XSTATS   (or some other name) [
>                 IFLA_STATS_LINK_CPU_64  (struct rtnl_link_stats64)    <---- you
> get the well defined struct here as you wanted.
>                 IFLA_STATS_LINK_HW_ACL_DROPS  /* u64 */              <----- we can
> keep extending this for other stats people want.
>                 /* just keeps it extensible */
> ]
> 
> It gets you what you want and keeps the api extensible IMO. your call.
> 

I don't think extra nesting is a good idea because when we are called for 
IFLA_STATS_LINK_DRIVER_XSTATS  we can't distinguish between subtypes of it.
So for each driver stats we are going to need different XSTATS types anyway.

We shouldn't return all the stats a driver can provide when we are asked for one.
HW_ACL_DROPS shouldn't be queried when user want SW stats nor the  other
way around.
 (And I think that stats exposed here should be well defined so they could 
be exposed to ifstat. The place for not well define stats is in ethtool)

> >  	__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;
> >  }
> >

Powered by blists - more mailing lists