public inbox for netdev@vger.kernel.org 
 help / color / mirror / Atom feed

From: roopa <roopa@cumulusnetworks•com>
To: David Miller <davem@davemloft•net>
Cc: netdev@vger•kernel.org, jhs@mojatatu•com
Subject: Re: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats
Date: Wed, 24 Feb 2016 21:25:50 -0800	[thread overview]
Message-ID: <56CE905E.4030601@cumulusnetworks.com> (raw)
In-Reply-To: <20160224.164003.357595978348193472.davem@davemloft.net>

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!.

next prev parent reply	other threads:[~2016-02-25  5:25 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-23  6:01 [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats Roopa Prabhu
2016-02-23  9:26 ` Rosen, Rami
2016-02-25  4:59   ` roopa
2016-02-24 21:40 ` David Miller
2016-02-25  5:25   ` roopa [this message]
2016-02-25 16:26     ` David Miller
2016-02-26  5:01       ` roopa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56CE905E.4030601@cumulusnetworks.com \
    --to=roopa@cumulusnetworks$(echo .)com \
    --cc=davem@davemloft$(echo .)net \
    --cc=jhs@mojatatu$(echo .)com \
    --cc=netdev@vger$(echo .)kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox