[PATCH net-next] devlink: let kernel allocate region snapshot id

[PATCH net-next] 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.

The message carrying id gets sent immediately, but the
allocation is only valid if the entire operation succeeded.
This makes life easier, as sending the notification itself
may fail.

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

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Jiri, this is what I had in mind of snapshots and the same
thing will come back for slice allocation.

 net/core/devlink.c                            | 84 ++++++++++++++++---
 .../drivers/net/netdevsim/devlink.sh          | 13 +++
 2 files changed, 84 insertions(+), 13 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 1ec2e9fd8898..dad5d07dd4f8 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4065,10 +4065,65 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
 	return 0;
 }
 
+static int
+devlink_nl_alloc_snapshot_id(struct devlink *devlink, struct genl_info *info,
+			     struct devlink_region *region, u32 *snapshot_id)
+{
+	struct sk_buff *msg;
+	void *hdr;
+	int err;
+
+	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;
+	}
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg) {
+		err = -ENOMEM;
+		goto err_msg_alloc;
+	}
+
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, DEVLINK_CMD_REGION_NEW);
+	if (!hdr) {
+		err = -EMSGSIZE;
+		goto err_put_failure;
+	}
+	err = devlink_nl_put_handle(msg, devlink);
+	if (err)
+		goto err_attr_failure;
+	err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops->name);
+	if (err)
+		goto err_attr_failure;
+	err = nla_put_u32(msg, DEVLINK_ATTR_REGION_SNAPSHOT_ID, *snapshot_id);
+	if (err)
+		goto err_attr_failure;
+	genlmsg_end(msg, hdr);
+
+	err = genlmsg_reply(msg, info);
+	if (err)
+		goto err_reply;
+
+	return 0;
+
+err_attr_failure:
+	genlmsg_cancel(msg, hdr);
+err_put_failure:
+	nlmsg_free(msg);
+err_msg_alloc:
+err_reply:
+	__devlink_snapshot_id_decrement(devlink, *snapshot_id);
+	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;
@@ -4080,11 +4135,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) {
@@ -4102,16 +4152,24 @@ 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_nl_alloc_snapshot_id(devlink, info,
+						   region, &snapshot_id);
+		if (err)
+			return err;
+	}
 
 	err = region->ops->snapshot(devlink, info->extack, &data);
 	if (err)
diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
index 9f9741444549..ad539eccddcb 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
@@ -151,6 +151,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] devlink: let kernel allocate region snapshot id

[Jiri Pirko]

Wed, Apr 29, 2020 at 03:42:48AM 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.
>
>The message carrying id gets sent immediately, but the
>allocation is only valid if the entire operation succeeded.
>This makes life easier, as sending the notification itself
>may fail.
>
>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
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
>Jiri, this is what I had in mind of snapshots and the same
>thing will come back for slice allocation.

Okay. Could you please send the userspace patch too in order to see the
full picture?


>
> net/core/devlink.c                            | 84 ++++++++++++++++---
> .../drivers/net/netdevsim/devlink.sh          | 13 +++
> 2 files changed, 84 insertions(+), 13 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 1ec2e9fd8898..dad5d07dd4f8 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4065,10 +4065,65 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
> 	return 0;
> }
> 
>+static int
>+devlink_nl_alloc_snapshot_id(struct devlink *devlink, struct genl_info *info,
>+			     struct devlink_region *region, u32 *snapshot_id)
>+{
>+	struct sk_buff *msg;
>+	void *hdr;
>+	int err;
>+
>+	err = __devlink_region_snapshot_id_get(devlink, snapshot_id);
>+	if (err) {
>+		NL_SET_ERR_MSG_MOD(info->extack,

No need to wrap here.


>+				   "Failed to allocate a new snapshot id");
>+		return err;
>+	}
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg) {
>+		err = -ENOMEM;
>+		goto err_msg_alloc;
>+	}
>+
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, DEVLINK_CMD_REGION_NEW);
>+	if (!hdr) {
>+		err = -EMSGSIZE;
>+		goto err_put_failure;
>+	}
>+	err = devlink_nl_put_handle(msg, devlink);
>+	if (err)
>+		goto err_attr_failure;
>+	err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops->name);
>+	if (err)
>+		goto err_attr_failure;
>+	err = nla_put_u32(msg, DEVLINK_ATTR_REGION_SNAPSHOT_ID, *snapshot_id);
>+	if (err)
>+		goto err_attr_failure;
>+	genlmsg_end(msg, hdr);
>+
>+	err = genlmsg_reply(msg, info);
>+	if (err)
>+		goto err_reply;
>+
>+	return 0;
>+
>+err_attr_failure:
>+	genlmsg_cancel(msg, hdr);
>+err_put_failure:
>+	nlmsg_free(msg);
>+err_msg_alloc:
>+err_reply:
>+	__devlink_snapshot_id_decrement(devlink, *snapshot_id);
>+	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;
>@@ -4080,11 +4135,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) {
>@@ -4102,16 +4152,24 @@ 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_nl_alloc_snapshot_id(devlink, info,
>+						   region, &snapshot_id);
>+		if (err)
>+			return err;
>+	}
> 
> 	err = region->ops->snapshot(devlink, info->extack, &data);

How the output is going to looks like it this or any of the follow-up
calls in this function are going to fail?

I guess it is going to be handled gracefully in the userspace app,
right?



> 	if (err)
>diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>index 9f9741444549..ad539eccddcb 100755
>--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>@@ -151,6 +151,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] devlink: let kernel allocate region snapshot id

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Jiri Pirko
> Sent: Tuesday, April 28, 2020 10:46 PM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; kernel-team@fb.com;
> Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: Re: [PATCH net-next] devlink: let kernel allocate region snapshot id
> 
> Wed, Apr 29, 2020 at 03:42:48AM 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.
> >

I did propose something like this originally, but....

> >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.
> >

... sending a message back makes this work better.

> >The message carrying id gets sent immediately, but the
> >allocation is only valid if the entire operation succeeded.
> >This makes life easier, as sending the notification itself
> >may fail.
> >

It seems like we could wait until at least after the region is captured.

> >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
> >
> >Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> >---
> >Jiri, this is what I had in mind of snapshots and the same
> >thing will come back for slice allocation.
> 
> Okay. Could you please send the userspace patch too in order to see the
> full picture?
> 

Yes, I'd like to see this as well.

> 
> >
> > net/core/devlink.c                            | 84 ++++++++++++++++---
> > .../drivers/net/netdevsim/devlink.sh          | 13 +++
> > 2 files changed, 84 insertions(+), 13 deletions(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index 1ec2e9fd8898..dad5d07dd4f8 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -4065,10 +4065,65 @@ static int devlink_nl_cmd_region_del(struct sk_buff
> *skb,
> > 	return 0;
> > }
> >
> >+static int
> >+devlink_nl_alloc_snapshot_id(struct devlink *devlink, struct genl_info *info,
> >+			     struct devlink_region *region, u32 *snapshot_id)
> >+{
> >+	struct sk_buff *msg;
> >+	void *hdr;
> >+	int err;
> >+
> >+	err = __devlink_region_snapshot_id_get(devlink, snapshot_id);
> >+	if (err) {
> >+		NL_SET_ERR_MSG_MOD(info->extack,
> 
> No need to wrap here.
> 
> 
> >+				   "Failed to allocate a new snapshot id");
> >+		return err;
> >+	}
> >+
> >+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >+	if (!msg) {
> >+		err = -ENOMEM;
> >+		goto err_msg_alloc;
> >+	}
> >+
> >+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> >+			  &devlink_nl_family, 0, DEVLINK_CMD_REGION_NEW);
> >+	if (!hdr) {
> >+		err = -EMSGSIZE;
> >+		goto err_put_failure;
> >+	}
> >+	err = devlink_nl_put_handle(msg, devlink);
> >+	if (err)
> >+		goto err_attr_failure;
> >+	err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops-
> >name);
> >+	if (err)
> >+		goto err_attr_failure;
> >+	err = nla_put_u32(msg, DEVLINK_ATTR_REGION_SNAPSHOT_ID,
> *snapshot_id);
> >+	if (err)
> >+		goto err_attr_failure;
> >+	genlmsg_end(msg, hdr);
> >+
> >+	err = genlmsg_reply(msg, info);
> >+	if (err)
> >+		goto err_reply;
> >+
> >+	return 0;
> >+
> >+err_attr_failure:
> >+	genlmsg_cancel(msg, hdr);
> >+err_put_failure:
> >+	nlmsg_free(msg);
> >+err_msg_alloc:
> >+err_reply:
> >+	__devlink_snapshot_id_decrement(devlink, *snapshot_id);
> >+	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;
> >@@ -4080,11 +4135,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) {
> >@@ -4102,16 +4152,24 @@ 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_nl_alloc_snapshot_id(devlink, info,
> >+						   region, &snapshot_id);
> >+		if (err)
> >+			return err;
> >+	}
> >
> > 	err = region->ops->snapshot(devlink, info->extack, &data);
> 
> How the output is going to looks like it this or any of the follow-up
> calls in this function are going to fail?
> 
> I guess it is going to be handled gracefully in the userspace app,
> right?
> 
> 

I'm wondering what the issue is with just waiting to send the snapshot id back until after this succeeds. Is it just easier to keep it near the allocation?

Thanks,
Jake

[PATCH iproute2-next] 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 net-next] devlink: let kernel allocate region snapshot id

[Jakub Kicinski]

On Wed, 29 Apr 2020 07:45:52 +0200 Jiri Pirko wrote:
> Wed, Apr 29, 2020 at 03:42:48AM CEST, kuba@kernel.org wrote:
> >Jiri, this is what I had in mind of snapshots and the same
> >thing will come back for slice allocation.  
> 
> Okay. Could you please send the userspace patch too in order to see the
> full picture?

You got it, I didn't do anything fancy there.

> >+static int
> >+devlink_nl_alloc_snapshot_id(struct devlink *devlink, struct genl_info *info,
> >+			     struct devlink_region *region, u32 *snapshot_id)
> >+{
> >+	struct sk_buff *msg;
> >+	void *hdr;
> >+	int err;
> >+
> >+	err = __devlink_region_snapshot_id_get(devlink, snapshot_id);
> >+	if (err) {
> >+		NL_SET_ERR_MSG_MOD(info->extack,  
> 
> No need to wrap here.

Ok.

> >+				   "Failed to allocate a new snapshot id");
> >+		return err;
> >+	}

> >-	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_nl_alloc_snapshot_id(devlink, info,
> >+						   region, &snapshot_id);
> >+		if (err)
> >+			return err;
> >+	}
> > 
> > 	err = region->ops->snapshot(devlink, info->extack, &data);  
> 
> How the output is going to looks like it this or any of the follow-up
> calls in this function are going to fail?
> 
> I guess it is going to be handled gracefully in the userspace app,
> right?

The output is the same, just the return code is non-zero.

I can change that not to print the snapshot info until we are sure the
operation succeeded if you prefer.

Initially I had the kernel not sent the message until it's done with
the operation, but that seems unnecessarily complex. The send operation
itself may fail, and if we ever have an operation that requires more
than one notification we'll have to struggle with atomic sends.

Better have the user space treat the failure like an exception, and
ignore all the notifications that came earlier.

That said the iproute2 patch can be improved with extra 20 lines so
that it holds off printing the snapshot info until success.

Re: [PATCH net-next] devlink: let kernel allocate region snapshot id

[Jakub Kicinski]

On Wed, 29 Apr 2020 15:34:30 +0000 Keller, Jacob E wrote:
> > How the output is going to looks like it this or any of the follow-up
> > calls in this function are going to fail?
> > 
> > I guess it is going to be handled gracefully in the userspace app,
> > right?
>
> I'm wondering what the issue is with just waiting to send the
> snapshot id back until after this succeeds. Is it just easier to keep
> it near the allocation?

I just wasn't happy with the fact that the response send may fail.
So it just seems like better protocol from the start to expect user
space to pay attention to the error code at the end, before it takes
action on the response.

RE: [PATCH net-next] devlink: let kernel allocate region snapshot id

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, April 29, 2020 9:39 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; davem@davemloft.net; netdev@vger.kernel.org;
> kernel-team@fb.com
> Subject: Re: [PATCH net-next] devlink: let kernel allocate region snapshot id
> 
> On Wed, 29 Apr 2020 15:34:30 +0000 Keller, Jacob E wrote:
> > > How the output is going to looks like it this or any of the follow-up
> > > calls in this function are going to fail?
> > >
> > > I guess it is going to be handled gracefully in the userspace app,
> > > right?
> >
> > I'm wondering what the issue is with just waiting to send the
> > snapshot id back until after this succeeds. Is it just easier to keep
> > it near the allocation?
> 
> I just wasn't happy with the fact that the response send may fail.
> So it just seems like better protocol from the start to expect user
> space to pay attention to the error code at the end, before it takes
> action on the response.

Ok that seems reasonable to me.

Can we get documentation updates for this?

Thanks,
Jake

Re: [PATCH net-next] devlink: let kernel allocate region snapshot id

[Jiri Pirko]

Wed, Apr 29, 2020 at 06:35:18PM CEST, kuba@kernel.org wrote:
>On Wed, 29 Apr 2020 07:45:52 +0200 Jiri Pirko wrote:
>> Wed, Apr 29, 2020 at 03:42:48AM CEST, kuba@kernel.org wrote:
>> >Jiri, this is what I had in mind of snapshots and the same
>> >thing will come back for slice allocation.  
>> 
>> Okay. Could you please send the userspace patch too in order to see the
>> full picture?
>
>You got it, I didn't do anything fancy there.
>
>> >+static int
>> >+devlink_nl_alloc_snapshot_id(struct devlink *devlink, struct genl_info *info,
>> >+			     struct devlink_region *region, u32 *snapshot_id)
>> >+{
>> >+	struct sk_buff *msg;
>> >+	void *hdr;
>> >+	int err;
>> >+
>> >+	err = __devlink_region_snapshot_id_get(devlink, snapshot_id);
>> >+	if (err) {
>> >+		NL_SET_ERR_MSG_MOD(info->extack,  
>> 
>> No need to wrap here.
>
>Ok.
>
>> >+				   "Failed to allocate a new snapshot id");
>> >+		return err;
>> >+	}
>
>> >-	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_nl_alloc_snapshot_id(devlink, info,
>> >+						   region, &snapshot_id);
>> >+		if (err)
>> >+			return err;
>> >+	}
>> > 
>> > 	err = region->ops->snapshot(devlink, info->extack, &data);  
>> 
>> How the output is going to looks like it this or any of the follow-up
>> calls in this function are going to fail?
>> 
>> I guess it is going to be handled gracefully in the userspace app,
>> right?
>
>The output is the same, just the return code is non-zero.
>
>I can change that not to print the snapshot info until we are sure the
>operation succeeded if you prefer.

I think that would be clean to do this, in kernel.


>
>Initially I had the kernel not sent the message until it's done with
>the operation, but that seems unnecessarily complex. The send operation
>itself may fail, and if we ever have an operation that requires more
>than one notification we'll have to struggle with atomic sends.
>
>Better have the user space treat the failure like an exception, and
>ignore all the notifications that came earlier.
>
>That said the iproute2 patch can be improved with extra 20 lines so
>that it holds off printing the snapshot info until success.