RE: [patch net-next v8 2/3] net: core: Add offload stats to if_stats_msg
> -Original Message-
> From: Nikolay Aleksandrov [mailto:niko...@cumulusnetworks.com]
> Sent: Friday, September 09, 2016 4:39 PM
> To: Nikolay Aleksandrov
> Cc: Jiri Pirko ; Linux Kernel Network Developers
> ; David S. Miller
> ; Nogah Frankel ; Ido Schimmel
> ; Elad Raz
> ; Yotam Gigi ; Or Gerlitz
> ; ro...@cumulusnetworks.com;
> linvi...@tuxdriver.com; tg...@suug.ch; go...@cumulusnetworks.com;
> sfel...@gmail.com; s...@queasysnail.net; Eran Ben Elisha
> ; a...@plumgrid.com; eduma...@google.com;
> han...@stressinduktion.org; f.faine...@gmail.com;
> d...@cumulusnetworks.com
> Subject: Re: [patch net-next v8 2/3] net: core: Add offload stats to
> if_stats_msg
>
>
> > On Sep 9, 2016, at 4:36 PM, Nikolay Aleksandrov
> > wrote:
> >
> >>
> >> On Sep 8, 2016, at 9:19 AM, Jiri Pirko wrote:
> >>
> >> From: Nogah Frankel
> >>
> >> Add a nested attribute of offload stats to if_stats_msg
> >> named IFLA_STATS_LINK_OFFLOAD_XSTATS.
> >> Under it, add SW stats, meaning stats only per packets that went via
> >> slowpath to the cpu, named IFLA_OFFLOAD_XSTATS_CPU_HIT.
> >>
> >> Signed-off-by: Nogah Frankel
> >> Signed-off-by: Jiri Pirko
> >> ---
> >> include/uapi/linux/if_link.h | 9
> >> net/core/rtnetlink.c | 97
> >> ++--
> >> 2 files changed, 102 insertions(+), 4 deletions(-)
> >>
> > [snip]
> >> @@ -3655,6 +3725,22 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
> >> struct net_device *dev,
> >>}
> >>}
> >>
> >> + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS,
> >> + *idxattr)) {
> >> + attr = nla_nest_start(skb, IFLA_STATS_LINK_OFFLOAD_XSTATS);
> >> + if (!attr)
> >> + goto nla_put_failure;
> >> +
> >> + err = rtnl_get_offload_stats(skb, dev);
> >> + if (err == -ENODATA)
> >> + nla_nest_cancel(skb, attr);
> >> + else
> >> + nla_nest_end(skb, attr);
> >> +
> >> + if ((err) && (err != -ENODATA))
> >> + goto nla_put_failure;
> >> + }
> >> +
> >
> > Hmm, actually on a second read I think there’s a potential problem here.
> > Since you don’t set *idxattr and if the space
> > isn’t enough the dump will get restarted and this will lead to an infinite
> > loop.
>
> Err, poor choice of words. I meant the loop will be for userspace since the
> dump will err out at the same spot all the time so it
> won’t be able to ever finish. :-)
>
> > I.e. if the previous attributes filled the skb and there’s not enough room
> > for this one, it will return an error
> > but *idxattr will be == 0 from the previous attribute thus the whole dump
> > will be restarted (this is in case someone
> > requests this attribute with some of the others of course).
> >
> > Cheers,
> > Nik
> >
> >
I'll fix it. Thanks.
> >>nlmsg_end(skb, nlh);
> >>
> >>return 0;
> >> @@ -3712,6 +3798,9 @@ static size_t if_nlmsg_stats_size(const struct
> >> net_device *dev,
> >>}
> >>}
> >>
> >> + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS, 0))
> >> + size += rtnl_get_offload_stats_size(dev);
> >> +
> >>return size;
> >> }
> >>
> >> --
> >> 2.5.5
Re: [patch net-next v8 2/3] net: core: Add offload stats to if_stats_msg
> On Sep 8, 2016, at 9:19 AM, Jiri Pirko wrote:
>
> From: Nogah Frankel
>
> Add a nested attribute of offload stats to if_stats_msg
> named IFLA_STATS_LINK_OFFLOAD_XSTATS.
> Under it, add SW stats, meaning stats only per packets that went via
> slowpath to the cpu, named IFLA_OFFLOAD_XSTATS_CPU_HIT.
>
> Signed-off-by: Nogah Frankel
> Signed-off-by: Jiri Pirko
> ---
> include/uapi/linux/if_link.h | 9
> net/core/rtnetlink.c | 97 ++--
> 2 files changed, 102 insertions(+), 4 deletions(-)
>
[snip]
> @@ -3655,6 +3725,22 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
> struct net_device *dev,
> }
> }
>
> + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS,
> + *idxattr)) {
> + attr = nla_nest_start(skb, IFLA_STATS_LINK_OFFLOAD_XSTATS);
> + if (!attr)
> + goto nla_put_failure;
> +
> + err = rtnl_get_offload_stats(skb, dev);
> + if (err == -ENODATA)
> + nla_nest_cancel(skb, attr);
> + else
> + nla_nest_end(skb, attr);
> +
> + if ((err) && (err != -ENODATA))
> + goto nla_put_failure;
> + }
> +
Hmm, actually on a second read I think there’s a potential problem here. Since
you don’t set *idxattr and if the space
isn’t enough the dump will get restarted and this will lead to an infinite loop.
I.e. if the previous attributes filled the skb and there’s not enough room for
this one, it will return an error
but *idxattr will be == 0 from the previous attribute thus the whole dump will
be restarted (this is in case someone
requests this attribute with some of the others of course).
Cheers,
Nik
> nlmsg_end(skb, nlh);
>
> return 0;
> @@ -3712,6 +3798,9 @@ static size_t if_nlmsg_stats_size(const struct
> net_device *dev,
> }
> }
>
> + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS, 0))
> + size += rtnl_get_offload_stats_size(dev);
> +
> return size;
> }
>
> --
> 2.5.5
>
Re: [patch net-next v8 2/3] net: core: Add offload stats to if_stats_msg
> On Sep 9, 2016, at 4:36 PM, Nikolay Aleksandrov
> wrote:
>
>>
>> On Sep 8, 2016, at 9:19 AM, Jiri Pirko wrote:
>>
>> From: Nogah Frankel
>>
>> Add a nested attribute of offload stats to if_stats_msg
>> named IFLA_STATS_LINK_OFFLOAD_XSTATS.
>> Under it, add SW stats, meaning stats only per packets that went via
>> slowpath to the cpu, named IFLA_OFFLOAD_XSTATS_CPU_HIT.
>>
>> Signed-off-by: Nogah Frankel
>> Signed-off-by: Jiri Pirko
>> ---
>> include/uapi/linux/if_link.h | 9
>> net/core/rtnetlink.c | 97
>> ++--
>> 2 files changed, 102 insertions(+), 4 deletions(-)
>>
> [snip]
>> @@ -3655,6 +3725,22 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
>> struct net_device *dev,
>> }
>> }
>>
>> +if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS,
>> + *idxattr)) {
>> +attr = nla_nest_start(skb, IFLA_STATS_LINK_OFFLOAD_XSTATS);
>> +if (!attr)
>> +goto nla_put_failure;
>> +
>> +err = rtnl_get_offload_stats(skb, dev);
>> +if (err == -ENODATA)
>> +nla_nest_cancel(skb, attr);
>> +else
>> +nla_nest_end(skb, attr);
>> +
>> +if ((err) && (err != -ENODATA))
>> +goto nla_put_failure;
>> +}
>> +
>
> Hmm, actually on a second read I think there’s a potential problem here.
> Since you don’t set *idxattr and if the space
> isn’t enough the dump will get restarted and this will lead to an infinite
> loop.
Err, poor choice of words. I meant the loop will be for userspace since the
dump will err out at the same spot all the time so it
won’t be able to ever finish. :-)
> I.e. if the previous attributes filled the skb and there’s not enough room
> for this one, it will return an error
> but *idxattr will be == 0 from the previous attribute thus the whole dump
> will be restarted (this is in case someone
> requests this attribute with some of the others of course).
>
> Cheers,
> Nik
>
>
>> nlmsg_end(skb, nlh);
>>
>> return 0;
>> @@ -3712,6 +3798,9 @@ static size_t if_nlmsg_stats_size(const struct
>> net_device *dev,
>> }
>> }
>>
>> +if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS, 0))
>> +size += rtnl_get_offload_stats_size(dev);
>> +
>> return size;
>> }
>>
>> --
>> 2.5.5
Re: [patch net-next v8 2/3] net: core: Add offload stats to if_stats_msg
> On Sep 8, 2016, at 9:19 AM, Jiri Pirko wrote:
>
> From: Nogah Frankel
>
> Add a nested attribute of offload stats to if_stats_msg
> named IFLA_STATS_LINK_OFFLOAD_XSTATS.
> Under it, add SW stats, meaning stats only per packets that went via
> slowpath to the cpu, named IFLA_OFFLOAD_XSTATS_CPU_HIT.
>
> Signed-off-by: Nogah Frankel
> Signed-off-by: Jiri Pirko
> ---
> include/uapi/linux/if_link.h | 9
> net/core/rtnetlink.c | 97 ++--
> 2 files changed, 102 insertions(+), 4 deletions(-)
>
[snip]
>
> + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS,
> + *idxattr)) {
> + attr = nla_nest_start(skb, IFLA_STATS_LINK_OFFLOAD_XSTATS);
> + if (!attr)
> + goto nla_put_failure;
> +
> + err = rtnl_get_offload_stats(skb, dev);
> + if (err == -ENODATA)
> + nla_nest_cancel(skb, attr);
> + else
> + nla_nest_end(skb, attr);
> +
> + if ((err) && (err != -ENODATA))
> + goto nla_put_failure;
Hi,
Sorry I’m a little late to the party, one minor nit though - could you please
drop the extra braces here.
Overall the set looks good to me and you can add my
Reviewed-by: Nikolay Aleksandrov
Thanks,
Nik
> + }
> +
> nlmsg_end(skb, nlh);
>
> return 0;
> @@ -3712,6 +3798,9 @@ static size_t if_nlmsg_stats_size(const struct
> net_device *dev,
> }
> }
>
> + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS, 0))
> + size += rtnl_get_offload_stats_size(dev);
> +
> return size;
> }
>
> --
> 2.5.5
>
[patch net-next v8 2/3] net: core: Add offload stats to if_stats_msg
From: Nogah Frankel
Add a nested attribute of offload stats to if_stats_msg
named IFLA_STATS_LINK_OFFLOAD_XSTATS.
Under it, add SW stats, meaning stats only per packets that went via
slowpath to the cpu, named IFLA_OFFLOAD_XSTATS_CPU_HIT.
Signed-off-by: Nogah Frankel
Signed-off-by: Jiri Pirko
---
include/uapi/linux/if_link.h | 9
net/core/rtnetlink.c | 97 ++--
2 files changed, 102 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 9bf3aec..2351776 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -826,6 +826,7 @@ enum {
IFLA_STATS_LINK_64,
IFLA_STATS_LINK_XSTATS,
IFLA_STATS_LINK_XSTATS_SLAVE,
+ IFLA_STATS_LINK_OFFLOAD_XSTATS,
__IFLA_STATS_MAX,
};
@@ -845,6 +846,14 @@ enum {
};
#define LINK_XSTATS_TYPE_MAX (__LINK_XSTATS_TYPE_MAX - 1)
+/* These are stats embedded into IFLA_STATS_LINK_OFFLOAD_XSTATS */
+enum {
+ IFLA_OFFLOAD_XSTATS_UNSPEC,
+ IFLA_OFFLOAD_XSTATS_CPU_HIT, /* struct rtnl_link_stats64 */
+ __IFLA_OFFLOAD_XSTATS_MAX
+};
+#define IFLA_OFFLOAD_XSTATS_MAX (__IFLA_OFFLOAD_XSTATS_MAX - 1)
+
/* XDP section */
enum {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1dfca1c..fe3c1ba 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3577,6 +3577,79 @@ static bool stats_attr_valid(unsigned int mask, int
attrid, int idxattr)
(!idxattr || idxattr == attrid);
}
+#define IFLA_OFFLOAD_XSTATS_FIRST (IFLA_OFFLOAD_XSTATS_UNSPEC + 1)
+static int rtnl_get_offload_stats_attr_size(int attr_id)
+{
+ switch (attr_id) {
+ case IFLA_OFFLOAD_XSTATS_CPU_HIT:
+ return sizeof(struct rtnl_link_stats64);
+ }
+
+ return 0;
+}
+
+static int rtnl_get_offload_stats(struct sk_buff *skb, struct net_device *dev)
+{
+ struct nlattr *attr = NULL;
+ int attr_id, size;
+ void *attr_data;
+ int err;
+
+ if (!(dev->netdev_ops && dev->netdev_ops->ndo_has_offload_stats &&
+ dev->netdev_ops->ndo_get_offload_stats))
+ return -ENODATA;
+
+ for (attr_id = IFLA_OFFLOAD_XSTATS_FIRST;
+attr_id <= IFLA_OFFLOAD_XSTATS_MAX; attr_id++) {
+ size = rtnl_get_offload_stats_attr_size(attr_id);
+ if (!size)
+ continue;
+
+ if (!dev->netdev_ops->ndo_has_offload_stats(attr_id))
+ continue;
+
+ attr = nla_reserve_64bit(skb, attr_id, size,
+IFLA_OFFLOAD_XSTATS_UNSPEC);
+ if (!attr)
+ return -EMSGSIZE;
+
+ attr_data = nla_data(attr);
+ memset(attr_data, 0, size);
+ err = dev->netdev_ops->ndo_get_offload_stats(attr_id, dev,
+attr_data);
+ if (err)
+ return err;
+ }
+
+ if (!attr)
+ return -ENODATA;
+ return 0;
+}
+
+static int rtnl_get_offload_stats_size(const struct net_device *dev)
+{
+ int nla_size = 0;
+ int attr_id;
+ int size;
+
+ if (!(dev->netdev_ops && dev->netdev_ops->ndo_has_offload_stats &&
+ dev->netdev_ops->ndo_get_offload_stats))
+ return 0;
+
+ for (attr_id = IFLA_OFFLOAD_XSTATS_FIRST;
+attr_id <= IFLA_OFFLOAD_XSTATS_MAX; attr_id++) {
+ if (!dev->netdev_ops->ndo_has_offload_stats(attr_id))
+ continue;
+ size = rtnl_get_offload_stats_attr_size(attr_id);
+ nla_size += nla_total_size_64bit(size);
+ }
+
+ if (nla_size != 0)
+ nla_size += nla_total_size(0);
+
+ return nla_size;
+}
+
static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
int type, u32 pid, u32 seq, u32 change,
unsigned int flags, unsigned int filter_mask,
@@ -3586,6 +3659,7 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
struct net_device *dev,
struct nlmsghdr *nlh;
struct nlattr *attr;
int s_prividx = *prividx;
+ int err;
ASSERT_RTNL();
@@ -3614,8 +3688,6 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
struct net_device *dev,
const struct rtnl_link_ops *ops = dev->rtnl_link_ops;
if (ops && ops->fill_linkxstats) {
- int err;
-
*idxattr = IFLA_STATS_LINK_XSTATS;
attr = nla_nest_start(skb,
IFLA_STATS_LINK_XSTATS);
@@ -3639,8 +3711,6 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
struct net_device *dev,
if (master)
ops = master->rtnl_link_ops;
if (ops && ops->fill_linkxst
