[RFC PATCH net-next 0/2] devlink: Add port stats

[RFC PATCH net-next 0/2] devlink: Add port stats

[Michael Guralnik]

This patch set adds port statistics to the devlink port object.
It allows device drivers to dynamically attach and detach counters from a
devlink port object.

The approach of adding object-attached statistics is already supported for trap
with traffic statistics and for the dev object with reload statistics.
For the port object, this will allow the device driver to expose and dynamicly
control a set of metrics related to the port.
Currently we add support only for counters, but later API extensions can be made
to support histograms or configurable counters.

The statistics are exposed to the user with the port get command.

Example:
# devlink -s port show
pci/0000:00:0b.0/65535: type eth netdev eth1 flavour physical port 0 splittable false
  stats:
    counter1 235
    counter2 18

Michael Guralnik (2):
  devlink: Introduce stats to the port object
  devlink: Add statistics to port get

 include/net/devlink.h        |  28 +++++++++
 include/uapi/linux/devlink.h |   4 ++
 net/core/devlink.c           | 119 +++++++++++++++++++++++++++++++++++
 3 files changed, 151 insertions(+)

-- 
2.17.2

[RFC PATCH net-next 1/2] devlink: Introduce stats to the port object

[Michael Guralnik]

Introducing a mechanism for drivers to register statistics counters to
devlink ports.
Drivers to supply only name and a get method for the counter.

Signed-off-by: Michael Guralnik <michaelgur@nvidia.com>
---
 include/net/devlink.h | 28 +++++++++++++++
 net/core/devlink.c    | 80 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8d5349d2fb68..e9c3996ed359 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -115,6 +115,11 @@ struct devlink_rate {
 	};
 };
 
+struct devlink_port_stats {
+	struct list_head stats_list;
+	int (*get)(struct devlink_port *port);
+};
+
 struct devlink_port {
 	struct list_head list;
 	struct list_head param_list;
@@ -135,6 +140,7 @@ struct devlink_port {
 	struct mutex reporters_lock; /* Protects reporter_list */
 
 	struct devlink_rate *devlink_rate;
+	struct devlink_port_stats stats;
 };
 
 struct devlink_port_new_attrs {
@@ -387,6 +393,16 @@ struct devlink_param_gset_ctx {
 	enum devlink_param_cmode cmode;
 };
 
+struct devlink_port_stat {
+	char *name;
+	u64 val;
+};
+
+struct devlink_port_stat_item {
+	struct list_head list;
+	const struct devlink_port_stat *stat;
+};
+
 /**
  * struct devlink_flash_notify - devlink dev flash notify data
  * @status_msg: current status string
@@ -1712,6 +1728,18 @@ devlink_trap_policers_unregister(struct devlink *devlink,
 				 const struct devlink_trap_policer *policers,
 				 size_t policers_count);
 
+void devlink_port_stat_unregister(struct devlink_port *port,
+				  const struct devlink_port_stat *stat);
+
+int devlink_port_stats_register(struct devlink_port *port,
+				const struct devlink_port_stat *stats,
+				size_t stats_count,
+				int (*get)(struct devlink_port *port));
+
+void devlink_port_stats_unregister(struct devlink_port *port,
+				   const struct devlink_port_stat *stats,
+				   size_t stats_count);
+
 #if IS_ENABLED(CONFIG_NET_DEVLINK)
 
 struct devlink *__must_check devlink_try_get(struct devlink *devlink);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index fcd9f6d85cf1..9636d7998630 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4740,6 +4740,85 @@ static void devlink_param_notify(struct devlink *devlink,
 				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
 }
 
+struct devlink_port_stat_item *
+devlink_port_stat_find_by_name(struct list_head *stats_list, const char *stat_name)
+{
+	struct devlink_port_stat_item *stat_item;
+
+	list_for_each_entry(stat_item, stats_list, list)
+		if (!strcmp(stat_item->stat->name, stat_name))
+			return stat_item;
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(devlink_port_stat_find_by_name);
+
+void devlink_port_stat_unregister(struct devlink_port *port,
+			          const struct devlink_port_stat *stat)
+{
+	struct devlink_port_stat_item *stat_item;
+
+	stat_item =
+		devlink_port_stat_find_by_name(&port->stats.stats_list, stat->name);
+	WARN_ON(!stat_item);
+	list_del(&stat_item->list);
+	kfree(stat_item);
+}
+
+static int devlink_port_stat_register(struct devlink_port *port,
+				      const struct devlink_port_stat *stats)
+{
+	struct devlink_port_stat_item *stat_item;
+
+	WARN_ON(devlink_port_stat_find_by_name(&port->stats.stats_list, stats->name));
+
+	stat_item = kzalloc(sizeof(*stat_item), GFP_KERNEL);
+	if (!stat_item)
+		return -ENOMEM;
+
+	stat_item->stat = stats;
+
+	list_add_tail(&stat_item->list, &port->stats.stats_list);
+	return 0;
+}
+
+void devlink_port_stats_unregister(struct devlink_port *port,
+				   const struct devlink_port_stat *stats,
+				   size_t stats_count)
+{
+	const struct devlink_port_stat *stat = stats;
+	int i;
+
+	for (i = 0; i < stats_count; i++, stat++)
+		devlink_port_stat_unregister(port, stat);
+}
+EXPORT_SYMBOL_GPL(devlink_port_stats_unregister);
+
+int devlink_port_stats_register(struct devlink_port *port,
+				const struct devlink_port_stat *stats,
+				size_t stats_count,
+				int (*get)(struct devlink_port *port))
+{
+	const struct devlink_port_stat *stat = stats;
+	int i, err;
+
+	port->stats.get = get;
+	for (i = 0; i < stats_count; i++, stat++) {
+		err = devlink_port_stat_register(port, stat);
+		if (err)
+			goto rollback;
+	}
+	return 0;
+
+rollback:
+	if (!i)
+		return err;
+
+	for (stat--; i > 0; i--, stat--)
+		devlink_port_stat_unregister(port, stat);
+	return err;
+}
+EXPORT_SYMBOL_GPL(devlink_port_stats_register);
+
 static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
 					   struct netlink_callback *cb)
 {
@@ -9281,6 +9360,7 @@ int devlink_port_register(struct devlink *devlink,
 	list_add_tail(&devlink_port->list, &devlink->port_list);
 	INIT_LIST_HEAD(&devlink_port->param_list);
 	INIT_LIST_HEAD(&devlink_port->region_list);
+	INIT_LIST_HEAD(&devlink_port->stats.stats_list);
 	mutex_unlock(&devlink->lock);
 	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
 	devlink_port_type_warn_schedule(devlink_port);
-- 
2.17.2

[RFC PATCH net-next 2/2] devlink: Add statistics to port get

[Michael Guralnik]

Add a list of all registered stats and their value to the port get
command.

Signed-off-by: Michael Guralnik <michaelgur@nvidia.com>
---
 include/uapi/linux/devlink.h |  4 ++++
 net/core/devlink.c           | 39 ++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index b897b80770f6..8da01e551695 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -553,6 +553,10 @@ enum devlink_attr {
 
 	DEVLINK_ATTR_REGION_MAX_SNAPSHOTS,	/* u32 */
 
+	DEVLINK_ATTR_STATS_ENTRY,		/* nested */
+	DEVLINK_ATTR_STATS_NAME,		/* string */
+	DEVLINK_ATTR_STATS_VALUE,		/* u64 */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 9636d7998630..b735cca727a7 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1080,6 +1080,41 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
 	return err;
 }
 
+static int devlink_port_stats_get(struct devlink_port *port)
+{
+	if (!port->stats.get)
+		return -EOPNOTSUPP;
+	return port->stats.get(port);
+}
+
+static int devlink_nl_port_stats_put(struct sk_buff *msg, struct devlink_port *port)
+{
+	struct nlattr *stats_attr, *stats_entry_attr;
+	struct devlink_port_stat_item *stats_item;
+	int err;
+
+	stats_attr = nla_nest_start(msg, DEVLINK_ATTR_STATS);
+	if (!stats_attr)
+		return -EMSGSIZE;
+
+	err = devlink_port_stats_get(port);
+	if (err)
+		return err;
+
+	list_for_each_entry(stats_item, &port->stats.stats_list, list) {
+		stats_entry_attr = nla_nest_start(msg, DEVLINK_ATTR_STATS_ENTRY);
+		if (!stats_entry_attr)
+			return -EMSGSIZE;
+
+		nla_put_string(msg, DEVLINK_ATTR_STATS_NAME, stats_item->stat->name);
+		nla_put_u32(msg, DEVLINK_ATTR_STATS_VALUE, stats_item->stat->val);
+
+		nla_nest_end(msg, stats_entry_attr);
+	}
+	nla_nest_end(msg, stats_attr);
+	return 0;
+}
+
 static int devlink_nl_port_fill(struct sk_buff *msg,
 				struct devlink_port *devlink_port,
 				enum devlink_command cmd, u32 portid, u32 seq,
@@ -1132,6 +1167,8 @@ static int devlink_nl_port_fill(struct sk_buff *msg,
 	if (devlink_nl_port_function_attrs_put(msg, devlink_port, extack))
 		goto nla_put_failure;
 
+	if (devlink_nl_port_stats_put(msg, devlink_port))
+		goto nla_put_failure;
 	genlmsg_end(msg, hdr);
 	return 0;
 
@@ -8670,6 +8707,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_RATE_TX_MAX] = { .type = NLA_U64 },
 	[DEVLINK_ATTR_RATE_NODE_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_STATS_NAME] = {.type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_STATS_VALUE] = {.type = NLA_U64 },
 };
 
 static const struct genl_small_ops devlink_nl_ops[] = {
-- 
2.17.2

Re: [RFC PATCH net-next 0/2] devlink: Add port stats

[Jakub Kicinski]

On Thu, 7 Apr 2022 11:40:48 +0300 Michael Guralnik wrote:
> This patch set adds port statistics to the devlink port object.
> It allows device drivers to dynamically attach and detach counters from a
> devlink port object.

The challenge in defining APIs for stats is not in how to wrap a free
form string in a netlink message but how do define values that have
clear semantics and are of value to the user.

Start from that, discuss what you have with the customer who requested
the feature. Then think about the API.

I have said this multiple times to multiple people on your team.

> The approach of adding object-attached statistics is already supported for trap
> with traffic statistics and for the dev object with reload statistics.

That's an entirely false comparison.

> For the port object, this will allow the device driver to expose and dynamicly
> control a set of metrics related to the port.
> Currently we add support only for counters, but later API extensions can be made
> to support histograms or configurable counters.
> 
> The statistics are exposed to the user with the port get command.
> 
> Example:
> # devlink -s port show
> pci/0000:00:0b.0/65535: type eth netdev eth1 flavour physical port 0 splittable false
>   stats:
>     counter1 235
>     counter2 18

Re: [RFC PATCH net-next 0/2] devlink: Add port stats

[Jiri Pirko]

Fri, Apr 08, 2022 at 05:16:38AM CEST, kuba@kernel.org wrote:
>On Thu, 7 Apr 2022 11:40:48 +0300 Michael Guralnik wrote:
>> This patch set adds port statistics to the devlink port object.
>> It allows device drivers to dynamically attach and detach counters from a
>> devlink port object.
>
>The challenge in defining APIs for stats is not in how to wrap a free
>form string in a netlink message but how do define values that have
>clear semantics and are of value to the user.
>
>Start from that, discuss what you have with the customer who requested
>the feature. Then think about the API.
>
>I have said this multiple times to multiple people on your team.
>
>> The approach of adding object-attached statistics is already supported for trap
>> with traffic statistics and for the dev object with reload statistics.
>
>That's an entirely false comparison.

The trap stats are there already, why do you thing it is a "false
comparison" to that?

They use DEVLINK_ATTR_STATS attribute to carry the stats nest and then:
        DEVLINK_ATTR_STATS_RX_PACKETS,          /* u64 */
        DEVLINK_ATTR_STATS_RX_BYTES,            /* u64 */
        DEVLINK_ATTR_STATS_RX_DROPPED,          /* u64 */
I think that the semantics of these are quite clear.



>
>> For the port object, this will allow the device driver to expose and dynamicly
>> control a set of metrics related to the port.
>> Currently we add support only for counters, but later API extensions can be made
>> to support histograms or configurable counters.
>> 
>> The statistics are exposed to the user with the port get command.
>> 
>> Example:
>> # devlink -s port show
>> pci/0000:00:0b.0/65535: type eth netdev eth1 flavour physical port 0 splittable false
>>   stats:
>>     counter1 235
>>     counter2 18

Re: [RFC PATCH net-next 0/2] devlink: Add port stats

[Jakub Kicinski]

On Mon, 11 Apr 2022 12:28:40 +0200 Jiri Pirko wrote:
> >> The approach of adding object-attached statistics is already supported for trap
> >> with traffic statistics and for the dev object with reload statistics.  
> >
> >That's an entirely false comparison.  
> 
> The trap stats are there already, why do you thing it is a "false
> comparison" to that?
> 
> They use DEVLINK_ATTR_STATS attribute to carry the stats nest and then:
>         DEVLINK_ATTR_STATS_RX_PACKETS,          /* u64 */
>         DEVLINK_ATTR_STATS_RX_BYTES,            /* u64 */
>         DEVLINK_ATTR_STATS_RX_DROPPED,          /* u64 */
> I think that the semantics of these are quite clear.

Exactly, (1) the semantics of statistics on traps and health are clearly
dictated by the object, there is no ambiguity what rx packets for a trap
means; (2) the statistics are enumerated by the core...

> >> For the port object, this will allow the device driver to expose and dynamicly
> >> control a set of metrics related to the port.

..this is like saying "it was 14C in Prague and San Francisco today, so
the weather was the same. It was sunny in Prague and raining in SF".

Re: [RFC PATCH net-next 0/2] devlink: Add port stats

[Jiri Pirko]

Fri, Apr 08, 2022 at 05:16:38AM CEST, kuba@kernel.org wrote:
>On Thu, 7 Apr 2022 11:40:48 +0300 Michael Guralnik wrote:
>> This patch set adds port statistics to the devlink port object.
>> It allows device drivers to dynamically attach and detach counters from a
>> devlink port object.
>
>The challenge in defining APIs for stats is not in how to wrap a free
>form string in a netlink message but how do define values that have
>clear semantics and are of value to the user.

Wait, does all stats have to be well-defined? I mean, look at the
ethtool stats. They are free-form strings too. Do you mean that in
devlink, we can only have well-defines enum-based stats?


>
>Start from that, discuss what you have with the customer who requested
>the feature. Then think about the API.
>
>I have said this multiple times to multiple people on your team.
>
>> The approach of adding object-attached statistics is already supported for trap
>> with traffic statistics and for the dev object with reload statistics.
>
>That's an entirely false comparison.
>
>> For the port object, this will allow the device driver to expose and dynamicly
>> control a set of metrics related to the port.
>> Currently we add support only for counters, but later API extensions can be made
>> to support histograms or configurable counters.
>> 
>> The statistics are exposed to the user with the port get command.
>> 
>> Example:
>> # devlink -s port show
>> pci/0000:00:0b.0/65535: type eth netdev eth1 flavour physical port 0 splittable false
>>   stats:
>>     counter1 235
>>     counter2 18

Re: [RFC PATCH net-next 0/2] devlink: Add port stats

[Jakub Kicinski]

On Mon, 11 Apr 2022 12:31:53 +0200 Jiri Pirko wrote:
> Fri, Apr 08, 2022 at 05:16:38AM CEST, kuba@kernel.org wrote:
> >On Thu, 7 Apr 2022 11:40:48 +0300 Michael Guralnik wrote:  
> >> This patch set adds port statistics to the devlink port object.
> >> It allows device drivers to dynamically attach and detach counters from a
> >> devlink port object.  
> >
> >The challenge in defining APIs for stats is not in how to wrap a free
> >form string in a netlink message but how do define values that have
> >clear semantics and are of value to the user.  
> 
> Wait, does all stats have to be well-defined? I mean, look at the
> ethtool stats. They are free-form strings too. Do you mean that in
> devlink, we can only have well-defines enum-based stats?

That's my strong preference, yes.

First, and obvious argument is that it make lazy coding less likely
(see devlink params).

More importantly, tho, if your stats are not well defined - users don't
need to seem them. Really! If I can't draw a line between a statistic
and device behavior then keep that stat in the register dump, debugfs 
or /dev/null.

That's why it's important that we talk about _what_ you're trying to
expose.

Re: [RFC PATCH net-next 0/2] devlink: Add port stats

[Jiri Pirko]

Mon, Apr 11, 2022 at 08:01:57PM CEST, kuba@kernel.org wrote:
>On Mon, 11 Apr 2022 12:31:53 +0200 Jiri Pirko wrote:
>> Fri, Apr 08, 2022 at 05:16:38AM CEST, kuba@kernel.org wrote:
>> >On Thu, 7 Apr 2022 11:40:48 +0300 Michael Guralnik wrote:  
>> >> This patch set adds port statistics to the devlink port object.
>> >> It allows device drivers to dynamically attach and detach counters from a
>> >> devlink port object.  
>> >
>> >The challenge in defining APIs for stats is not in how to wrap a free
>> >form string in a netlink message but how do define values that have
>> >clear semantics and are of value to the user.  
>> 
>> Wait, does all stats have to be well-defined? I mean, look at the
>> ethtool stats. They are free-form strings too. Do you mean that in
>> devlink, we can only have well-defines enum-based stats?
>
>That's my strong preference, yes.
>
>First, and obvious argument is that it make lazy coding less likely
>(see devlink params).
>
>More importantly, tho, if your stats are not well defined - users don't
>need to seem them. Really! If I can't draw a line between a statistic
>and device behavior then keep that stat in the register dump, debugfs 

During the DaveM's-only era, there was quite strict policy against any
debugfs usage. As far as I remember the claim was, find of define the
proper api or do your debug things out-of-tree.

Does that changed? I just want to make sure that we are now free to use
debugfs for exposuse of debugging info as "odd vendor stats".
Personally, I think it is good idea. I think that the rest of the kernel
actually uses debugfs like that.

Thanks!

>or /dev/null.
>
>That's why it's important that we talk about _what_ you're trying to
>expose.

Basically a mixture of quite generic things and very obscure device
specific items.

Re: [RFC PATCH net-next 0/2] devlink: Add port stats

[Jakub Kicinski]

On Tue, 12 Apr 2022 10:16:26 +0200 Jiri Pirko wrote:
> Mon, Apr 11, 2022 at 08:01:57PM CEST, kuba@kernel.org wrote:
> >> Wait, does all stats have to be well-defined? I mean, look at the
> >> ethtool stats. They are free-form strings too. Do you mean that in
> >> devlink, we can only have well-defines enum-based stats?  
> >
> >That's my strong preference, yes.
> >
> >First, and obvious argument is that it make lazy coding less likely
> >(see devlink params).
> >
> >More importantly, tho, if your stats are not well defined - users don't
> >need to seem them. Really! If I can't draw a line between a statistic
> >and device behavior then keep that stat in the register dump, debugfs   
> 
> During the DaveM's-only era, there was quite strict policy against any
> debugfs usage. As far as I remember the claim was, find of define the
> proper api or do your debug things out-of-tree.
> 
> Does that changed? I just want to make sure that we are now free to use
> debugfs for exposuse of debugging info as "odd vendor stats".
> Personally, I think it is good idea. I think that the rest of the kernel
> actually uses debugfs like that.

I think the policy is "it's fine as long as it's read-only".
Which could be a problem if you want to parametrize the counters.
But then again based on this RFC IDK if you do.

> >or /dev/null.
> >
> >That's why it's important that we talk about _what_ you're trying to
> >expose.  
> 
> Basically a mixture of quite generic things and very obscure device
> specific items.