Re: [PATCH net-next v3 1/2] rtnetlink: add new RTM_GETSTATS message to query stats

[roopa]

On 4/16/16, 12:49 AM, Thomas Graf wrote:
> On 04/15/16 at 08:28pm, Roopa Prabhu wrote:
>> +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, >dev_base_head, dev_list) {
>> +min_ifinfo_dump_size = max_t(u16, min_ifinfo_dump_size,
>> + if_nlmsg_stats_size(dev,
>> + filter_mask));
>> +}
> Iterating over all net_devices in the namespace is quite an expensive
> operation and it would now be done twice.
>
> I understand that this code is taken over from rtnl_calcit() but there
> the cost is at least only paid if ext_filter_mask is actually set and
> the user opts into additional statistics.
>
> I wonder if we can reduce the cost for the stats interface as its
> purpose is to be minimal cost. I suggest we only add the loop once we
> have an extension which actually depends on it. We can then try and
> figure out to not require it.
ok, ack. Its not absolutely necessary right now with the one link filter stats
I am adding support for. We can bring it back later when we see a first instance
 which makes it necessary.

In which case, I am going to trim down the series to absolute minimal.
just rtnl link stats64. I want to drop the ipv6 patch anyways.
With it I will also drop the general af stats handling. Will get it back when 
we get to the first
proper af stats implementation (hopefully mpls).

With this, the calcit for RTM_GETSTATS will be NULL, until we add the 
implementation for the other
stats attributes.

Re: [PATCH net-next v3 1/2] rtnetlink: add new RTM_GETSTATS message to query stats

[Thomas Graf]

On 04/15/16 at 08:28pm, Roopa Prabhu wrote:
> +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, >dev_base_head, dev_list) {
> + min_ifinfo_dump_size = max_t(u16, min_ifinfo_dump_size,
> +  if_nlmsg_stats_size(dev,
> +  filter_mask));
> + }

Iterating over all net_devices in the namespace is quite an expensive
operation and it would now be done twice.

I understand that this code is taken over from rtnl_calcit() but there
the cost is at least only paid if ext_filter_mask is actually set and
the user opts into additional statistics.

I wonder if we can reduce the cost for the stats interface as its
purpose is to be minimal cost. I suggest we only add the loop once we
have an extension which actually depends on it. We can then try and
figure out to not require it.

[PATCH net-next v3 1/2] rtnetlink: add new RTM_GETSTATS message to query stats

[Roopa Prabhu]

From: Roopa Prabhu 

This patch adds a new RTM_GETSTATS message to query stats via netlink
from the kernel. RTM_NEWLINK also dumps link 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 stats from the kernel. 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_LINK_64]  = { .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_STATS_LINK_MPLS  (nested. for mpls/mdev stats)
- IFLA_STATS_LINK_EXTENDED (nested. extended software netdev stats like bridge,
  vlan, vxlan etc)
- IFLA_STATS_LINK_HW_EXTENDED (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. filter mask
can be specified in the new hdr 'struct if_stats_msg' for stats messages.
Other important field in the header is the ifindex.

This api can be used for global stats (eg tcp) in the future. When global
stats are included in a stats msg, the ifindex in the header
must be zero. A single stats message cannot contain both global and
netdev specific stats. To easily distinguish them, netdev specific stat
attributes name are prefixed with IFLA_STATS_LINK_

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 
Signed-off-by: Roopa Prabhu 
---
 include/net/rtnetlink.h|   5 ++
 include/uapi/linux/if_link.h   |  23 +
 include/uapi/linux/rtnetlink.h |   5 ++
 net/core/rtnetlink.c   | 199 +
 4 files changed, 232 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..ab740fe 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -780,4 +780,27 @@ enum {
 
 #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
 
+/* STATS section */
+
+struct if_stats_msg {
+   __u8  family;
+   __u8  pad1;
+   __u16 pad2;
+   __u32 ifindex;
+   __u32 filter_mask;
+};
+
+/* A stats attribute can be netdev specific or a global stat.
+ * For netdev stats, lets use the prefix IFLA_STATS_LINK_*
+ */
+enum {
+   IFLA_STATS_UNSPEC,
+   IFLA_STATS_LINK_64,
+   __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 a7a3d34..2a8abe0 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3444,6 +3444,202 @@ 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)
+{
+   struct if_stats_msg *ifsm;
+   struct nlmsghdr *nlh;
+