From: Jamal Hadi Salim <jhs@mojatatu•com>
To: roopa <roopa@cumulusnetworks•com>
Cc: netdev@vger•kernel.org, davem@davemloft•net
Subject: Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats
Date: Sun, 10 Apr 2016 09:48:36 -0400 [thread overview]
Message-ID: <570A59B4.30007@mojatatu.com> (raw)
In-Reply-To: <57094326.9040009@cumulusnetworks.com>
On 16-04-09 02:00 PM, roopa wrote:
> This EXTENDED_HW_STATS is for ethtool like extended hw stats. This is keeping in
> mind that we want to also move ethtool to netlink in the future and with switchdev
> it becomes more necessary that we provide all stats closer to the other netdev stats.
> So far hw extended stats have always been available through this separate ethtool
> channel. The intent here is to unify the api for extended hw and software only stats.
Ok, so these are _not_ the stats which are broken down by packet size
ranges which are quiet common; but rather "proprietary" per port
type h/w stats? I browsed a couple of users of ethtool_stats and i see
they return proprietary looking 64 bit counters (batman for example
has a very strange meaning to those stats).
What i meant is a lot of ASICS have counters for byte ranges
[64bytes-128bytes], [128bytes-256bytes], etc - sorry i cant pin a name
to those but i am sure you have seen them and i thought those at minimal
need their own TLV since they are always fixed.
> XSTATS is per netdev can be included as a nested attribute inside IFLA_EXTENDED_STATS
> which are per netdev. bridge vlan stats will also fall here.
>
And you are going to distinguish which come from hardware and which are
software derived?
> And this api provides netdev specific stats. We have always mapped all
> asic stats to the switch port netdev stats. and this api does not cover the non-netdev specific stats.
> If you are for example asking for stats for a hardware offloaded bridge, then yes, they will fall here
> and be available on the bridge netdev. For asic stats that don't map to any netdev, devlink will be an
> appropriate infrastructure IMO.
>
> I am not sure if I answered your question :).
>
It is useful to have this discussion; unfortunately these user APIS once
are in will never be allowed to change. The answers could come
in time.
>> Should such a command then not be rejected with an error code?
> Dumps with no data are not rejected with an error code AFAIK. ie they don't return
> -ENODATA. This is consistent with all other dumps (unless i missed it).
> But, if there is a need for an error code, i can certainly check again.
>
It is mostly because you chose a whitelist filter i.e you list what
is allowed to be sent back. And if such a list is missing there
needs to be an opposite default (which is a deny all).
>>> +/* STATS section */
>>> +
>>> +struct if_stats_msg {
>>> + __u8 family;
>>> + __u32 ifindex;
>>> + __u32 filter_mask;
>>> +};
>>
>> Needs to be 32 bit aligned.
>> Do you need 32 bits for the filter mask?
> yes, i think we should keep it minimum 32 bits.
Ok, that is fine then. I thought it wont exceed
3-4 per port type but i could be wrong. 32 bits
should be safer.
>> Perhaps a 16bit mask and an 8bit pad for future use.
>>
>> struct if_stats_msg {
>> __u32 ifindex;
>> __u16 filter_mask;
>> __u8 family;
>> __u8 pad; /* future use */
>> };
>>
>> Or you could reverse those (from smallest to largest).
>
> The __u8 family needs to be the first field in the structure and at the first byte in the header data.
> hence family is first and i added the others after that. It follows the format for existing such structs (for other message types).
>
True, but it must be 32 bit aligned. So something like:
struct if_stats_msg {
__u8 family;
__u8 pad1;
__u16 pad2;
__u32 ifindex;
__u32 filter_mask;
}
> Yeah i remember :). But deferring it for a later incremental feature. That needs some more thought.
NP ;->
> Right now there is an urgent need to get the basic get stats api for a bunch of other stats: mpls, bridge vlan etc.
> Because it is not clean to extend the current stats infra part of the link message for this. So trying to get this in first.
>
Agreed.
The only thing i strongly feel about is the if_stats_msg - please fix
that and lets get at least the basics in. We can resolve other things
with further discussions.
> And this patchset only adds a handler for RTM_NEWSTATS dump and get stats. Your stats events request should be part of the RTM_NEWSTATS handler and can include other attributes (like timeout) in the future.
>
Ok.
cheers,
jamal
> Thanks,
> Roopa
>
next prev parent reply other threads:[~2016-04-10 13:48 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-09 6:38 [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message to dump link stats Roopa Prabhu
2016-04-09 14:30 ` Jamal Hadi Salim
2016-04-09 18:00 ` roopa
2016-04-10 13:48 ` Jamal Hadi Salim [this message]
2016-04-10 19:17 ` roopa
2016-04-10 8:16 ` Thomas Graf
2016-04-10 18:28 ` roopa
2016-04-12 3:53 ` roopa
2016-04-12 13:21 ` Thomas Graf
2016-04-13 12:11 ` Jamal Hadi Salim
2016-04-14 6:36 ` roopa
2016-04-14 4:19 ` David Miller
2016-04-14 6:35 ` 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=570A59B4.30007@mojatatu.com \
--to=jhs@mojatatu$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=roopa@cumulusnetworks$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox