From: Roopa Prabhu <roopa@cumulusnetworks•com>
To: Jiri Pirko <jiri@resnulli•us>
Cc: netdev@vger•kernel.org, davem@davemloft•net, nogahf@mellanox•com,
idosch@mellanox•com, eladr@mellanox•com, yotamg@mellanox•com,
ogerlitz@mellanox•com, nikolay@cumulusnetworks•com,
linville@tuxdriver•com, tgraf@suug•ch, gospo@cumulusnetworks•com,
sfeldma@gmail•com, sd@queasysnail•net, eranbe@mellanox•com,
ast@plumgrid•com, edumazet@google•com,
hannes@stressinduktion•org, f.fainelli@gmail•com,
dsa@cumulusnetworks•com
Subject: Re: [patch net-next v7 2/3] net: core: Add offload stats to if_stats_msg
Date: Tue, 06 Sep 2016 08:20:12 -0700 [thread overview]
Message-ID: <57CEDEAC.1040806@cumulusnetworks.com> (raw)
In-Reply-To: <1473095937-27100-3-git-send-email-jiri@resnulli.us>
On 9/5/16, 10:18 AM, Jiri Pirko wrote:
> From: Nogah Frankel <nogahf@mellanox•com>
>
> 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 <nogahf@mellanox•com>
> Signed-off-by: Jiri Pirko <jiri@mellanox•com>
> ---
> include/uapi/linux/if_link.h | 10 +++++
> net/core/rtnetlink.c | 88 ++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 9bf3aec..4aaa2a1 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,15 @@ 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)
> +#define IFLA_OFFLOAD_XSTATS_FIRST (IFLA_OFFLOAD_XSTATS_UNSPEC + 1)
not sure why we need a first in the uapi.
> +
> /* XDP section */
>
> enum {
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 1dfca1c..95eb131 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3577,6 +3577,74 @@ static bool stats_attr_valid(unsigned int mask, int attrid, int idxattr)
> (!idxattr || idxattr == attrid);
> }
>
> +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;
> + 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 0;
> +
> + 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;
you need this 64bit alignment padding only for the specific nested attribute
your driver is adding ?. The code seems to add it unconditionally for every attribute.
But, I am guessing future attribute authors will need to adjust this somehow.
> +
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +static int rtnl_get_offload_stats_size(const struct net_device *dev)
> +{
> + int size = 0;
> + int attr_id;
> +
> + if (!(dev->netdev_ops && dev->netdev_ops->ndo_has_offload_stats &&
> + dev->netdev_ops->ndo_get_offload_stats))
> + return nla_total_size(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);
> + }
> +
> + size += nla_total_size(0);
> +
> + return 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 +3654,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 +3683,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 +3706,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_linkxstats) {
> - int err;
> -
> *idxattr = IFLA_STATS_LINK_XSTATS_SLAVE;
> attr = nla_nest_start(skb,
> IFLA_STATS_LINK_XSTATS_SLAVE);
> @@ -3655,6 +3720,18 @@ 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);
> + nla_nest_end(skb, attr);
seems like this can result in an empty nested attribute. better thing would be to
cancel the nest if size from rtnl_get_offload_stats is zero ?
> + if (err)
> + goto nla_put_failure;
> + }
> +
> nlmsg_end(skb, nlh);
>
> return 0;
> @@ -3712,6 +3789,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;
> }
>
next prev parent reply other threads:[~2016-09-06 15:20 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-05 17:18 [patch net-next v7 0/3] return offloaded stats as default and expose original sw stats Jiri Pirko
2016-09-05 17:18 ` [patch net-next v7 1/3] netdevice: Add offload statistics ndo Jiri Pirko
2016-09-06 15:08 ` Roopa Prabhu
2016-09-06 15:46 ` Jiri Pirko
2016-09-05 17:18 ` [patch net-next v7 2/3] net: core: Add offload stats to if_stats_msg Jiri Pirko
2016-09-06 15:20 ` Roopa Prabhu [this message]
2016-09-07 14:31 ` Nogah Frankel
2016-09-05 17:18 ` [patch net-next v7 3/3] mlxsw: spectrum: Implement offload stats ndo and expose HW stats by default Jiri Pirko
2016-09-06 14:59 ` [patch net-next v7 0/3] return offloaded stats as default and expose original sw stats Roopa Prabhu
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=57CEDEAC.1040806@cumulusnetworks.com \
--to=roopa@cumulusnetworks$(echo .)com \
--cc=ast@plumgrid$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dsa@cumulusnetworks$(echo .)com \
--cc=edumazet@google$(echo .)com \
--cc=eladr@mellanox$(echo .)com \
--cc=eranbe@mellanox$(echo .)com \
--cc=f.fainelli@gmail$(echo .)com \
--cc=gospo@cumulusnetworks$(echo .)com \
--cc=hannes@stressinduktion$(echo .)org \
--cc=idosch@mellanox$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=linville@tuxdriver$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nikolay@cumulusnetworks$(echo .)com \
--cc=nogahf@mellanox$(echo .)com \
--cc=ogerlitz@mellanox$(echo .)com \
--cc=sd@queasysnail$(echo .)net \
--cc=sfeldma@gmail$(echo .)com \
--cc=tgraf@suug$(echo .)ch \
--cc=yotamg@mellanox$(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