[PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch adds a new RTM_GETSTATS message to query link stats via netlink
from the kernel. RTM_NEWLINK also dumps stats today, but RTM_NEWLINK
returns a lot more than just stats and is expensive in some cases when
frequent polling for stats from userspace is a common operation.

RTM_GETSTATS is an attempt to provide a light weight netlink message
to explicity query only link stats from the kernel on an interface.
The idea is to also keep it extensible so that new kinds of stats can be
added to it in the future.

This patch adds the following attribute for NETDEV stats:
struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
        [IFLA_STATS_LINK64]  = { .len = sizeof(struct rtnl_link_stats64) },
};

This patch also allows for af family stats (an example af stats for IPV6
is available with the second patch in the series).

Like any other rtnetlink message, RTM_GETSTATS can be used to get stats of
a single interface or all interfaces with NLM_F_DUMP.

Future possible new types of stat attributes:
- IFLA_MPLS_STATS  (nested. for mpls/mdev stats)
- IFLA_EXTENDED_STATS (nested. extended software netdev stats like bridge,
  vlan, vxlan etc)
- IFLA_EXTENDED_HW_STATS (nested. extended hardware stats which are
  available via ethtool today)

This patch also declares a filter mask for all stat attributes.
User has to provide a mask of stats attributes to query. This will be
specified in a new hdr 'struct if_stats_msg' for stats messages.

Without any attributes in the filter_mask, no stats will be returned.

This patch has been tested with mofified iproute2 ifstat.

Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 include/net/rtnetlink.h        |   5 ++
 include/uapi/linux/if_link.h   |  18 ++++
 include/uapi/linux/rtnetlink.h |   5 ++
 net/core/rtnetlink.c           | 200 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 228 insertions(+)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 2f87c1b..fa68158 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -131,6 +131,11 @@ struct rtnl_af_ops {
 						    const struct nlattr *attr);
 	int			(*set_link_af)(struct net_device *dev,
 					       const struct nlattr *attr);
+	size_t			(*get_link_af_stats_size)(const struct net_device *dev,
+							  u32 filter_mask);
+	int			(*fill_link_af_stats)(struct sk_buff *skb,
+						      const struct net_device *dev,
+						      u32 filter_mask);
 };
 
 void __rtnl_af_unregister(struct rtnl_af_ops *ops);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 9427f17..4cfd029 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -780,4 +780,22 @@ enum {
 
 #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
 
+/* STATS section */
+
+struct if_stats_msg {
+	__u8  family;
+	__u32 ifindex;
+	__u32 filter_mask;
+};
+
+enum {
+	IFLA_STATS_UNSPEC,
+	IFLA_STATS_LINK64,
+	__IFLA_STATS_MAX,
+};
+
+#define IFLA_STATS_MAX (__IFLA_STATS_MAX - 1)
+
+#define IFLA_STATS_FILTER_BIT(ATTR)	(1 << (ATTR))
+
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index ca764b5..cc885c4 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -139,6 +139,11 @@ enum {
 	RTM_GETNSID = 90,
 #define RTM_GETNSID RTM_GETNSID
 
+	RTM_NEWSTATS = 92,
+#define RTM_NEWSTATS RTM_NEWSTATS
+	RTM_GETSTATS = 94,
+#define RTM_GETSTATS RTM_GETSTATS
+
 	__RTM_MAX,
 #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
 };
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a75f7e9..d1fba58 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3451,6 +3451,203 @@ out:
 	return err;
 }
 
+static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
+			       int type, u32 pid, u32 seq, u32 change,
+			       unsigned int flags, unsigned int filter_mask)
+{
+	const struct rtnl_link_stats64 *stats;
+	struct rtnl_link_stats64 temp;
+	struct if_stats_msg *ifsm;
+	struct nlmsghdr *nlh;
+	struct rtnl_af_ops *af_ops;
+	struct nlattr *attr;
+
+	ASSERT_RTNL();
+
+	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifsm), flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	ifsm = nlmsg_data(nlh);
+	ifsm->ifindex = dev->ifindex;
+	ifsm->filter_mask = filter_mask;
+
+	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK64)) {
+		attr = nla_reserve(skb, IFLA_STATS_LINK64,
+				   sizeof(struct rtnl_link_stats64));
+		if (!attr)
+			return -EMSGSIZE;
+
+		stats = dev_get_stats(dev, &temp);
+
+		copy_rtnl_link_stats64(nla_data(attr), stats);
+	}
+
+	list_for_each_entry(af_ops, &rtnl_af_ops, list) {
+		if (af_ops->fill_link_af_stats) {
+			int err;
+
+			err = af_ops->fill_link_af_stats(skb, dev, filter_mask);
+			if (err < 0)
+				goto nla_put_failure;
+		}
+	}
+
+	nlmsg_end(skb, nlh);
+
+	return 0;
+
+nla_put_failure:
+	nlmsg_cancel(skb, nlh);
+
+	return -EMSGSIZE;
+}
+
+static const struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
+	[IFLA_STATS_LINK64]	= { .len = sizeof(struct rtnl_link_stats64) },
+};
+
+static size_t rtnl_link_get_af_stats_size(const struct net_device *dev,
+					  u32 filter_mask)
+{
+	struct rtnl_af_ops *af_ops;
+	size_t size = 0;
+
+	list_for_each_entry(af_ops, &rtnl_af_ops, list) {
+		if (af_ops->get_link_af_stats_size)
+			size += af_ops->get_link_af_stats_size(dev,
+							       filter_mask);
+	}
+
+	return size;
+}
+
+static size_t if_nlmsg_stats_size(const struct net_device *dev,
+				  u32 filter_mask)
+{
+	size_t size = 0;
+
+	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK64))
+		size += nla_total_size(sizeof(struct rtnl_link_stats64));
+
+	size += rtnl_link_get_af_stats_size(dev, filter_mask);
+
+	return size;
+}
+
+static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
+{
+	struct net *net = sock_net(skb->sk);
+	struct if_stats_msg *ifsm;
+	struct net_device *dev = NULL;
+	struct sk_buff *nskb;
+	u32 filter_mask;
+	int err;
+
+	ifsm = nlmsg_data(nlh);
+	if (ifsm->ifindex > 0)
+		dev = __dev_get_by_index(net, ifsm->ifindex);
+	else
+		return -EINVAL;
+
+	if (!dev)
+		return -ENODEV;
+
+	filter_mask = ifsm->filter_mask;
+	if (!filter_mask)
+		return -EINVAL;
+
+	nskb = nlmsg_new(if_nlmsg_stats_size(dev, filter_mask), GFP_KERNEL);
+	if (!nskb)
+		return -ENOBUFS;
+
+	err = rtnl_fill_statsinfo(nskb, dev, RTM_NEWSTATS,
+				  NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0,
+				  0, filter_mask);
+	if (err < 0) {
+		/* -EMSGSIZE implies BUG in if_nlmsg_stats_size */
+		WARN_ON(err == -EMSGSIZE);
+		kfree_skb(nskb);
+	} else {
+		err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
+	}
+
+	return err;
+}
+
+static u16 rtnl_stats_calcit(struct sk_buff *skb, struct nlmsghdr *nlh)
+{
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
+	u16 min_ifinfo_dump_size = 0;
+	struct if_stats_msg *ifsm;
+	u32 filter_mask;
+
+	ifsm = nlmsg_data(nlh);
+	filter_mask = ifsm->filter_mask;
+
+	/* traverse the list of net devices and compute the minimum
+	 * buffer size based upon the filter mask.
+	 */
+	list_for_each_entry(dev, &net->dev_base_head, dev_list) {
+		min_ifinfo_dump_size = max_t(u16, min_ifinfo_dump_size,
+					     if_nlmsg_stats_size(dev,
+								 filter_mask));
+	}
+
+	return min_ifinfo_dump_size;
+}
+
+static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct net *net = sock_net(skb->sk);
+	struct if_stats_msg *ifsm;
+	int h, s_h;
+	int idx = 0, s_idx;
+	struct net_device *dev;
+	struct hlist_head *head;
+	unsigned int flags = NLM_F_MULTI;
+	u32 filter_mask = 0;
+	int err;
+
+	s_h = cb->args[0];
+	s_idx = cb->args[1];
+
+	cb->seq = net->dev_base_seq;
+
+	ifsm = nlmsg_data(cb->nlh);
+	filter_mask = ifsm->filter_mask;
+
+	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
+		idx = 0;
+		head = &net->dev_index_head[h];
+		hlist_for_each_entry(dev, head, index_hlist) {
+			if (idx < s_idx)
+				goto cont;
+			err = rtnl_fill_statsinfo(skb, dev, RTM_NEWSTATS,
+						  NETLINK_CB(cb->skb).portid,
+						  cb->nlh->nlmsg_seq, 0,
+						  flags, filter_mask);
+			/* If we ran out of room on the first message,
+			 * we're in trouble
+			 */
+			WARN_ON((err == -EMSGSIZE) && (skb->len == 0));
+
+			if (err < 0)
+				goto out;
+
+			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
+cont:
+			idx++;
+		}
+	}
+out:
+	cb->args[1] = idx;
+	cb->args[0] = h;
+
+	return skb->len;
+}
+
 /* Process one rtnetlink message. */
 
 static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
@@ -3600,4 +3797,7 @@ void __init rtnetlink_init(void)
 	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, NULL);
 	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, NULL);
 	rtnl_register(PF_BRIDGE, RTM_SETLINK, rtnl_bridge_setlink, NULL, NULL);
+
+	rtnl_register(PF_UNSPEC, RTM_GETSTATS, rtnl_stats_get, rtnl_stats_dump,
+		      rtnl_stats_calcit);
 }
-- 
1.9.1

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Jamal Hadi Salim]

Thanks for doing the work Roopa and I apologize for late comments
below:

On 16-04-09 02:38 AM, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>

> This patch also allows for af family stats (an example af stats for IPV6
> is available with the second patch in the series).
>
> Like any other rtnetlink message, RTM_GETSTATS can be used to get stats of
> a single interface or all interfaces with NLM_F_DUMP.
>
> Future possible new types of stat attributes:
> - IFLA_MPLS_STATS  (nested. for mpls/mdev stats)
> - IFLA_EXTENDED_STATS (nested. extended software netdev stats like bridge,
>    vlan, vxlan etc)
> - IFLA_EXTENDED_HW_STATS (nested. extended hardware stats which are
>    available via ethtool today)
>

I got the extended_hw_stats (which are very common in a lot of ASICS) if
you mean stats on packet sizes. But would the other extended stats not
just be per netdev kind specific? We have concept of XSTATS which maybe
a fit.

> This patch also declares a filter mask for all stat attributes.
> User has to provide a mask of stats attributes to query. This will be
> specified in a new hdr 'struct if_stats_msg' for stats messages.
>
> Without any attributes in the filter_mask, no stats will be returned.
>

Should such a command then not be rejected with an error code?

> +/* STATS section */
> +
> +struct if_stats_msg {
> +	__u8  family;
> +	__u32 ifindex;
> +	__u32 filter_mask;
> +};

Needs to be 32 bit aligned.
Do you need 32 bits for the filter mask?
Perhaps a 16bit mask and an 8bit pad for future use.

struct if_stats_msg {
            __u32 ifindex;
	   __u16 filter_mask;
	   __u8  family;
            __u8 pad; /* future use */
};

Or you could reverse those (from smallest to largest).
BTW, any plans to do the cool feature where i inject a timeout period
and i just get STATS events ;-> The filter struct would have to be more
sophisticated - user would need to pass a list of ifindices and
filter_mask as well as timeout.

cheers,
jamal

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[roopa]

On 4/9/16, 7:30 AM, Jamal Hadi Salim wrote:
>
> Thanks for doing the work Roopa and I apologize for late comments
> below:
>
> On 16-04-09 02:38 AM, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>
>> This patch also allows for af family stats (an example af stats for IPV6
>> is available with the second patch in the series).
>>
>> Like any other rtnetlink message, RTM_GETSTATS can be used to get stats of
>> a single interface or all interfaces with NLM_F_DUMP.
>>
>> Future possible new types of stat attributes:
>> - IFLA_MPLS_STATS  (nested. for mpls/mdev stats)
>> - IFLA_EXTENDED_STATS (nested. extended software netdev stats like bridge,
>>    vlan, vxlan etc)
>> - IFLA_EXTENDED_HW_STATS (nested. extended hardware stats which are
>>    available via ethtool today)
>>
>
> I got the extended_hw_stats (which are very common in a lot of ASICS) if
> you mean stats on packet sizes. But would the other extended stats not
> just be per netdev kind specific? We have concept of XSTATS which maybe
> a fit.
This EXTENDED_HW_STATS is for ethtool like extended hw stats. This is keeping in
mind that we want to also move ethtool to netlink in the future and with switchdev
it becomes more necessary that we provide all stats closer to the other netdev stats.
So far hw extended stats have always been available through this separate ethtool
channel. The intent here is to unify the api for extended hw and software only stats.

XSTATS is per netdev can be included as a nested attribute inside IFLA_EXTENDED_STATS
which are per netdev. bridge vlan stats will also fall here.

And this api  provides netdev specific stats. We have always mapped all
asic stats to the switch port netdev stats. and this api does not cover the non-netdev specific stats.
If you are for example asking for stats for a hardware offloaded bridge, then yes, they will fall here
and be available on the bridge netdev. For asic stats that don't map to any netdev, devlink will be an
appropriate infrastructure IMO.

I am not sure if I answered your question :).

>
>> This patch also declares a filter mask for all stat attributes.
>> User has to provide a mask of stats attributes to query. This will be
>> specified in a new hdr 'struct if_stats_msg' for stats messages.
>>
>> Without any attributes in the filter_mask, no stats will be returned.
>>
>
> Should such a command then not be rejected with an error code?
Dumps with no data are not rejected with an error code AFAIK. ie they don't return
-ENODATA. This is consistent with all other dumps (unless i missed it).
 But, if there is a need for an error code, i can certainly check again.

>> +/* STATS section */
>> +
>> +struct if_stats_msg {
>> +    __u8  family;
>> +    __u32 ifindex;
>> +    __u32 filter_mask;
>> +};
>
> Needs to be 32 bit aligned.
> Do you need 32 bits for the filter mask?
yes, i think we should keep it minimum 32 bits.
> Perhaps a 16bit mask and an 8bit pad for future use.
>
> struct if_stats_msg {
>            __u32 ifindex;
>        __u16 filter_mask;
>        __u8  family;
>            __u8 pad; /* future use */
> };
>
> Or you could reverse those (from smallest to largest).

The __u8 family needs to be the first field in the structure and at the first byte in the header data.
hence family is first and i added the others after that. It follows the format for existing such structs (for other message types).

> BTW, any plans to do the cool feature where i inject a timeout period
> and i just get STATS events ;-> The filter struct would have to be more
> sophisticated - user would need to pass a list of ifindices and
> filter_mask as well as timeout.
Yeah i remember :). But deferring it for a later incremental feature. That needs some more thought.
Right now there is an urgent need to get the basic get stats api for a bunch of other stats: mpls, bridge vlan etc.
Because it is not clean to extend the current stats infra part of the link message for this. So trying to get this in first.

And this patchset only adds a handler for RTM_NEWSTATS dump and get stats. Your stats events request should be part of the  RTM_NEWSTATS handler and can include other attributes (like timeout) in the future.

Thanks,
Roopa

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Jamal Hadi Salim]

On 16-04-09 02:00 PM, roopa wrote:

> This EXTENDED_HW_STATS is for ethtool like extended hw stats. This is keeping in
> mind that we want to also move ethtool to netlink in the future and with switchdev
> it becomes more necessary that we provide all stats closer to the other netdev stats.
> So far hw extended stats have always been available through this separate ethtool
> channel. The intent here is to unify the api for extended hw and software only stats.

Ok, so these are _not_ the stats which are broken down by packet size
ranges which are quiet common; but rather "proprietary" per port
type h/w stats? I browsed a couple of users of ethtool_stats and i see
they return proprietary looking 64 bit counters (batman for example
has a very strange meaning to those stats).
What i meant is a lot of ASICS have counters for byte ranges
[64bytes-128bytes], [128bytes-256bytes], etc - sorry i cant pin a name
to those but i am sure you have seen them and i thought those at minimal
need their own TLV since they are always fixed.

> XSTATS is per netdev can be included as a nested attribute inside IFLA_EXTENDED_STATS
> which are per netdev. bridge vlan stats will also fall here.
>

And you are going to distinguish which come from hardware and which are
software derived?

> And this api  provides netdev specific stats. We have always mapped all
> asic stats to the switch port netdev stats. and this api does not cover the non-netdev specific stats.
> If you are for example asking for stats for a hardware offloaded bridge, then yes, they will fall here
> and be available on the bridge netdev. For asic stats that don't map to any netdev, devlink will be an
> appropriate infrastructure IMO.
>
> I am not sure if I answered your question :).
>

It is useful to have this discussion; unfortunately these user APIS once 
are in will never be allowed to change. The answers could come
in time.

>> Should such a command then not be rejected with an error code?
> Dumps with no data are not rejected with an error code AFAIK. ie they don't return
> -ENODATA. This is consistent with all other dumps (unless i missed it).
>   But, if there is a need for an error code, i can certainly check again.
>

It is mostly because you chose a whitelist filter i.e you list what
is allowed to be sent back. And if such a list is missing there
needs to be an opposite default (which is a deny all).

>>> +/* STATS section */
>>> +
>>> +struct if_stats_msg {
>>> +    __u8  family;
>>> +    __u32 ifindex;
>>> +    __u32 filter_mask;
>>> +};
>>
>> Needs to be 32 bit aligned.
>> Do you need 32 bits for the filter mask?
> yes, i think we should keep it minimum 32 bits.

Ok, that is fine then. I thought it wont exceed
3-4 per port type but i could be wrong. 32 bits
should be safer.

>> Perhaps a 16bit mask and an 8bit pad for future use.
>>
>> struct if_stats_msg {
>>             __u32 ifindex;
>>         __u16 filter_mask;
>>         __u8  family;
>>             __u8 pad; /* future use */
>> };
>>
>> Or you could reverse those (from smallest to largest).
>
> The __u8 family needs to be the first field in the structure and at the first byte in the header data.
> hence family is first and i added the others after that. It follows the format for existing such structs (for other message types).
>

True, but it must be 32 bit aligned. So something like:
struct if_stats_msg {
      __u8  family;
      __u8 pad1;
      __u16 pad2;
     __u32 ifindex;
     __u32 filter_mask;
}


> Yeah i remember :). But deferring it for a later incremental feature. That needs some more thought.

NP ;->

> Right now there is an urgent need to get the basic get stats api for a bunch of other stats: mpls, bridge vlan etc.
> Because it is not clean to extend the current stats infra part of the link message for this. So trying to get this in first.
>

Agreed.
The only thing i strongly feel about is the if_stats_msg - please fix
that and lets get at least the basics in. We can resolve other things
with further discussions.

> And this patchset only adds a handler for RTM_NEWSTATS dump and get stats. Your stats events request should be part of the  RTM_NEWSTATS handler and can include other attributes (like timeout) in the future.
>

Ok.

cheers,
jamal
> Thanks,
> Roopa
>

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[roopa]

On 4/10/16, 6:48 AM, Jamal Hadi Salim wrote:
> On 16-04-09 02:00 PM, roopa wrote:
>
>> This EXTENDED_HW_STATS is for ethtool like extended hw stats. This is keeping in
>> mind that we want to also move ethtool to netlink in the future and with switchdev
>> it becomes more necessary that we provide all stats closer to the other netdev stats.
>> So far hw extended stats have always been available through this separate ethtool
>> channel. The intent here is to unify the api for extended hw and software only stats.
>
> Ok, so these are _not_ the stats which are broken down by packet size
> ranges which are quiet common; but rather "proprietary" per port
> type h/w stats? I browsed a couple of users of ethtool_stats and i see
> they return proprietary looking 64 bit counters (batman for example
> has a very strange meaning to those stats).
> What i meant is a lot of ASICS have counters for byte ranges
> [64bytes-128bytes], [128bytes-256bytes], etc - sorry i cant pin a name
> to those but i am sure you have seen them and i thought those at minimal
> need their own TLV since they are always fixed.

yes, I think i have seen this. But, these if presented by the driver or hardware,
can be part of the extended hw stats (just like how ethtool would do).
So, there is the base netdev stats IFLA_STATS_LINK64 which is nothing but
rtnl_link_stats64 (which this patch adds). And this patch does not add but lists
examples for future additions IFLA_STATS_EXTENDED (for example: bridge
(can include igmp, stp, vlan stats here). bond can includelacp, and other stats here). All these will be TLV based.
and note that this patch does notrestrict or define the design for these yet.


>
>> XSTATS is per netdev can be included as a nested attribute inside IFLA_EXTENDED_STATS
>> which are per netdev. bridge vlan stats will also fall here.
>>
>
> And you are going to distinguish which come from hardware and which are
> software derived?

a) IFLA_EXTENDED_STATS - these are all software. ( for example: bridge (can include 
igmp, stp, vlan stats here).
bond can include lacp, and other stats here).

b) IFLA_HW_EXTENDED_STATS - for hardware stats. for a switch port these will be 
similar to ethtool
physical port stats today. For logical devices, this can include specific hw 
stats that switch asics
provide and those which cannot be added to the software stats.

And since this patch does not define the format for these stats yet, we can 
potentially design it when we see the first case of such stats.




>
>> And this api  provides netdev specific stats. We have always mapped all
>> asic stats to the switch port netdev stats. and this api does not cover the non-netdev specific stats.
>> If you are for example asking for stats for a hardware offloaded bridge, then yes, they will fall here
>> and be available on the bridge netdev. For asic stats that don't map to any netdev, devlink will be an
>> appropriate infrastructure IMO.
>>
>> I am not sure if I answered your question :).
>>
>
> It is useful to have this discussion; unfortunately these user APIS once are in will never be allowed to change. The answers could come
> in time.

yep, agreed.
>
>>> Should such a command then not be rejected with an error code?
>> Dumps with no data are not rejected with an error code AFAIK. ie they don't return
>> -ENODATA. This is consistent with all other dumps (unless i missed it).
>>   But, if there is a need for an error code, i can certainly check again.
>>
>
> It is mostly because you chose a whitelist filter i.e you list what
> is allowed to be sent back. And if such a list is missing there
> needs to be an opposite default (which is a deny all).

ok, by default no-filter = no-data is what we decided. And that
keeps the api simple. will think some more about if we should return
an err in case of no-filter.


>
>

[snip]

> True, but it must be 32 bit aligned. So something like:
> struct if_stats_msg {
>      __u8  family;
>      __u8 pad1;
>      __u16 pad2;
>     __u32 ifindex;
>     __u32 filter_mask;
> }
>

ok ack

>
>> Yeah i remember :). But deferring it for a later incremental feature. That needs some more thought.
>
> NP ;->
>
>> Right now there is an urgent need to get the basic get stats api for a bunch of other stats: mpls, bridge vlan etc.
>> Because it is not clean to extend the current stats infra part of the link message for this. So trying to get this in first.
>>
>
> Agreed.
> The only thing i strongly feel about is the if_stats_msg - please fix
> that and lets get at least the basics in. We can resolve other things
> with further discussions.

will do. Thanks for the review.

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Thomas Graf]

On 04/08/16 at 11:38pm, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> This patch adds a new RTM_GETSTATS message to query link stats via netlink
> from the kernel. RTM_NEWLINK also dumps stats today, but RTM_NEWLINK
> returns a lot more than just stats and is expensive in some cases when
> frequent polling for stats from userspace is a common operation.
> 
> RTM_GETSTATS is an attempt to provide a light weight netlink message
> to explicity query only link stats from the kernel on an interface.
> The idea is to also keep it extensible so that new kinds of stats can be
> added to it in the future.
> 
> This patch adds the following attribute for NETDEV stats:
> struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
>         [IFLA_STATS_LINK64]  = { .len = sizeof(struct rtnl_link_stats64) },
> };
> 
> This patch also allows for af family stats (an example af stats for IPV6
> is available with the second patch in the series).
> 
> Like any other rtnetlink message, RTM_GETSTATS can be used to get stats of
> a single interface or all interfaces with NLM_F_DUMP.

Awesome stuff Roopa.

This currently ties everything to a net_device with a selector to
include certain bits of that net_device. How about we take it half a
step further and allow for non net_device stats such as IP, TCP,
routing or ipsec stats to be retrieved as well?

A simple array of nested attributes replacing IFLA_STATS_* would
allow for that, e.g.

1. {.type = ST_IPSTATS, value = { ...} }

2. {.type = ST_LINK, .value = {
    {.type = ST_LINK_NAME, .value = "eth0"},
    {.type = ST_LINK_Q, .value = 10}
  }}

3. ...

So for your initial patch, you'd simply add a new top level attribute
which ties all the net_device specific statistics to a top level
attribute and we can add the non net_device specific stats later on.
We can't do that later on though without breaking ABI so that would
have to go in with the first iteration.

We can preserve all the existing attribute formats, we simply have
to introduce new attribute types which don't overlap and document
which statistic identifier maps to which existing attribute id.

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[roopa]

On 4/10/16, 1:16 AM, Thomas Graf wrote:
> On 04/08/16 at 11:38pm, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This patch adds a new RTM_GETSTATS message to query link stats via netlink
>> from the kernel. RTM_NEWLINK also dumps stats today, but RTM_NEWLINK
>> returns a lot more than just stats and is expensive in some cases when
>> frequent polling for stats from userspace is a common operation.
>>
>> RTM_GETSTATS is an attempt to provide a light weight netlink message
>> to explicity query only link stats from the kernel on an interface.
>> The idea is to also keep it extensible so that new kinds of stats can be
>> added to it in the future.
>>
>> This patch adds the following attribute for NETDEV stats:
>> struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
>>         [IFLA_STATS_LINK64]  = { .len = sizeof(struct rtnl_link_stats64) },
>> };
>>
>> This patch also allows for af family stats (an example af stats for IPV6
>> is available with the second patch in the series).
>>
>> Like any other rtnetlink message, RTM_GETSTATS can be used to get stats of
>> a single interface or all interfaces with NLM_F_DUMP.
> Awesome stuff Roopa.
>
> This currently ties everything to a net_device with a selector to
> include certain bits of that net_device. How about we take it half a
> step further and allow for non net_device stats such as IP, TCP,
> routing or ipsec stats to be retrieved as well?
yes, absolutely. and that is also the goal.
> A simple array of nested attributes replacing IFLA_STATS_* would
> allow for that, e.g.
>
> 1. {.type = ST_IPSTATS, value = { ...} }
>
> 2. {.type = ST_LINK, .value = {
>     {.type = ST_LINK_NAME, .value = "eth0"},
>     {.type = ST_LINK_Q, .value = 10}
>   }}
>
> 3. ...

One thing though,  Its unclear to me if we absolutely need the additional nest.
Every stats netlink msg has an ifindex in the header (if_stats_msg) if the scope
of the stats is a netdev. If the msg does not have an ifindex in the if_stats_msg,
it represents a global stat. ie Cant a dump, include other stats netlink msgs after
all the netdev msgs are done when the filter has global stat filters ?.
same will apply to RTM_GETSTATS (without NLM_F_DUMP).

Since the msg may potentially have more nest levels
in the IFLA_EXT_STATS categories, just trying to see if i can avoid adding another
top-level nest. We can sure add it if there is no other way to include global
stats in the same dump.
> So for your initial patch, you'd simply add a new top level attribute
> which ties all the net_device specific statistics to a top level
> attribute and we can add the non net_device specific stats later on.
> We can't do that later on though without breaking ABI so that would
> have to go in with the first iteration.
agreed
>
> We can preserve all the existing attribute formats, we simply have
> to introduce new attribute types which don't overlap and document
> which statistic identifier maps to which existing attribute id.
sure,

thanks Thomas.

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[roopa]

On 4/10/16, 11:28 AM, roopa wrote:
> On 4/10/16, 1:16 AM, Thomas Graf wrote:
[snip]
>>
>> This currently ties everything to a net_device with a selector to
>> include certain bits of that net_device. How about we take it half a
>> step further and allow for non net_device stats such as IP, TCP,
>> routing or ipsec stats to be retrieved as well?
> yes, absolutely. and that is also the goal.
>> A simple array of nested attributes replacing IFLA_STATS_* would
>> allow for that, e.g.
>>
>> 1. {.type = ST_IPSTATS, value = { ...} }
>>
>> 2. {.type = ST_LINK, .value = {
>>     {.type = ST_LINK_NAME, .value = "eth0"},
>>     {.type = ST_LINK_Q, .value = 10}
>>   }}
>>
>> 3. ...
> One thing though,  Its unclear to me if we absolutely need the additional nest.
> Every stats netlink msg has an ifindex in the header (if_stats_msg) if the scope
> of the stats is a netdev. If the msg does not have an ifindex in the if_stats_msg,
> it represents a global stat. ie Cant a dump, include other stats netlink msgs after
> all the netdev msgs are done when the filter has global stat filters ?.
> same will apply to RTM_GETSTATS (without NLM_F_DUMP).
>
> Since the msg may potentially have more nest levels
> in the IFLA_EXT_STATS categories, just trying to see if i can avoid adding another
> top-level nest. We can sure add it if there is no other way to include global
> stats in the same dump.
>

Just wanted to elaborate on what i was trying to say:

Top level stats attributes can be netdev or global attributes: We can include string "LINK" in
the names of all stats belonging to a netdev to make it easier to recognize the netdev stats (example):
 IFLA_STATS_LINK64,           (netdev)
 IFLA_STATS_LINK_INET6,    (netdev)
 IFLA_STATS_TCP,                (non-netdev, global tcp stats)

RTM_GETSTATS (NLM_F_DUMP)  with user given filter_mask {
        If filter-mask contains any link stats, start with per netdev stats messages:
        { if_stats_msg.ifindex = 1,
          if_stats_msg.filter_mask = mask of included link stats,
          <stats attributes> }

        { if_stats_msg.ifindex = 2,
          if_stats_msg.filter_mask = mask of included link stats,
          <stats attributes> }

        global stats (if user given filter mask contains global filters.):
        { if_stats_msg.ifindex = 0,
          if_stats_msg.filter_mask = mask of included global stats,
          <stats attributes> }
}

We will need a field in netlink_callback to indicate global or netdev stats when the stats
crosses skb boundaries. A single nlmsg cannot have both netdev and global stats.

Non-dump RTM_GETSTATS examples:

RTM_GETSTATS with valid ifindex and filter_mask {
        filter_mask cannot have global stats (return -EINVAL)
        { if_stats_msg.ifindex = <user_given_ifindex>,
          if_stats_msg.filter_mask = mask of included link stats,
          <stats attributes> }
}

RTM_GETSTATS with ifindex = 0 and filter_mask {
        filter_mask cannot have link stats (return -EINVAL)
        { if_stats_msg.ifindex = 0,
          if_stats_msg.filter_mask = mask of included global link stats,
          <stats attributes> }
}


Will this not work ?

Thanks,
Roopa

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Thomas Graf]

On 04/11/16 at 08:53pm, roopa wrote:
> Top level stats attributes can be netdev or global attributes: We can include string "LINK" in
> the names of all stats belonging to a netdev to make it easier to recognize the netdev stats (example):
>  IFLA_STATS_LINK64,           (netdev)
>  IFLA_STATS_LINK_INET6,    (netdev)
>  IFLA_STATS_TCP,                (non-netdev, global tcp stats)

This is fine as well. It means that we cant mix netdev and non-netdev
stats or stats for multiple netdevs in the same request which would
not be the case if you nest it and have a top level attribute which
is a list of requests. That may be borderline to overengineering
though so I'm fine this as well.

> We will need a field in netlink_callback to indicate global or netdev stats when the stats
> crosses skb boundaries. A single nlmsg cannot have both netdev and global stats.

I would treat each IFLA_STATS_ as its own nlmsg in the reply and
enforce an NLM_F_DUMP request for any multi request message.

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Jamal Hadi Salim]

On 16-04-12 09:21 AM, Thomas Graf wrote:
> On 04/11/16 at 08:53pm, roopa wrote:
>> Top level stats attributes can be netdev or global attributes: We can include string "LINK" in
>> the names of all stats belonging to a netdev to make it easier to recognize the netdev stats (example):
>>   IFLA_STATS_LINK64,           (netdev)
>>   IFLA_STATS_LINK_INET6,    (netdev)
>>   IFLA_STATS_TCP,                (non-netdev, global tcp stats)
>
> This is fine as well. It means that we cant mix netdev and non-netdev
> stats or stats for multiple netdevs in the same request which would
> not be the case if you nest it and have a top level attribute which
> is a list of requests. That may be borderline to overengineering
> though so I'm fine this as well.


Well - using a subheader which has ifindex on it for non-netdev stats
seems wrong then.

cheers,
jamal

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[roopa]

On 4/13/16, 5:11 AM, Jamal Hadi Salim wrote:
> On 16-04-12 09:21 AM, Thomas Graf wrote:
>> On 04/11/16 at 08:53pm, roopa wrote:
>>> Top level stats attributes can be netdev or global attributes: We can include string "LINK" in
>>> the names of all stats belonging to a netdev to make it easier to recognize the netdev stats (example):
>>>   IFLA_STATS_LINK64,           (netdev)
>>>   IFLA_STATS_LINK_INET6,    (netdev)
>>>   IFLA_STATS_TCP,                (non-netdev, global tcp stats)
>>
>> This is fine as well. It means that we cant mix netdev and non-netdev
>> stats or stats for multiple netdevs in the same request which would
>> not be the case if you nest it and have a top level attribute which
>> is a list of requests. That may be borderline to overengineering
>> though so I'm fine this as well.
>
>
> Well - using a subheader which has ifindex on it for non-netdev stats
> seems wrong then.
I think its ok for ifindex to be optional.
Almost all msg types have subheaders with ifindex and it is almost always
optional in the GET request (eg RTM_GETLINK with NLM_F_DUMP ifindex is
optional and never checked). Here we make ifindex optional both ways.

Its optional in the GET request with NLM_F_DUMP and it is also optional
in the msg to userspace (NEW). And no ifindex in NEW will indicate that the
message is not tied to a specific netdev (ie global stats).

Thanks!

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Fri,  8 Apr 2016 23:38:11 -0700

> This patch adds a new RTM_GETSTATS message to query link stats via netlink
> from the kernel. RTM_NEWLINK also dumps stats today, but RTM_NEWLINK
> returns a lot more than just stats and is expensive in some cases when
> frequent polling for stats from userspace is a common operation.

Great work.  One thing catches my eye:

> +	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK64)) {
> +		attr = nla_reserve(skb, IFLA_STATS_LINK64,
> +				   sizeof(struct rtnl_link_stats64));
> +		if (!attr)
> +			return -EMSGSIZE;
> +
> +		stats = dev_get_stats(dev, &temp);
> +
> +		copy_rtnl_link_stats64(nla_data(attr), stats);

This extra copy bothers me, so I tried to figure out what is going
on here.

dev_get_stats() always returns the rtnl_link_stats64 pointer it was
given.  We should be able to pass, therefore, nla_data(attr), straight
there to avoid the copy.

Bonding even uses dev_get_stats() in this way.

The existing rtnl_fill_stats() can be improved similarly but is of
course a separate change.  In that case, we'd do something like:

	struct rtnl_link_stats64 *sp;

	attr = nla_reserve(skb, IFLA_STATS64,
			   sizeof(struct rtnl_link_stats64));
	if (!attr)
		return -EMSGSIZE;

	sp = nla_data(attr);
	dev_get_stats(dev, sp);

	attr = nla_reserve(skb, IFLA_STATS,
			   sizeof(struct rtnl_link_stats));
	if (!attr)
		return -EMSGSIZE;

	copy_rtnl_link_stats(nla_data(attr), sp);

Thanks.

Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats

[roopa]

On 4/13/16, 9:19 PM, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Fri,  8 Apr 2016 23:38:11 -0700
>
>> This patch adds a new RTM_GETSTATS message to query link stats via netlink
>> from the kernel. RTM_NEWLINK also dumps stats today, but RTM_NEWLINK
>> returns a lot more than just stats and is expensive in some cases when
>> frequent polling for stats from userspace is a common operation.
> Great work.  One thing catches my eye:
>
>> +	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK64)) {
>> +		attr = nla_reserve(skb, IFLA_STATS_LINK64,
>> +				   sizeof(struct rtnl_link_stats64));
>> +		if (!attr)
>> +			return -EMSGSIZE;
>> +
>> +		stats = dev_get_stats(dev, &temp);
>> +
>> +		copy_rtnl_link_stats64(nla_data(attr), stats);
> This extra copy bothers me, so I tried to figure out what is going
> on here.
>
> dev_get_stats() always returns the rtnl_link_stats64 pointer it was
> given.  We should be able to pass, therefore, nla_data(attr), straight
> there to avoid the copy.

nice catch. I picked this up straight from rtnl_fill_stats. agree, also thanks
 for the example below.

>
> Bonding even uses dev_get_stats() in this way.
>
> The existing rtnl_fill_stats() can be improved similarly but is of
> course a separate change.  In that case, we'd do something like:
>
> 	struct rtnl_link_stats64 *sp;
>
> 	attr = nla_reserve(skb, IFLA_STATS64,
> 			   sizeof(struct rtnl_link_stats64));
> 	if (!attr)
> 		return -EMSGSIZE;
>
> 	sp = nla_data(attr);
> 	dev_get_stats(dev, sp);
>
> 	attr = nla_reserve(skb, IFLA_STATS,
> 			   sizeof(struct rtnl_link_stats));
> 	if (!attr)
> 		return -EMSGSIZE;
>
> 	copy_rtnl_link_stats(nla_data(attr), sp);

I will submit a separate patch for this with some testing.

Will send a v3 out before end of this week.

Thank you!.