[PATCH net-next v3 0/3] devlink: kernel region snapshot id allocation

[PATCH net-next v3 0/3] devlink: kernel region snapshot id allocation

[Jakub Kicinski]

Hi!

currently users have to find a free snapshot id to pass
to the kernel when they are requesting a snapshot to be
taken.

This set extends the kernel so it can allocate the id
on its own and send it back to user space in a response.

Jakub Kicinski (3):
  devlink: factor out building a snapshot notification
  devlink: let kernel allocate region snapshot id
  docs: devlink: clarify the scope of snapshot id

 .../networking/devlink/devlink-region.rst     |  11 +-
 net/core/devlink.c                            | 108 ++++++++++++++----
 .../drivers/net/netdevsim/devlink.sh          |  13 +++
 3 files changed, 106 insertions(+), 26 deletions(-)

-- 
2.25.4

[PATCH net-next v3 1/3] devlink: factor out building a snapshot notification

[Jakub Kicinski]

We'll need to send snapshot info back on the socket
which requested a snapshot to be created. Factor out
constructing a snapshot description from the broadcast
notification code.

v3: new patch

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/devlink.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 9f0af8931a9c..92afb85bad89 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3716,24 +3716,26 @@ static int devlink_nl_region_fill(struct sk_buff *msg, struct devlink *devlink,
 	return err;
 }
 
-static void devlink_nl_region_notify(struct devlink_region *region,
-				     struct devlink_snapshot *snapshot,
-				     enum devlink_command cmd)
+static struct sk_buff *
+devlink_nl_region_notify_build(struct devlink_region *region,
+			       struct devlink_snapshot *snapshot,
+			       enum devlink_command cmd, u32 portid, u32 seq)
 {
 	struct devlink *devlink = region->devlink;
 	struct sk_buff *msg;
 	void *hdr;
 	int err;
 
-	WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL);
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
-		return;
+		return ERR_PTR(-ENOMEM);
 
-	hdr = genlmsg_put(msg, 0, 0, &devlink_nl_family, 0, cmd);
-	if (!hdr)
+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, 0, cmd);
+	if (!hdr) {
+		err = -EMSGSIZE;
 		goto out_free_msg;
+	}
 
 	err = devlink_nl_put_handle(msg, devlink);
 	if (err)
@@ -3757,15 +3759,30 @@ static void devlink_nl_region_notify(struct devlink_region *region,
 	}
 	genlmsg_end(msg, hdr);
 
-	genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
-				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
-
-	return;
+	return msg;
 
 out_cancel_msg:
 	genlmsg_cancel(msg, hdr);
 out_free_msg:
 	nlmsg_free(msg);
+	return ERR_PTR(err);
+}
+
+static void devlink_nl_region_notify(struct devlink_region *region,
+				     struct devlink_snapshot *snapshot,
+				     enum devlink_command cmd)
+{
+	struct devlink *devlink = region->devlink;
+	struct sk_buff *msg;
+
+	WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL);
+
+	msg = devlink_nl_region_notify_build(region, snapshot, cmd, 0, 0);
+	if (IS_ERR(msg))
+		return;
+
+	genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
+				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
 }
 
 /**
-- 
2.25.4

[PATCH net-next v3 2/3] devlink: let kernel allocate region snapshot id

[Jakub Kicinski]

Currently users have to choose a free snapshot id before
calling DEVLINK_CMD_REGION_NEW. This is potentially racy
and inconvenient.

Make the DEVLINK_ATTR_REGION_SNAPSHOT_ID optional and try
to allocate id automatically. Send a message back to the
caller with the snapshot info.

Example use:
$ devlink region new netdevsim/netdevsim1/dummy
netdevsim/netdevsim1/dummy: snapshot 1

$ id=$(devlink -j region new netdevsim/netdevsim1/dummy | \
       jq '.[][][][]')
$ devlink region dump netdevsim/netdevsim1/dummy snapshot $id
[...]
$ devlink region del netdevsim/netdevsim1/dummy snapshot $id

v3:
 - send the notification only once snapshot creation completed.
v2:
 - don't wrap the line containing extack;
 - add a few sentences to the docs.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../networking/devlink/devlink-region.rst     |  7 +-
 net/core/devlink.c                            | 69 +++++++++++++++----
 .../drivers/net/netdevsim/devlink.sh          | 13 ++++
 3 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
index 04e04d1ff627..daf35427fce1 100644
--- a/Documentation/networking/devlink/devlink-region.rst
+++ b/Documentation/networking/devlink/devlink-region.rst
@@ -23,7 +23,9 @@ states, but see also :doc:`devlink-health`
 Regions may optionally support capturing a snapshot on demand via the
 ``DEVLINK_CMD_REGION_NEW`` netlink message. A driver wishing to allow
 requested snapshots must implement the ``.snapshot`` callback for the region
-in its ``devlink_region_ops`` structure.
+in its ``devlink_region_ops`` structure. If snapshot id is not set in
+the ``DEVLINK_CMD_REGION_NEW`` request kernel will allocate one and send
+the snapshot information to user space.
 
 example usage
 -------------
@@ -45,7 +47,8 @@ example usage
     $ devlink region del pci/0000:00:05.0/cr-space snapshot 1
 
     # Request an immediate snapshot, if supported by the region
-    $ devlink region new pci/0000:00:05.0/cr-space snapshot 5
+    $ devlink region new pci/0000:00:05.0/cr-space
+    pci/0000:00:05.0/cr-space: snapshot 5
 
     # Dump a snapshot:
     $ devlink region dump pci/0000:00:05.0/fw-health snapshot 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 92afb85bad89..4df947fb90d9 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4082,10 +4082,41 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
 	return 0;
 }
 
+static int
+devlink_nl_snapshot_id_notify(struct devlink *devlink, struct genl_info *info,
+			      struct devlink_region *region, u32 snapshot_id)
+{
+	struct devlink_snapshot *snapshot;
+	struct sk_buff *msg;
+	int err;
+
+	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
+	if (WARN_ON(!snapshot))
+		return -EINVAL;
+
+	msg = devlink_nl_region_notify_build(region, snapshot,
+					     DEVLINK_CMD_REGION_NEW,
+					     info->snd_portid, info->snd_seq);
+	err = PTR_ERR_OR_ZERO(msg);
+	if (err)
+		goto err_notify;
+
+	err = genlmsg_reply(msg, info);
+	if (err)
+		goto err_notify;
+
+	return 0;
+
+err_notify:
+	devlink_region_snapshot_del(region, snapshot);
+	return err;
+}
+
 static int
 devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
+	struct nlattr *snapshot_id_attr;
 	struct devlink_region *region;
 	const char *region_name;
 	u32 snapshot_id;
@@ -4097,11 +4128,6 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	}
 
-	if (!info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
-		NL_SET_ERR_MSG_MOD(info->extack, "No snapshot id provided");
-		return -EINVAL;
-	}
-
 	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
 	region = devlink_region_get_by_name(devlink, region_name);
 	if (!region) {
@@ -4119,16 +4145,25 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 		return -ENOSPC;
 	}
 
-	snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
+	snapshot_id_attr = info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
+	if (snapshot_id_attr) {
+		snapshot_id = nla_get_u32(snapshot_id_attr);
 
-	if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
-		NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
-		return -EEXIST;
-	}
+		if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
+			NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
+			return -EEXIST;
+		}
 
-	err = __devlink_snapshot_id_insert(devlink, snapshot_id);
-	if (err)
-		return err;
+		err = __devlink_snapshot_id_insert(devlink, snapshot_id);
+		if (err)
+			return err;
+	} else {
+		err = __devlink_region_snapshot_id_get(devlink, &snapshot_id);
+		if (err) {
+			NL_SET_ERR_MSG_MOD(info->extack, "Failed to allocate a new snapshot id");
+			return err;
+		}
+	}
 
 	err = region->ops->snapshot(devlink, info->extack, &data);
 	if (err)
@@ -4138,6 +4173,14 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 	if (err)
 		goto err_snapshot_create;
 
+	if (!snapshot_id_attr) {
+		/* destroys the snapshot on failure */
+		err = devlink_nl_snapshot_id_notify(devlink, info,
+						    region, snapshot_id);
+		if (err)
+			return err;
+	}
+
 	return 0;
 
 err_snapshot_create:
diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
index 1d15afd62941..de4b32fc4223 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
@@ -166,6 +166,19 @@ regions_test()
 
 	check_region_snapshot_count dummy post-second-delete 2
 
+	sid=$(devlink -j region new $DL_HANDLE/dummy | jq '.[][][][]')
+	check_err $? "Failed to create a new snapshot with id allocated by the kernel"
+
+	check_region_snapshot_count dummy post-first-request 3
+
+	devlink region dump $DL_HANDLE/dummy snapshot $sid >> /dev/null
+	check_err $? "Failed to dump a snapshot with id allocated by the kernel"
+
+	devlink region del $DL_HANDLE/dummy snapshot $sid
+	check_err $? "Failed to delete snapshot with id allocated by the kernel"
+
+	check_region_snapshot_count dummy post-first-request 2
+
 	log_test "regions test"
 }
 
-- 
2.25.4

[PATCH net-next v3 3/3] docs: devlink: clarify the scope of snapshot id

[Jakub Kicinski]

In past discussions Jiri explained snapshot ids are cross-region.
Explain this in the docs.

v3: new patch

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/networking/devlink/devlink-region.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
index daf35427fce1..3654c3e9658f 100644
--- a/Documentation/networking/devlink/devlink-region.rst
+++ b/Documentation/networking/devlink/devlink-region.rst
@@ -14,6 +14,10 @@ Region snapshots are collected by the driver, and can be accessed via read
 or dump commands. This allows future analysis on the created snapshots.
 Regions may optionally support triggering snapshots on demand.
 
+Snapshot identifiers are scoped to the devlink instance, not a region.
+All snapshots with the same snapshot id within a devlink instance
+correspond to the same event.
+
 The major benefit to creating a region is to provide access to internal
 address regions that are otherwise inaccessible to the user.
 
-- 
2.25.4

[PATCH iproute2-next v3] devlink: support kernel-side snapshot id allocation

[Jakub Kicinski]

Make ID argument optional and read the snapshot info
that kernel sends us.

$ devlink region new netdevsim/netdevsim1/dummy
netdevsim/netdevsim1/dummy: snapshot 0
$ devlink -jp region new netdevsim/netdevsim1/dummy
{
    "regions": {
        "netdevsim/netdevsim1/dummy": {
            "snapshot": [ 1 ]
        }
    }
}
$ devlink region show netdevsim/netdevsim1/dummy
netdevsim/netdevsim1/dummy: size 32768 snapshot [0 1]

v3: back to v1..

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 devlink/devlink.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index bd48a73bc0e4..507972c360a7 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6476,6 +6476,23 @@ static int cmd_region_read(struct dl *dl)
 	return err;
 }
 
+static int cmd_region_snapshot_new_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+	struct dl *dl = data;
+
+	mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
+	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME] ||
+	    !tb[DEVLINK_ATTR_REGION_NAME] ||
+	    !tb[DEVLINK_ATTR_REGION_SNAPSHOT_ID])
+		return MNL_CB_ERROR;
+
+	pr_out_region(dl, tb);
+
+	return MNL_CB_OK;
+}
+
 static int cmd_region_snapshot_new(struct dl *dl)
 {
 	struct nlmsghdr *nlh;
@@ -6484,12 +6501,15 @@ static int cmd_region_snapshot_new(struct dl *dl)
 	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_REGION_NEW,
 			       NLM_F_REQUEST | NLM_F_ACK);
 
-	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE_REGION |
-				DL_OPT_REGION_SNAPSHOT_ID, 0);
+	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE_REGION,
+				DL_OPT_REGION_SNAPSHOT_ID);
 	if (err)
 		return err;
 
-	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+	pr_out_section_start(dl, "regions");
+	err = _mnlg_socket_sndrcv(dl->nlg, nlh, cmd_region_snapshot_new_cb, dl);
+	pr_out_section_end(dl);
+	return err;
 }
 
 static void cmd_region_help(void)
-- 
2.25.4

Re: [PATCH net-next v3 1/3] devlink: factor out building a snapshot notification

[Jiri Pirko]

Thu, Apr 30, 2020 at 07:57:56PM CEST, kuba@kernel.org wrote:
>We'll need to send snapshot info back on the socket
>which requested a snapshot to be created. Factor out
>constructing a snapshot description from the broadcast
>notification code.
>
>v3: new patch
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next v3 2/3] devlink: let kernel allocate region snapshot id

[Jiri Pirko]

Thu, Apr 30, 2020 at 07:57:57PM CEST, kuba@kernel.org wrote:
>Currently users have to choose a free snapshot id before
>calling DEVLINK_CMD_REGION_NEW. This is potentially racy
>and inconvenient.
>
>Make the DEVLINK_ATTR_REGION_SNAPSHOT_ID optional and try
>to allocate id automatically. Send a message back to the
>caller with the snapshot info.
>
>Example use:
>$ devlink region new netdevsim/netdevsim1/dummy
>netdevsim/netdevsim1/dummy: snapshot 1
>
>$ id=$(devlink -j region new netdevsim/netdevsim1/dummy | \
>       jq '.[][][][]')
>$ devlink region dump netdevsim/netdevsim1/dummy snapshot $id
>[...]
>$ devlink region del netdevsim/netdevsim1/dummy snapshot $id
>
>v3:
> - send the notification only once snapshot creation completed.
>v2:
> - don't wrap the line containing extack;
> - add a few sentences to the docs.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> .../networking/devlink/devlink-region.rst     |  7 +-
> net/core/devlink.c                            | 69 +++++++++++++++----
> .../drivers/net/netdevsim/devlink.sh          | 13 ++++
> 3 files changed, 74 insertions(+), 15 deletions(-)
>
>diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
>index 04e04d1ff627..daf35427fce1 100644
>--- a/Documentation/networking/devlink/devlink-region.rst
>+++ b/Documentation/networking/devlink/devlink-region.rst
>@@ -23,7 +23,9 @@ states, but see also :doc:`devlink-health`
> Regions may optionally support capturing a snapshot on demand via the
> ``DEVLINK_CMD_REGION_NEW`` netlink message. A driver wishing to allow
> requested snapshots must implement the ``.snapshot`` callback for the region
>-in its ``devlink_region_ops`` structure.
>+in its ``devlink_region_ops`` structure. If snapshot id is not set in
>+the ``DEVLINK_CMD_REGION_NEW`` request kernel will allocate one and send
>+the snapshot information to user space.
> 
> example usage
> -------------
>@@ -45,7 +47,8 @@ example usage
>     $ devlink region del pci/0000:00:05.0/cr-space snapshot 1
> 
>     # Request an immediate snapshot, if supported by the region
>-    $ devlink region new pci/0000:00:05.0/cr-space snapshot 5
>+    $ devlink region new pci/0000:00:05.0/cr-space
>+    pci/0000:00:05.0/cr-space: snapshot 5
> 
>     # Dump a snapshot:
>     $ devlink region dump pci/0000:00:05.0/fw-health snapshot 1
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 92afb85bad89..4df947fb90d9 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4082,10 +4082,41 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
> 	return 0;
> }
> 
>+static int
>+devlink_nl_snapshot_id_notify(struct devlink *devlink, struct genl_info *info,
>+			      struct devlink_region *region, u32 snapshot_id)
>+{
>+	struct devlink_snapshot *snapshot;
>+	struct sk_buff *msg;
>+	int err;
>+
>+	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>+	if (WARN_ON(!snapshot))
>+		return -EINVAL;
>+
>+	msg = devlink_nl_region_notify_build(region, snapshot,
>+					     DEVLINK_CMD_REGION_NEW,
>+					     info->snd_portid, info->snd_seq);
>+	err = PTR_ERR_OR_ZERO(msg);
>+	if (err)
>+		goto err_notify;
>+
>+	err = genlmsg_reply(msg, info);
>+	if (err)
>+		goto err_notify;
>+
>+	return 0;
>+
>+err_notify:
>+	devlink_region_snapshot_del(region, snapshot);

This is odd to have "del" in "notify helper". Could you please move the
"del" call to the caller so "create" and "del" are called from the same
function? I mean, that is the usual way to do cleanup, I don't see why
to do it differently here.

Otherwise the patch looks fine. Thanks!


>+	return err;
>+}
>+
> static int
> devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
> {
> 	struct devlink *devlink = info->user_ptr[0];
>+	struct nlattr *snapshot_id_attr;
> 	struct devlink_region *region;
> 	const char *region_name;
> 	u32 snapshot_id;
>@@ -4097,11 +4128,6 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
> 		return -EINVAL;
> 	}
> 
>-	if (!info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
>-		NL_SET_ERR_MSG_MOD(info->extack, "No snapshot id provided");
>-		return -EINVAL;
>-	}
>-
> 	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
> 	region = devlink_region_get_by_name(devlink, region_name);
> 	if (!region) {
>@@ -4119,16 +4145,25 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
> 		return -ENOSPC;
> 	}
> 
>-	snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
>+	snapshot_id_attr = info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
>+	if (snapshot_id_attr) {
>+		snapshot_id = nla_get_u32(snapshot_id_attr);
> 
>-	if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
>-		NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
>-		return -EEXIST;
>-	}
>+		if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
>+			NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
>+			return -EEXIST;
>+		}
> 
>-	err = __devlink_snapshot_id_insert(devlink, snapshot_id);
>-	if (err)
>-		return err;
>+		err = __devlink_snapshot_id_insert(devlink, snapshot_id);
>+		if (err)
>+			return err;
>+	} else {
>+		err = __devlink_region_snapshot_id_get(devlink, &snapshot_id);
>+		if (err) {
>+			NL_SET_ERR_MSG_MOD(info->extack, "Failed to allocate a new snapshot id");
>+			return err;
>+		}
>+	}
> 
> 	err = region->ops->snapshot(devlink, info->extack, &data);
> 	if (err)
>@@ -4138,6 +4173,14 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
> 	if (err)
> 		goto err_snapshot_create;
> 
>+	if (!snapshot_id_attr) {
>+		/* destroys the snapshot on failure */
>+		err = devlink_nl_snapshot_id_notify(devlink, info,
>+						    region, snapshot_id);
>+		if (err)
>+			return err;
>+	}
>+
> 	return 0;
> 
> err_snapshot_create:
>diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>index 1d15afd62941..de4b32fc4223 100755
>--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>@@ -166,6 +166,19 @@ regions_test()
> 
> 	check_region_snapshot_count dummy post-second-delete 2
> 
>+	sid=$(devlink -j region new $DL_HANDLE/dummy | jq '.[][][][]')
>+	check_err $? "Failed to create a new snapshot with id allocated by the kernel"
>+
>+	check_region_snapshot_count dummy post-first-request 3
>+
>+	devlink region dump $DL_HANDLE/dummy snapshot $sid >> /dev/null
>+	check_err $? "Failed to dump a snapshot with id allocated by the kernel"
>+
>+	devlink region del $DL_HANDLE/dummy snapshot $sid
>+	check_err $? "Failed to delete snapshot with id allocated by the kernel"
>+
>+	check_region_snapshot_count dummy post-first-request 2
>+
> 	log_test "regions test"
> }
> 
>-- 
>2.25.4
>

Re: [PATCH net-next v3 3/3] docs: devlink: clarify the scope of snapshot id

[Jiri Pirko]

Thu, Apr 30, 2020 at 07:57:58PM CEST, kuba@kernel.org wrote:
>In past discussions Jiri explained snapshot ids are cross-region.
>Explain this in the docs.
>
>v3: new patch
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next v3 1/3] devlink: factor out building a snapshot notification

[Jacob Keller]

On 4/30/2020 10:57 AM, Jakub Kicinski wrote:
> We'll need to send snapshot info back on the socket
> which requested a snapshot to be created. Factor out
> constructing a snapshot description from the broadcast
> notification code.
> 
> v3: new patch
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net-next v3 2/3] devlink: let kernel allocate region snapshot id

[Jacob Keller]

On 4/30/2020 10:57 AM, Jakub Kicinski wrote:
> Currently users have to choose a free snapshot id before
> calling DEVLINK_CMD_REGION_NEW. This is potentially racy
> and inconvenient.
> 
> Make the DEVLINK_ATTR_REGION_SNAPSHOT_ID optional and try
> to allocate id automatically. Send a message back to the
> caller with the snapshot info.
> 
> Example use:
> $ devlink region new netdevsim/netdevsim1/dummy
> netdevsim/netdevsim1/dummy: snapshot 1
> 
> $ id=$(devlink -j region new netdevsim/netdevsim1/dummy | \
>        jq '.[][][][]')
> $ devlink region dump netdevsim/netdevsim1/dummy snapshot $id
> [...]
> $ devlink region del netdevsim/netdevsim1/dummy snapshot $id
> 
> v3:
>  - send the notification only once snapshot creation completed.
> v2:
>  - don't wrap the line containing extack;
>  - add a few sentences to the docs.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../networking/devlink/devlink-region.rst     |  7 +-
>  net/core/devlink.c                            | 69 +++++++++++++++----
>  .../drivers/net/netdevsim/devlink.sh          | 13 ++++
>  3 files changed, 74 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
> index 04e04d1ff627..daf35427fce1 100644
> --- a/Documentation/networking/devlink/devlink-region.rst
> +++ b/Documentation/networking/devlink/devlink-region.rst
> @@ -23,7 +23,9 @@ states, but see also :doc:`devlink-health`
>  Regions may optionally support capturing a snapshot on demand via the
>  ``DEVLINK_CMD_REGION_NEW`` netlink message. A driver wishing to allow
>  requested snapshots must implement the ``.snapshot`` callback for the region
> -in its ``devlink_region_ops`` structure.
> +in its ``devlink_region_ops`` structure. If snapshot id is not set in
> +the ``DEVLINK_CMD_REGION_NEW`` request kernel will allocate one and send
> +the snapshot information to user space.
>  
>  example usage
>  -------------
> @@ -45,7 +47,8 @@ example usage
>      $ devlink region del pci/0000:00:05.0/cr-space snapshot 1
>  
>      # Request an immediate snapshot, if supported by the region
> -    $ devlink region new pci/0000:00:05.0/cr-space snapshot 5
> +    $ devlink region new pci/0000:00:05.0/cr-space
> +    pci/0000:00:05.0/cr-space: snapshot 5
>  
>      # Dump a snapshot:
>      $ devlink region dump pci/0000:00:05.0/fw-health snapshot 1
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 92afb85bad89..4df947fb90d9 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4082,10 +4082,41 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static int
> +devlink_nl_snapshot_id_notify(struct devlink *devlink, struct genl_info *info,
> +			      struct devlink_region *region, u32 snapshot_id)
> +{
> +	struct devlink_snapshot *snapshot;
> +	struct sk_buff *msg;
> +	int err;
> +
> +	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
> +	if (WARN_ON(!snapshot))
> +		return -EINVAL;
> +
> +	msg = devlink_nl_region_notify_build(region, snapshot,
> +					     DEVLINK_CMD_REGION_NEW,
> +					     info->snd_portid, info->snd_seq);
> +	err = PTR_ERR_OR_ZERO(msg);
> +	if (err)
> +		goto err_notify;
> +
> +	err = genlmsg_reply(msg, info);
> +	if (err)
> +		goto err_notify;
> +
> +	return 0;
> +
> +err_notify:
> +	devlink_region_snapshot_del(region, snapshot);
> +	return err;
> +}
> +
>  static int
>  devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct devlink *devlink = info->user_ptr[0];
> +	struct nlattr *snapshot_id_attr;
>  	struct devlink_region *region;
>  	const char *region_name;
>  	u32 snapshot_id;
> @@ -4097,11 +4128,6 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
>  		return -EINVAL;
>  	}
>  
> -	if (!info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
> -		NL_SET_ERR_MSG_MOD(info->extack, "No snapshot id provided");
> -		return -EINVAL;
> -	}
> -
>  	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
>  	region = devlink_region_get_by_name(devlink, region_name);
>  	if (!region) {
> @@ -4119,16 +4145,25 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
>  		return -ENOSPC;
>  	}
>  
> -	snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
> +	snapshot_id_attr = info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
> +	if (snapshot_id_attr) {
> +		snapshot_id = nla_get_u32(snapshot_id_attr);
>  
> -	if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
> -		NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
> -		return -EEXIST;
> -	}
> +		if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
> +			NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
> +			return -EEXIST;
> +		}
>  
> -	err = __devlink_snapshot_id_insert(devlink, snapshot_id);
> -	if (err)
> -		return err;
> +		err = __devlink_snapshot_id_insert(devlink, snapshot_id);
> +		if (err)
> +			return err;
> +	} else {
> +		err = __devlink_region_snapshot_id_get(devlink, &snapshot_id);
> +		if (err) {
> +			NL_SET_ERR_MSG_MOD(info->extack, "Failed to allocate a new snapshot id");
> +			return err;
> +		}
> +	}
>  
>  	err = region->ops->snapshot(devlink, info->extack, &data);
>  	if (err)
> @@ -4138,6 +4173,14 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
>  	if (err)
>  		goto err_snapshot_create;
>  
> +	if (!snapshot_id_attr) {
> +		/* destroys the snapshot on failure */
> +		err = devlink_nl_snapshot_id_notify(devlink, info,
> +						    region, snapshot_id);
> +		if (err)
> +			return err;
> +	}
> +

Does it make sense to report back the snapshot in all cases? I guess no,
since the caller knows the id, and it doesn't really simplify anything
to display this if you know the id already. Ok.

Thanks for this, I really like how this simplifies creating a snapshot!

-Jake

Re: [PATCH net-next v3 3/3] docs: devlink: clarify the scope of snapshot id

[Jacob Keller]

On 4/30/2020 10:57 AM, Jakub Kicinski wrote:
> In past discussions Jiri explained snapshot ids are cross-region.
> Explain this in the docs.
> 
> v3: new patch
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>


Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH iproute2-next v3] devlink: support kernel-side snapshot id allocation

[Jiri Pirko]

Thu, Apr 30, 2020 at 07:57:59PM CEST, kuba@kernel.org wrote:
>Make ID argument optional and read the snapshot info
>that kernel sends us.
>
>$ devlink region new netdevsim/netdevsim1/dummy
>netdevsim/netdevsim1/dummy: snapshot 0
>$ devlink -jp region new netdevsim/netdevsim1/dummy
>{
>    "regions": {
>        "netdevsim/netdevsim1/dummy": {
>            "snapshot": [ 1 ]
>        }
>    }
>}
>$ devlink region show netdevsim/netdevsim1/dummy
>netdevsim/netdevsim1/dummy: size 32768 snapshot [0 1]
>
>v3: back to v1..
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH iproute2-next v3] devlink: support kernel-side snapshot id allocation

[David Ahern]

On 4/30/20 11:57 AM, Jakub Kicinski wrote:
> Make ID argument optional and read the snapshot info
> that kernel sends us.
> 
> $ devlink region new netdevsim/netdevsim1/dummy
> netdevsim/netdevsim1/dummy: snapshot 0
> $ devlink -jp region new netdevsim/netdevsim1/dummy
> {
>     "regions": {
>         "netdevsim/netdevsim1/dummy": {
>             "snapshot": [ 1 ]
>         }
>     }
> }
> $ devlink region show netdevsim/netdevsim1/dummy
> netdevsim/netdevsim1/dummy: size 32768 snapshot [0 1]
> 
> v3: back to v1..
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  devlink/devlink.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)

this does not apply to current iproute2-next

Re: [PATCH iproute2-next v3] devlink: support kernel-side snapshot id allocation

[Jakub Kicinski]

On Tue, 5 May 2020 10:14:24 -0600 David Ahern wrote:
> On 4/30/20 11:57 AM, Jakub Kicinski wrote:
> > Make ID argument optional and read the snapshot info
> > that kernel sends us.
> > 
> > $ devlink region new netdevsim/netdevsim1/dummy
> > netdevsim/netdevsim1/dummy: snapshot 0
> > $ devlink -jp region new netdevsim/netdevsim1/dummy
> > {
> >     "regions": {
> >         "netdevsim/netdevsim1/dummy": {
> >             "snapshot": [ 1 ]
> >         }
> >     }
> > }
> > $ devlink region show netdevsim/netdevsim1/dummy
> > netdevsim/netdevsim1/dummy: size 32768 snapshot [0 1]
> > 
> > v3: back to v1..
> > 
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  devlink/devlink.c | 26 +++++++++++++++++++++++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)  
> 
> this does not apply to current iproute2-next

Hm. This was on top of Jake's patch, but Stephen took that one into
iproute2, since the kernel feature is in 5.7 already. What is the
protocol here? Can you merge iproute2 into iproute2-next? :S

Re: [PATCH iproute2-next v3] devlink: support kernel-side snapshot id allocation

[David Ahern]

On 5/5/20 10:20 AM, Jakub Kicinski wrote:
> On Tue, 5 May 2020 10:14:24 -0600 David Ahern wrote:
>> On 4/30/20 11:57 AM, Jakub Kicinski wrote:
>>> Make ID argument optional and read the snapshot info
>>> that kernel sends us.
>>>
>>> $ devlink region new netdevsim/netdevsim1/dummy
>>> netdevsim/netdevsim1/dummy: snapshot 0
>>> $ devlink -jp region new netdevsim/netdevsim1/dummy
>>> {
>>>     "regions": {
>>>         "netdevsim/netdevsim1/dummy": {
>>>             "snapshot": [ 1 ]
>>>         }
>>>     }
>>> }
>>> $ devlink region show netdevsim/netdevsim1/dummy
>>> netdevsim/netdevsim1/dummy: size 32768 snapshot [0 1]
>>>
>>> v3: back to v1..
>>>
>>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>> ---
>>>  devlink/devlink.c | 26 +++++++++++++++++++++++---
>>>  1 file changed, 23 insertions(+), 3 deletions(-)  
>>
>> this does not apply to current iproute2-next
> 
> Hm. This was on top of Jake's patch, but Stephen took that one into
> iproute2, since the kernel feature is in 5.7 already. What is the
> protocol here? Can you merge iproute2 into iproute2-next? :S
> 

merged and pushed. can you resend? I deleted it after it failed to apply
and now has vanished.

[PATCH iproute2-next RESEND] devlink: support kernel-side snapshot id allocation

[Jakub Kicinski]

Make ID argument optional and read the snapshot info
that kernel sends us.

$ devlink region new netdevsim/netdevsim1/dummy
netdevsim/netdevsim1/dummy: snapshot 0
$ devlink -jp region new netdevsim/netdevsim1/dummy
{
    "regions": {
        "netdevsim/netdevsim1/dummy": {
            "snapshot": [ 1 ]
        }
    }
}
$ devlink region show netdevsim/netdevsim1/dummy
netdevsim/netdevsim1/dummy: size 32768 snapshot [0 1]

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 devlink/devlink.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index bd48a73bc0e4..507972c360a7 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6476,6 +6476,23 @@ static int cmd_region_read(struct dl *dl)
 	return err;
 }
 
+static int cmd_region_snapshot_new_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+	struct dl *dl = data;
+
+	mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
+	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME] ||
+	    !tb[DEVLINK_ATTR_REGION_NAME] ||
+	    !tb[DEVLINK_ATTR_REGION_SNAPSHOT_ID])
+		return MNL_CB_ERROR;
+
+	pr_out_region(dl, tb);
+
+	return MNL_CB_OK;
+}
+
 static int cmd_region_snapshot_new(struct dl *dl)
 {
 	struct nlmsghdr *nlh;
@@ -6484,12 +6501,15 @@ static int cmd_region_snapshot_new(struct dl *dl)
 	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_REGION_NEW,
 			       NLM_F_REQUEST | NLM_F_ACK);
 
-	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE_REGION |
-				DL_OPT_REGION_SNAPSHOT_ID, 0);
+	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE_REGION,
+				DL_OPT_REGION_SNAPSHOT_ID);
 	if (err)
 		return err;
 
-	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+	pr_out_section_start(dl, "regions");
+	err = _mnlg_socket_sndrcv(dl->nlg, nlh, cmd_region_snapshot_new_cb, dl);
+	pr_out_section_end(dl);
+	return err;
 }
 
 static void cmd_region_help(void)
-- 
2.25.4

Re: [PATCH iproute2-next v3] devlink: support kernel-side snapshot id allocation

[Jakub Kicinski]

On Tue, 5 May 2020 10:59:28 -0600 David Ahern wrote:
> merged and pushed. can you resend? I deleted it after it failed to apply
> and now has vanished.

Thanks! Resent now

Re: [PATCH iproute2-next v3] devlink: support kernel-side snapshot id allocation

[Jakub Kicinski]

On Tue, 5 May 2020 10:05:36 -0700 Jakub Kicinski wrote:
> On Tue, 5 May 2020 10:59:28 -0600 David Ahern wrote:
> > merged and pushed. can you resend? I deleted it after it failed to apply
> > and now has vanished.  
> 
> Thanks! Resent now

Ah damn, I didn't add Jiri's review tag, should I resent again?

Re: [PATCH iproute2-next RESEND] devlink: support kernel-side snapshot id allocation

[David Ahern]

On 5/5/20 11:04 AM, Jakub Kicinski wrote:
> Make ID argument optional and read the snapshot info
> that kernel sends us.
> 
> $ devlink region new netdevsim/netdevsim1/dummy
> netdevsim/netdevsim1/dummy: snapshot 0
> $ devlink -jp region new netdevsim/netdevsim1/dummy
> {
>     "regions": {
>         "netdevsim/netdevsim1/dummy": {
>             "snapshot": [ 1 ]
>         }
>     }
> }
> $ devlink region show netdevsim/netdevsim1/dummy
> netdevsim/netdevsim1/dummy: size 32768 snapshot [0 1]
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  devlink/devlink.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 

applied to iproute2-next. Thanks