Tue, Mar 15, 2016 at 07:24:22AM CET, ro...@cumulusnetworks.com wrote:
>On 3/14/16, 12:04 PM, Jiri Pirko wrote:
>> Mon, Mar 14, 2016 at 07:45:23PM CET, ro...@cumulusnetworks.com wrote:
>>> On 3/14/16, 7:51 AM, Jiri Pirko wrote:
>>>> Sun, Mar 13, 2016 at 02:56:25AM CET, ro...@cumulusnetworks.com wrote:
>>>>> From: Roopa Prabhu <ro...@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.
>>>>>
>>>>> 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 <j...@mojatatu.com>
>>>>> Signed-off-by: Roopa Prabhu <ro...@cumulusnetworks.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.
>
>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 :(
>
>
>
>
>>
>>
>>>
>>>> Why don't you use nested
>>>> attribute IFLA_STATS_FILTER with flag attributes for every type?
>>>> That
>>>> would be easily extendable.
>>> a u8 for each stats selector seems like an overkill.
>>>> Using netlink header struct for this does not look correct to me.
>>>> In past, this was done lot of times and turned out to be a problem later.
>>>>
>>>>
>>> I started with not adding it, but rtnetlink rcv handler looks for family
>>> in the hdr. And hence all of the messages have a struct header
>>> with family as the first field (you can correct me if you find that it is
>>> not necessary.)
>>>
>>>
>
