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