Re: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[roopa]

On 2/25/16, 8:26 AM, David Miller wrote:
> From: roopa 
> Date: Wed, 24 Feb 2016 21:25:50 -0800
>
>> I did go back and forth on the attribute vs mask.
>> cosmetic but, i guess i did not feel good about having to redefine every 
>> attribute again
>> for the bitmap filter ...and i anticipate the list of stat attributes to 
>> grow overtime (maybe there is a better way).
>> enum {
>>IFLA_LINK_STATS64,
>>IFLA_LINK_INET6_STATS,
>>IFLA_LINK_MPLS_STATS,
>>__IFLA_LINK_STATS_MAX,
>> };
>>
>> #define IFLA_LINK_STATS64_FILTER  0
>> #define IFLA_LINK_INET6_STATS_FILTER (1 << 0)
>> #define IFLA_LINK_MPLS_STATS_FILTER (2 << 0)
> The filter for X is always (1 << X), so we could work with something like:
>
> #define IFLA_LINK_FILTER_BIT(ATTR)(1 << (ATTR))
i like it
>
> Which seems to suggest that emitting no stats unless they are explicitly 
> requested in
> the bitmask makes sense because:
>
> 1) You don't have to special case STATS64 in the filter mask
>
> 2) Application are forced to be aware of filtering and thus choose only
>what they want to see
>
> How about this?
I am ok with it. It keeps the filtering mask handling consistent. Its a bit 
inconsistent with the other dump functions,
but this gives user full control of what combination of stats she wants. For 
something like stats, i think this makes sense.

Thanks again!
Roopa

Re: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[David Miller]

From: roopa 
Date: Wed, 24 Feb 2016 21:25:50 -0800

> I did go back and forth on the attribute vs mask.
> cosmetic but, i guess i did not feel good about having to redefine every 
> attribute again
> for the bitmap filter ...and i anticipate the list of stat attributes to grow 
> overtime (maybe there is a better way).
> enum {
>IFLA_LINK_STATS64,
>IFLA_LINK_INET6_STATS,
>IFLA_LINK_MPLS_STATS,
>__IFLA_LINK_STATS_MAX,
> };
> 
> #define IFLA_LINK_STATS64_FILTER  0
> #define IFLA_LINK_INET6_STATS_FILTER (1 << 0)
> #define IFLA_LINK_MPLS_STATS_FILTER (2 << 0)

The filter for X is always (1 << X), so we could work with something like:

#define IFLA_LINK_FILTER_BIT(ATTR)  (1 << (ATTR))

Which seems to suggest that emitting no stats unless they are explicitly 
requested in
the bitmask makes sense because:

1) You don't have to special case STATS64 in the filter mask

2) Application are forced to be aware of filtering and thus choose only
   what they want to see

How about this?

Re: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[roopa]

On 2/24/16, 1:40 PM, David Miller wrote:
> From: Roopa Prabhu 
> Date: Mon, 22 Feb 2016 22:01:33 -0800
>
>> From: Roopa Prabhu 
>>
>> 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.
> Even worse, we push two copies of the same exact stats.  Once to give
> the (deprecated) 32-bit rtnl stats, and once to give the 64-bit ones.
>
> This is rather rediculous, but was probably done for compatability.
i would think so.
>
>> To begin with, this patch adds the following types of stats (which are
>> also available via RTM_NEWLINK today):
>>
>> struct nla_policy ifla_stats_policy[IFLA_LINK_STATS_MAX + 1] = {
>> [IFLA_LINK_STATS]   = { .len = sizeof(struct rtnl_link_stats) },
>> [IFLA_LINK_STATS64] = { .len = sizeof(struct rtnl_link_stats64) 
>> },
>> [IFLA_LINK_INET6_STATS] = {. type = NLA_NESTED },
>> };
> I think we should skip the old 32-bit stats and only provide the full
> native 64-bit rtnl_link_stats64.  Apps have to have changes to use
> this new facility, therefore they can be adjusted for the (extremely
> minimal) amount of work necessary to understand 64-bit stats if
> necessary.
>
> By using rtnl_fill_stats() we would unnecessarily continue to send
> duplicate and extra information unnecessarily.
>
> We have the chance to fix this here, so let's take advantage of the
> opportunity to promote the data structure which can then be a first
> class citizen not only in the kernel but in userspace too.
agree, very much
>
>> Filtering stats (if the user wanted to query just ipv6 stats ?):
>> a) I have a version of the patch that uses 'struct ifinfomsg->ifi_family'
>> as a filter from userspace. But, given that in the future we may want to
>> not just filter by family, I have dropped it in favor of
>> attribute filtering (not included in this patch but point b) below).
>>
>> b) RTM_GETSTATS message to contain attributes in the request message
>>
>>  nlmsg_parse(nlh, IFLA_LINK_STATS_MAX, ifla_stats_policy...)
>>
>>  if tb[IFLA_LINK_INET6_STATS]
>>  include ipv6 stats in the dump
>>
>>  if tb[IFLA_LINK_MPLS_STATS]
>>  include mpls stats in the dump
> I wonder if an additive method is better.  Provide IFLA_LINK_STATS64
> by default, and the user has to ask for more.
>
> Or the user gets nothing, and a simply u32 bitmap in the request acts
> as a selector, with enable bits for each stat type.
>
> It really has to be easy to carve out exactly the information the user
> wants, and be very minimal by default in order to be as efficient as
> possible.
ok, agreed here too. I like the default minimal approach. keeps it simple and 
efficient.

I did go back and forth on the attribute vs mask.
cosmetic but, i guess i did not feel good about having to redefine every 
attribute again
for the bitmap filter ...and i anticipate the list of stat attributes to grow 
overtime (maybe there is a better way).
enum {
   IFLA_LINK_STATS64,
   IFLA_LINK_INET6_STATS,
   IFLA_LINK_MPLS_STATS,
   __IFLA_LINK_STATS_MAX,
};

#define IFLA_LINK_STATS64_FILTER  0
#define IFLA_LINK_INET6_STATS_FILTER (1 << 0)
#define IFLA_LINK_MPLS_STATS_FILTER (2 << 0)

Will post non-RFC with the changes later this week.

Thanks for the feedback!.

Re: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[roopa]

On 2/23/16, 1:26 AM, Rosen, Rami wrote:
> Hi,
>
> + if (!dev)
> + return -ENODEV;
> +
> + nskb = nlmsg_new(if_nlmsg_stats_size(dev), GFP_KERNEL);
> + if (!nskb)
> + return -ENOBUFS;
> +
> + err = rtnl_fill_statsinfo(nskb, dev, RTM_NEWSTATS,
> +   NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0, 0);
> + if (err < 0) {
>
> It should be here:  -EMSGSIZE implies BUG in if_nlmsg_stats_size  (instead of 
> if_nlmsg_size)
>
> + /* -EMSGSIZE implies BUG in if_nlmsg_size */
> + WARN_ON(err == -EMSGSIZE);
> + kfree_skb(nskb);
> + } else {
> + err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
> + }
>
>
> Other than that, it seems ok, thanks for this patch!
>
>
will fix it, thanks for the review.

Re: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[David Miller]

From: Roopa Prabhu 
Date: Mon, 22 Feb 2016 22:01:33 -0800

> From: Roopa Prabhu 
> 
> 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.

Even worse, we push two copies of the same exact stats.  Once to give
the (deprecated) 32-bit rtnl stats, and once to give the 64-bit ones.

This is rather rediculous, but was probably done for compatability.

> To begin with, this patch adds the following types of stats (which are
> also available via RTM_NEWLINK today):
> 
> struct nla_policy ifla_stats_policy[IFLA_LINK_STATS_MAX + 1] = {
> [IFLA_LINK_STATS]   = { .len = sizeof(struct rtnl_link_stats) },
> [IFLA_LINK_STATS64] = { .len = sizeof(struct rtnl_link_stats64) },
> [IFLA_LINK_INET6_STATS] = {. type = NLA_NESTED },
> };

I think we should skip the old 32-bit stats and only provide the full
native 64-bit rtnl_link_stats64.  Apps have to have changes to use
this new facility, therefore they can be adjusted for the (extremely
minimal) amount of work necessary to understand 64-bit stats if
necessary.

By using rtnl_fill_stats() we would unnecessarily continue to send
duplicate and extra information unnecessarily.

We have the chance to fix this here, so let's take advantage of the
opportunity to promote the data structure which can then be a first
class citizen not only in the kernel but in userspace too.

> Filtering stats (if the user wanted to query just ipv6 stats ?):
> a) I have a version of the patch that uses 'struct ifinfomsg->ifi_family'
> as a filter from userspace. But, given that in the future we may want to
> not just filter by family, I have dropped it in favor of
> attribute filtering (not included in this patch but point b) below).
> 
> b) RTM_GETSTATS message to contain attributes in the request message
> 
>   nlmsg_parse(nlh, IFLA_LINK_STATS_MAX, ifla_stats_policy...)
> 
>   if tb[IFLA_LINK_INET6_STATS]
>   include ipv6 stats in the dump
> 
>   if tb[IFLA_LINK_MPLS_STATS]
>   include mpls stats in the dump

I wonder if an additive method is better.  Provide IFLA_LINK_STATS64
by default, and the user has to ask for more.

Or the user gets nothing, and a simply u32 bitmap in the request acts
as a selector, with enable bits for each stat type.

It really has to be easy to carve out exactly the information the user
wants, and be very minimal by default in order to be as efficient as
possible.

RE: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[Rosen, Rami]

Hi,

+   if (!dev)
+   return -ENODEV;
+
+   nskb = nlmsg_new(if_nlmsg_stats_size(dev), GFP_KERNEL);
+   if (!nskb)
+   return -ENOBUFS;
+
+   err = rtnl_fill_statsinfo(nskb, dev, RTM_NEWSTATS,
+ NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0, 0);
+   if (err < 0) {

It should be here:  -EMSGSIZE implies BUG in if_nlmsg_stats_size  (instead of 
if_nlmsg_size)

+   /* -EMSGSIZE implies BUG in if_nlmsg_size */
+   WARN_ON(err == -EMSGSIZE);
+   kfree_skb(nskb);
+   } else {
+   err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
+   }


Other than that, it seems ok, thanks for this patch!

Regards,
Rami Rosen
Intel Corporation

[PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[Roopa Prabhu]

From: Roopa Prabhu 

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.

To begin with, this patch adds the following types of stats (which are
also available via RTM_NEWLINK today):

struct nla_policy ifla_stats_policy[IFLA_LINK_STATS_MAX + 1] = {
[IFLA_LINK_STATS]   = { .len = sizeof(struct rtnl_link_stats) },
[IFLA_LINK_STATS64] = { .len = sizeof(struct rtnl_link_stats64) },
[IFLA_LINK_INET6_STATS] = {. type = NLA_NESTED },
};

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

Most of the above attributes maintain their current types like 'struct
rtnl_link_stats' because there are many apps today that understand this
format. These attributes can be made nested or new nested quivalents can
be added in the future if the need arises.

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

Filtering stats (if the user wanted to query just ipv6 stats ?):
a) I have a version of the patch that uses 'struct ifinfomsg->ifi_family'
as a filter from userspace. But, given that in the future we may want to
not just filter by family, I have dropped it in favor of
attribute filtering (not included in this patch but point b) below).

b) RTM_GETSTATS message to contain attributes in the request message

nlmsg_parse(nlh, IFLA_LINK_STATS_MAX, ifla_stats_policy...)

if tb[IFLA_LINK_INET6_STATS]
include ipv6 stats in the dump

if tb[IFLA_LINK_MPLS_STATS]
include mpls stats in the dump

This patch has been minimally tested with a patch to iproute2 ifstat.

Jamals original thought at netdev/netconf also had a RTM_NEWSTATS notification
that people can register to for stats updates. This patch does not include those
changes, but those can be added separately.

Suggested-by: Jamal Hadi Salim 
Signed-off-by: Roopa Prabhu 
---
 include/net/rtnetlink.h|   3 +
 include/uapi/linux/if_link.h   |  10 +++
 include/uapi/linux/rtnetlink.h |   6 ++
 net/core/rtnetlink.c   | 164 +
 net/ipv6/addrconf.c|  69 +++--
 5 files changed, 244 insertions(+), 8 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 2f87c1b..8892d21 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -131,6 +131,9 @@ 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);
+   int (*fill_link_af_stats)(struct sk_buff *skb,
+ const struct net_device 
*dev);
 };
 
 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 d452cea..3d20091 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -739,4 +739,14 @@ enum {
 
 #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
 
+/* STATS section */
+enum {
+   IFLA_LINK_STATS,
+   IFLA_LINK_STATS64,
+   IFLA_LINK_INET6_STATS,
+   __IFLA_LINK_STATS_MAX,
+};
+
+#define IFLA_LINK_STATS_MAX (__IFLA_LINK_STATS_MAX - 1)
+
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index ca764b5..be41654 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -139,6 +139,12 @@ enum {
RTM_GETNSID = 90,
 #define RTM_GETNSID RTM_GETNSID
 
+   RTM_NEWSTATS = 91,
+#define RTM_NEWSTATS RTM_GETSTATS
+
+   RTM_GETSTATS = 92,
+#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 62737f4..18af7a1 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3413,6 +3413,167 @@ out:
return err;
 }
 
+static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
+