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

[roopa <roopa@cumulusnetworks.com>]

On 2/24/16, 1:40 PM, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Mon, 22 Feb 2016 22:01:33 -0800
>
>> 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.
> 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!.