| Message ID | 20200430175759.1301789-3-kuba@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | devlink: kernel region snapshot id allocation | expand |
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 >
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
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" }
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(-)