Message-ID: <56E7BC0B.3070107@cumulusnetworks.com>
Date:	Tue, 15 Mar 2016 00:38:51 -0700
From:	roopa <roopa@...ulusnetworks.com>
To:	Jiri Pirko <jiri@...nulli.us>
CC:	netdev@...r.kernel.org, jhs@...atatu.com, davem@...emloft.net
Subject: Re: [PATCH net-next 1/2] rtnetlink: add new RTM_GETSTATS message
 to dump link stats

On 3/15/16, 12:28 AM, Jiri Pirko wrote:
> Tue, Mar 15, 2016 at 07:24:22AM CET, roopa@...ulusnetworks.com wrote:
>> On 3/14/16, 12:04 PM, Jiri Pirko wrote:
>>> Mon, Mar 14, 2016 at 07:45:23PM CET, roopa@...ulusnetworks.com wrote:
>>>> On 3/14/16, 7:51 AM, Jiri Pirko wrote:
>>>>> Sun, Mar 13, 2016 at 02:56:25AM CET, roopa@...ulusnetworks.com wrote:
>>>>>> From: Roopa Prabhu <roopa@...ulusnetworks.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 modified iproute2 ifstat.
>>>>>>
>>>>>> Suggested-by: Jamal Hadi Salim <jhs@...atatu.com>
>>>>>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>>>>>> ---
>> [snip]
>>>>>> +
>>>>>> +struct if_stats_msg {
>>>>>> +	__u8  family;
>>>>>> +	__u32 ifindex;
>>>>>> +	__u32 filter_mask;
>>>>> This limit future extension to only 32 groups of stats. I can imagine
>>>>> that more than that can be added, easily.
>>>> I thought about that, but it is going to be a while before we run out of the u32.
>>>> Most of the other stats will be nested like per logical interface stats or
>>>> per hw stats. If we do run out of them, in the future we could add a netlink
>>>> attribute for extended filter mask to carry more bits (similar to IFLA_EXT_MASK).
>>>> I did also start with just having a IFLA_STATS_EXT_MASK like attribute
>>>> to begin with, but since no stats are dumped by default, having a way to easily specify
>>>> mask in the hdr will be easier on apps. And this will again be a u32 anyways.
>>> I believe that using *any* structs to send over netlink is a mistake.
>>> Netlink is capable to transfer everything using attrs. Easy to compose,
>>> easy to parse. easy to extend. Couple of more bytes in the message? So what?
>>> For newly introduced things, I suggest to do this properly.
>> Jiri, I hear you. I don't prefer structs for netlink attributes either.
> Looks like you clearly prefer structs, otherwise we wouldn't be having
> this discussion.
>
>
>> But in this case, the struct is for the msg hdr which immediately follows the netlink
>> header. Its not an attribute value. see my last reply below. rtnetlink_rcv_msg does assume
>> a struct and family right after the netlink header.
>> All messages define this struct (see struct ndmsg, or ifinfomsg, rtmsg, br_port_msg etc).
>> so it is required.
> Okay. So let's kee[ that struct as small as possible. Containing only
> family and ifindex. That should be enough. 

how does it matter if we have reached an agreement that the struct is required ?.
unlike other messages, a filter_mask is an important and must attribute for
stats. If you are worried about us running out of bits in u32, the netlink attribute you will
 define for the filter_mask will also be u32 to begin with.
So, i don't understand what we gain from making filter_mask a separate attribute right now.
I would have agreed with your argument if filter_mask was optional.


>> And I do think this struct simplifies a minimum request message and
>> I have also realized that it really helps if this struct contains basic minimum required
>> attributes. Ifindex as a filter really helps with RTM_GETSTATS when not used with NLM_F_DUMP
>> and filter_mask is important for RTM_GETSTATS with NLM_F_DUMP because without
>> a filter no stats are reported. so, making it part of the base message simplifies the stats
>> request message from app perspective.
> I don't understand this argument. As I wrote earlier, user app can
> easily specify filter mask by flag attrs. It is very easy.
>
>
>> yes the struct cannot be extended, but further extensions can be done as netlink attributes.
> Exactly, we now now that this is not extendable, we know that if will
> likely get extended, yet you still argue for the non-extendable
> approach. I don't get it, sorry :(
>

Powered by blists - more mailing lists