* [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW
@ 2020-03-24 22:34 Jacob Keller
2020-03-24 22:34 ` [PATCH 01/10] devlink: prepare to support region operations Jacob Keller
` (11 more replies)
0 siblings, 12 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller
This series adds support for the DEVLINK_CMD_REGION_NEW operation, used to
enable userspace requesting a snapshot of a region on demand.
This can be useful to enable adding regions for a driver for which there is
no trigger to create snapshots. By making this a core part of devlink, there
is no need for the drivers to use a separate channel such as debugfs.
The primary intent for this kind of region is to expose device information
that might be useful for diagnostics and information gathering.
The first few patches refactor regions to support a new ops structure for
extending the available operations that regions can perform. This includes
converting the destructor into an op from a function argument.
Next, patches refactor the snapshot id allocation to use an xarray which
tracks the number of current snapshots using a given id. This is done so
that id lifetime can be determined, and ids can be released when no longer
in use.
Without this change, snapshot ids remain used forever, until the snapshot_id
count rolled over UINT_MAX.
Finally, code to enable the previously unused DEVLINK_CMD_REGION_NEW is
added. This code enforces that the snapshot id is always provided, unlike
previous revisions of this series.
Finally, a patch is added to enable using this new command via the .snapshot
callback in both netdevsim and the ice driver.
For the ice driver, a new "nvm-flash" region is added, which will enable
read access to the NVM flash contents. The intention for this is to allow
diagnostics tools to gather information about the device. By using a
snapshot and gathering the NVM contents all at once, the contents can be
atomic.
General changes since the v2 RFC:
* Use an xarray instead of IDRs
* Rebase onto net-next now that the initial ice devlink patches landed.
Patch specific changes:
* devlink: add functions to take snapshot while locked
Split this into two patches, so that an explanation of why the
devlink_region_snapshot_id_get is still extracted, even though only one
caller will remain.
* devlink: track snapshot ids using an IDR and refcounts
Convert to using an xarray storing the total number of snapshots directly,
rather than an IDR with a refcount structure. This significantly
simplifies the code, and avoids the complication of a NULL refcount.
* devlink: implement DEVLINK_CMD_REGION_NEW
As suggested by Jiri, remove the ability for DEVLINK_CMD_REGION_NEW to
dynamically generate IDs. Instead, always require a snapshot id. This
aligns with DEVLINK_CMD_REGION_DEL, and helps reduce confusion.
Refactor this patch to use the xarray instead of the IDR, as in the
previous patch.
Clean up and remove unnecessary new lines on NL_SET_ERR_MSG_MOD
* ice: add a devlink region to dump shadow RAM contents
Remove the code for immediate region read, as this will be worked on in a
separate series following this one.
Jacob Keller (10):
devlink: prepare to support region operations
devlink: convert snapshot destructor callback to region op
devlink: trivial: fix tab in function documentation
devlink: add function to take snapshot while locked
devlink: extract snapshot id allocation to helper function
devlink: convert snapshot id getter to return an error
devlink: track snapshot id usage count using an xarray
devlink: implement DEVLINK_CMD_REGION_NEW
netdevsim: support taking immediate snapshot via devlink
ice: add a devlink region for dumping NVM contents
.../networking/devlink/devlink-region.rst | 8 +
Documentation/networking/devlink/ice.rst | 26 ++
drivers/net/ethernet/intel/ice/ice.h | 2 +
drivers/net/ethernet/intel/ice/ice_devlink.c | 99 +++++
drivers/net/ethernet/intel/ice/ice_devlink.h | 3 +
drivers/net/ethernet/intel/ice/ice_main.c | 4 +
drivers/net/ethernet/mellanox/mlx4/crdump.c | 32 +-
drivers/net/netdevsim/dev.c | 41 +-
include/net/devlink.h | 31 +-
net/core/devlink.c | 354 +++++++++++++++---
.../drivers/net/netdevsim/devlink.sh | 15 +
11 files changed, 538 insertions(+), 77 deletions(-)
--
2.24.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 01/10] devlink: prepare to support region operations
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-24 22:34 ` [PATCH 02/10] devlink: convert snapshot destructor callback to region op Jacob Keller
` (10 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller, Jiri Pirko
Modify the devlink region code in preparation for adding new operations
on regions.
Create a devlink_region_ops structure, and move the name pointer from
within the devlink_region structure into the ops structure (similar to
the devlink_health_reporter_ops).
This prepares the regions to enable support of additional operations in
the future such as requesting snapshots, or accessing the region
directly without a snapshot.
In order to re-use the constant strings in the mlx4 driver their
declaration must be changed to 'const char * const' to ensure the
compiler realizes that both the data and the pointer cannot change.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx4/crdump.c | 16 +++++++++++----
drivers/net/netdevsim/dev.c | 6 +++++-
include/net/devlink.h | 16 +++++++++++----
net/core/devlink.c | 22 ++++++++++-----------
4 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c
index 64ed725aec28..cc2bf596c74b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/crdump.c
+++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c
@@ -38,8 +38,16 @@
#define CR_ENABLE_BIT_OFFSET 0xF3F04
#define MAX_NUM_OF_DUMPS_TO_STORE (8)
-static const char *region_cr_space_str = "cr-space";
-static const char *region_fw_health_str = "fw-health";
+static const char * const region_cr_space_str = "cr-space";
+static const char * const region_fw_health_str = "fw-health";
+
+static const struct devlink_region_ops region_cr_space_ops = {
+ .name = region_cr_space_str,
+};
+
+static const struct devlink_region_ops region_fw_health_ops = {
+ .name = region_fw_health_str,
+};
/* Set to true in case cr enable bit was set to true before crdump */
static bool crdump_enbale_bit_set;
@@ -205,7 +213,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev)
/* Create cr-space region */
crdump->region_crspace =
devlink_region_create(devlink,
- region_cr_space_str,
+ ®ion_cr_space_ops,
MAX_NUM_OF_DUMPS_TO_STORE,
pci_resource_len(pdev, 0));
if (IS_ERR(crdump->region_crspace))
@@ -216,7 +224,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev)
/* Create fw-health region */
crdump->region_fw_health =
devlink_region_create(devlink,
- region_fw_health_str,
+ ®ion_fw_health_ops,
MAX_NUM_OF_DUMPS_TO_STORE,
HEALTH_BUFFER_SIZE);
if (IS_ERR(crdump->region_fw_health))
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 7bfd0622cef1..47a8f8c570c4 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -340,11 +340,15 @@ static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink)
#define NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX 16
+static const struct devlink_region_ops dummy_region_ops = {
+ .name = "dummy",
+};
+
static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
struct devlink *devlink)
{
nsim_dev->dummy_region =
- devlink_region_create(devlink, "dummy",
+ devlink_region_create(devlink, &dummy_region_ops,
NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
NSIM_DEV_DUMMY_REGION_SIZE);
return PTR_ERR_OR_ZERO(nsim_dev->dummy_region);
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 37230e23b5b0..85db5dd5184d 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -498,6 +498,14 @@ struct devlink_info_req;
typedef void devlink_snapshot_data_dest_t(const void *data);
+/**
+ * struct devlink_region_ops - Region operations
+ * @name: region name
+ */
+struct devlink_region_ops {
+ const char *name;
+};
+
struct devlink_fmsg;
struct devlink_health_reporter;
@@ -963,10 +971,10 @@ void devlink_port_param_value_changed(struct devlink_port *devlink_port,
u32 param_id);
void devlink_param_value_str_fill(union devlink_param_value *dst_val,
const char *src);
-struct devlink_region *devlink_region_create(struct devlink *devlink,
- const char *region_name,
- u32 region_max_snapshots,
- u64 region_size);
+struct devlink_region *
+devlink_region_create(struct devlink *devlink,
+ const struct devlink_region_ops *ops,
+ u32 region_max_snapshots, u64 region_size);
void devlink_region_destroy(struct devlink_region *region);
u32 devlink_region_snapshot_id_get(struct devlink *devlink);
int devlink_region_snapshot_create(struct devlink_region *region,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 73bb8fbe3393..ca5362530567 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -344,7 +344,7 @@ devlink_sb_tc_index_get_from_info(struct devlink_sb *devlink_sb,
struct devlink_region {
struct devlink *devlink;
struct list_head list;
- const char *name;
+ const struct devlink_region_ops *ops;
struct list_head snapshot_list;
u32 max_snapshots;
u32 cur_snapshots;
@@ -365,7 +365,7 @@ devlink_region_get_by_name(struct devlink *devlink, const char *region_name)
struct devlink_region *region;
list_for_each_entry(region, &devlink->region_list, list)
- if (!strcmp(region->name, region_name))
+ if (!strcmp(region->ops->name, region_name))
return region;
return NULL;
@@ -3695,7 +3695,7 @@ static int devlink_nl_region_fill(struct sk_buff *msg, struct devlink *devlink,
if (err)
goto nla_put_failure;
- err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->name);
+ err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops->name);
if (err)
goto nla_put_failure;
@@ -3741,7 +3741,7 @@ static void devlink_nl_region_notify(struct devlink_region *region,
goto out_cancel_msg;
err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME,
- region->name);
+ region->ops->name);
if (err)
goto out_cancel_msg;
@@ -7647,21 +7647,21 @@ EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
* devlink_region_create - create a new address region
*
* @devlink: devlink
- * @region_name: region name
+ * @ops: region operations and name
* @region_max_snapshots: Maximum supported number of snapshots for region
* @region_size: size of region
*/
-struct devlink_region *devlink_region_create(struct devlink *devlink,
- const char *region_name,
- u32 region_max_snapshots,
- u64 region_size)
+struct devlink_region *
+devlink_region_create(struct devlink *devlink,
+ const struct devlink_region_ops *ops,
+ u32 region_max_snapshots, u64 region_size)
{
struct devlink_region *region;
int err = 0;
mutex_lock(&devlink->lock);
- if (devlink_region_get_by_name(devlink, region_name)) {
+ if (devlink_region_get_by_name(devlink, ops->name)) {
err = -EEXIST;
goto unlock;
}
@@ -7674,7 +7674,7 @@ struct devlink_region *devlink_region_create(struct devlink *devlink,
region->devlink = devlink;
region->max_snapshots = region_max_snapshots;
- region->name = region_name;
+ region->ops = ops;
region->size = region_size;
INIT_LIST_HEAD(®ion->snapshot_list);
list_add_tail(®ion->list, &devlink->region_list);
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 02/10] devlink: convert snapshot destructor callback to region op
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
2020-03-24 22:34 ` [PATCH 01/10] devlink: prepare to support region operations Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-24 22:34 ` [PATCH 03/10] devlink: trivial: fix tab in function documentation Jacob Keller
` (9 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller, Jiri Pirko
It does not makes sense that two snapshots for a given region would use
different destructors. Simplify snapshot creation by adding
a .destructor op for regions.
This operation will replace the data_destructor for the snapshot
creation, and makes snapshot creation easier.
Noticed-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx4/crdump.c | 6 ++++--
drivers/net/netdevsim/dev.c | 3 ++-
include/net/devlink.h | 7 +++----
net/core/devlink.c | 11 +++++------
4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c
index cc2bf596c74b..c3f90c0f9554 100644
--- a/drivers/net/ethernet/mellanox/mlx4/crdump.c
+++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c
@@ -43,10 +43,12 @@ static const char * const region_fw_health_str = "fw-health";
static const struct devlink_region_ops region_cr_space_ops = {
.name = region_cr_space_str,
+ .destructor = &kvfree,
};
static const struct devlink_region_ops region_fw_health_ops = {
.name = region_fw_health_str,
+ .destructor = &kvfree,
};
/* Set to true in case cr enable bit was set to true before crdump */
@@ -107,7 +109,7 @@ static void mlx4_crdump_collect_crspace(struct mlx4_dev *dev,
readl(cr_space + offset);
err = devlink_region_snapshot_create(crdump->region_crspace,
- crspace_data, id, &kvfree);
+ crspace_data, id);
if (err) {
kvfree(crspace_data);
mlx4_warn(dev, "crdump: devlink create %s snapshot id %d err %d\n",
@@ -146,7 +148,7 @@ static void mlx4_crdump_collect_fw_health(struct mlx4_dev *dev,
readl(health_buf_start + offset);
err = devlink_region_snapshot_create(crdump->region_fw_health,
- health_data, id, &kvfree);
+ health_data, id);
if (err) {
kvfree(health_data);
mlx4_warn(dev, "crdump: devlink create %s snapshot id %d err %d\n",
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 47a8f8c570c4..f7621ccb7b88 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -56,7 +56,7 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
err = devlink_region_snapshot_create(nsim_dev->dummy_region,
- dummy_data, id, kfree);
+ dummy_data, id);
if (err) {
pr_err("Failed to create region snapshot\n");
kfree(dummy_data);
@@ -342,6 +342,7 @@ static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink)
static const struct devlink_region_ops dummy_region_ops = {
.name = "dummy",
+ .destructor = &kfree,
};
static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 85db5dd5184d..8869ad75b965 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -496,14 +496,14 @@ enum devlink_param_generic_id {
struct devlink_region;
struct devlink_info_req;
-typedef void devlink_snapshot_data_dest_t(const void *data);
-
/**
* struct devlink_region_ops - Region operations
* @name: region name
+ * @destructor: callback used to free snapshot memory when deleting
*/
struct devlink_region_ops {
const char *name;
+ void (*destructor)(const void *data);
};
struct devlink_fmsg;
@@ -978,8 +978,7 @@ devlink_region_create(struct devlink *devlink,
void devlink_region_destroy(struct devlink_region *region);
u32 devlink_region_snapshot_id_get(struct devlink *devlink);
int devlink_region_snapshot_create(struct devlink_region *region,
- u8 *data, u32 snapshot_id,
- devlink_snapshot_data_dest_t *data_destructor);
+ u8 *data, u32 snapshot_id);
int devlink_info_serial_number_put(struct devlink_info_req *req,
const char *sn);
int devlink_info_driver_name_put(struct devlink_info_req *req,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ca5362530567..84d74fbcff62 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -354,7 +354,6 @@ struct devlink_region {
struct devlink_snapshot {
struct list_head list;
struct devlink_region *region;
- devlink_snapshot_data_dest_t *data_destructor;
u8 *data;
u32 id;
};
@@ -3775,7 +3774,7 @@ static void devlink_region_snapshot_del(struct devlink_region *region,
devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_DEL);
region->cur_snapshots--;
list_del(&snapshot->list);
- (*snapshot->data_destructor)(snapshot->data);
+ region->ops->destructor(snapshot->data);
kfree(snapshot);
}
@@ -7659,6 +7658,9 @@ devlink_region_create(struct devlink *devlink,
struct devlink_region *region;
int err = 0;
+ if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
+ return ERR_PTR(-EINVAL);
+
mutex_lock(&devlink->lock);
if (devlink_region_get_by_name(devlink, ops->name)) {
@@ -7745,11 +7747,9 @@ EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_get);
* @region: devlink region of the snapshot
* @data: snapshot data
* @snapshot_id: snapshot id to be created
- * @data_destructor: pointer to destructor function to free data
*/
int devlink_region_snapshot_create(struct devlink_region *region,
- u8 *data, u32 snapshot_id,
- devlink_snapshot_data_dest_t *data_destructor)
+ u8 *data, u32 snapshot_id)
{
struct devlink *devlink = region->devlink;
struct devlink_snapshot *snapshot;
@@ -7777,7 +7777,6 @@ int devlink_region_snapshot_create(struct devlink_region *region,
snapshot->id = snapshot_id;
snapshot->region = region;
snapshot->data = data;
- snapshot->data_destructor = data_destructor;
list_add_tail(&snapshot->list, ®ion->snapshot_list);
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 03/10] devlink: trivial: fix tab in function documentation
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
2020-03-24 22:34 ` [PATCH 01/10] devlink: prepare to support region operations Jacob Keller
2020-03-24 22:34 ` [PATCH 02/10] devlink: convert snapshot destructor callback to region op Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-24 22:34 ` [PATCH 04/10] devlink: add function to take snapshot while locked Jacob Keller
` (8 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller, Jiri Pirko
The function documentation comment for devlink_region_snapshot_create
included a literal tab character between 'future analyses' that was
difficult to spot as it happened to only display as one space wide.
Fix the comment to use a space here instead of a stray tab appearing in
the middle of a sentence.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
net/core/devlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 84d74fbcff62..73e66a779c13 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -7740,7 +7740,7 @@ EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_get);
* devlink_region_snapshot_create - create a new snapshot
* This will add a new snapshot of a region. The snapshot
* will be stored on the region struct and can be accessed
- * from devlink. This is useful for future analyses of snapshots.
+ * from devlink. This is useful for future analyses of snapshots.
* Multiple snapshots can be created on a region.
* The @snapshot_id should be obtained using the getter function.
*
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 04/10] devlink: add function to take snapshot while locked
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (2 preceding siblings ...)
2020-03-24 22:34 ` [PATCH 03/10] devlink: trivial: fix tab in function documentation Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-25 17:56 ` Jakub Kicinski
2020-03-24 22:34 ` [PATCH 05/10] devlink: extract snapshot id allocation to helper function Jacob Keller
` (7 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller, Jiri Pirko
A future change is going to add a new devlink command to request
a snapshot on demand. This function will want to call the
devlink_region_snapshot_create function while already holding the
devlink instance lock.
Extract the logic of this function into a static function prefixed by
`__` to indicate that it is an internal helper function. Modify the
original function to be implemented in terms of the new locked
function.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
net/core/devlink.c | 78 ++++++++++++++++++++++++++++------------------
1 file changed, 47 insertions(+), 31 deletions(-)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 73e66a779c13..620e9d07ac85 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3768,6 +3768,52 @@ static void devlink_nl_region_notify(struct devlink_region *region,
nlmsg_free(msg);
}
+/**
+ * __devlink_region_snapshot_create - create a new snapshot
+ * This will add a new snapshot of a region. The snapshot
+ * will be stored on the region struct and can be accessed
+ * from devlink. This is useful for future analyses of snapshots.
+ * Multiple snapshots can be created on a region.
+ * The @snapshot_id should be obtained using the getter function.
+ *
+ * Must be called only while holding the devlink instance lock.
+ *
+ * @region: devlink region of the snapshot
+ * @data: snapshot data
+ * @snapshot_id: snapshot id to be created
+ */
+static int
+__devlink_region_snapshot_create(struct devlink_region *region,
+ u8 *data, u32 snapshot_id)
+{
+ struct devlink *devlink = region->devlink;
+ struct devlink_snapshot *snapshot;
+
+ lockdep_assert_held(&devlink->lock);
+
+ /* check if region can hold one more snapshot */
+ if (region->cur_snapshots == region->max_snapshots)
+ return -ENOMEM;
+
+ if (devlink_region_snapshot_get_by_id(region, snapshot_id))
+ return -EEXIST;
+
+ snapshot = kzalloc(sizeof(*snapshot), GFP_KERNEL);
+ if (!snapshot)
+ return -ENOMEM;
+
+ snapshot->id = snapshot_id;
+ snapshot->region = region;
+ snapshot->data = data;
+
+ list_add_tail(&snapshot->list, ®ion->snapshot_list);
+
+ region->cur_snapshots++;
+
+ devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
+ return 0;
+}
+
static void devlink_region_snapshot_del(struct devlink_region *region,
struct devlink_snapshot *snapshot)
{
@@ -7752,42 +7798,12 @@ int devlink_region_snapshot_create(struct devlink_region *region,
u8 *data, u32 snapshot_id)
{
struct devlink *devlink = region->devlink;
- struct devlink_snapshot *snapshot;
int err;
mutex_lock(&devlink->lock);
-
- /* check if region can hold one more snapshot */
- if (region->cur_snapshots == region->max_snapshots) {
- err = -ENOMEM;
- goto unlock;
- }
-
- if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
- err = -EEXIST;
- goto unlock;
- }
-
- snapshot = kzalloc(sizeof(*snapshot), GFP_KERNEL);
- if (!snapshot) {
- err = -ENOMEM;
- goto unlock;
- }
-
- snapshot->id = snapshot_id;
- snapshot->region = region;
- snapshot->data = data;
-
- list_add_tail(&snapshot->list, ®ion->snapshot_list);
-
- region->cur_snapshots++;
-
- devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
+ err = __devlink_region_snapshot_create(region, data, snapshot_id);
mutex_unlock(&devlink->lock);
- return 0;
-unlock:
- mutex_unlock(&devlink->lock);
return err;
}
EXPORT_SYMBOL_GPL(devlink_region_snapshot_create);
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 05/10] devlink: extract snapshot id allocation to helper function
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (3 preceding siblings ...)
2020-03-24 22:34 ` [PATCH 04/10] devlink: add function to take snapshot while locked Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-25 14:08 ` Jiri Pirko
2020-03-24 22:34 ` [PATCH 06/10] devlink: convert snapshot id getter to return an error Jacob Keller
` (6 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller
A future change is going to implement a new devlink command to request
a snapshot on demand. As part of this, the logic for handling the
snapshot ids will be refactored. To simplify the snapshot id allocation
function, move it to a separate function prefixed by `__`. This helper
function will assume the lock is held.
While no other callers will exist, it simplifies refactoring the logic
because there is no need to complicate the function with gotos to handle
unlocking on failure.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
net/core/devlink.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 620e9d07ac85..6dc14eb2a5f7 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3768,6 +3768,19 @@ static void devlink_nl_region_notify(struct devlink_region *region,
nlmsg_free(msg);
}
+/**
+ * __devlink_region_snapshot_id_get - get snapshot ID
+ * @devlink: devlink instance
+ *
+ * Returns a new snapshot id. Must be called while holding the
+ * devlink instance lock.
+ */
+static u32 __devlink_region_snapshot_id_get(struct devlink *devlink)
+{
+ lockdep_assert_held(&devlink->lock);
+ return ++devlink->snapshot_id;
+}
+
/**
* __devlink_region_snapshot_create - create a new snapshot
* This will add a new snapshot of a region. The snapshot
@@ -7775,7 +7788,7 @@ u32 devlink_region_snapshot_id_get(struct devlink *devlink)
u32 id;
mutex_lock(&devlink->lock);
- id = ++devlink->snapshot_id;
+ id = __devlink_region_snapshot_id_get(devlink);
mutex_unlock(&devlink->lock);
return id;
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 06/10] devlink: convert snapshot id getter to return an error
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (4 preceding siblings ...)
2020-03-24 22:34 ` [PATCH 05/10] devlink: extract snapshot id allocation to helper function Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-25 18:04 ` Jakub Kicinski
2020-03-24 22:34 ` [PATCH] devlink: track snapshot id usage count using an xarray Jacob Keller
` (5 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller, Jiri Pirko
Modify the devlink_snapshot_id_get function to return a signed value,
enabling reporting an error on failure.
This enables easily refactoring how IDs are generated and kept track of
in the future. For now, just report ENOSPC once INT_MAX snapshot ids
have been returned.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx4/crdump.c | 10 +++++++---
drivers/net/netdevsim/dev.c | 7 +++++--
include/net/devlink.h | 2 +-
net/core/devlink.c | 16 +++++++++++-----
4 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c
index c3f90c0f9554..723a66efdf32 100644
--- a/drivers/net/ethernet/mellanox/mlx4/crdump.c
+++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c
@@ -169,7 +169,7 @@ int mlx4_crdump_collect(struct mlx4_dev *dev)
struct pci_dev *pdev = dev->persist->pdev;
unsigned long cr_res_size;
u8 __iomem *cr_space;
- u32 id;
+ int id;
if (!dev->caps.health_buffer_addrs) {
mlx4_info(dev, "crdump: FW doesn't support health buffer access, skipping\n");
@@ -189,10 +189,14 @@ int mlx4_crdump_collect(struct mlx4_dev *dev)
return -ENODEV;
}
- crdump_enable_crspace_access(dev, cr_space);
-
/* Get the available snapshot ID for the dumps */
id = devlink_region_snapshot_id_get(devlink);
+ if (id < 0) {
+ mlx4_err(dev, "crdump: devlink get snapshot id err %d\n", id);
+ return id;
+ }
+
+ crdump_enable_crspace_access(dev, cr_space);
/* Try to capture dumps */
mlx4_crdump_collect_crspace(dev, cr_space, id);
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index f7621ccb7b88..f9420b77e5fd 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -45,8 +45,7 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
{
struct nsim_dev *nsim_dev = file->private_data;
void *dummy_data;
- int err;
- u32 id;
+ int err, id;
dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
if (!dummy_data)
@@ -55,6 +54,10 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
+ if (id < 0) {
+ pr_err("Failed to get snapshot id\n");
+ return id;
+ }
err = devlink_region_snapshot_create(nsim_dev->dummy_region,
dummy_data, id);
if (err) {
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8869ad75b965..df9f6ddf6c66 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -976,7 +976,7 @@ devlink_region_create(struct devlink *devlink,
const struct devlink_region_ops *ops,
u32 region_max_snapshots, u64 region_size);
void devlink_region_destroy(struct devlink_region *region);
-u32 devlink_region_snapshot_id_get(struct devlink *devlink);
+int devlink_region_snapshot_id_get(struct devlink *devlink);
int devlink_region_snapshot_create(struct devlink_region *region,
u8 *data, u32 snapshot_id);
int devlink_info_serial_number_put(struct devlink_info_req *req,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6dc14eb2a5f7..62a8566e9851 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3772,12 +3772,16 @@ static void devlink_nl_region_notify(struct devlink_region *region,
* __devlink_region_snapshot_id_get - get snapshot ID
* @devlink: devlink instance
*
- * Returns a new snapshot id. Must be called while holding the
- * devlink instance lock.
+ * Returns a new snapshot id or a negative error code on failure. Must be
+ * called while holding the devlink instance lock.
*/
-static u32 __devlink_region_snapshot_id_get(struct devlink *devlink)
+static int __devlink_region_snapshot_id_get(struct devlink *devlink)
{
lockdep_assert_held(&devlink->lock);
+
+ if (devlink->snapshot_id >= INT_MAX)
+ return -ENOSPC;
+
return ++devlink->snapshot_id;
}
@@ -7781,11 +7785,13 @@ EXPORT_SYMBOL_GPL(devlink_region_destroy);
* Driver should use the same id for multiple snapshots taken
* on multiple regions at the same time/by the same trigger.
*
+ * Returns a positive id or a negative error code on failure.
+ *
* @devlink: devlink
*/
-u32 devlink_region_snapshot_id_get(struct devlink *devlink)
+int devlink_region_snapshot_id_get(struct devlink *devlink)
{
- u32 id;
+ int id;
mutex_lock(&devlink->lock);
id = __devlink_region_snapshot_id_get(devlink);
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH] devlink: track snapshot id usage count using an xarray
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (5 preceding siblings ...)
2020-03-24 22:34 ` [PATCH 06/10] devlink: convert snapshot id getter to return an error Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-25 16:08 ` Jiri Pirko
2020-03-24 22:34 ` [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (4 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller
Each snapshot created for a devlink region must have an id. These ids
are supposed to be unique per "event" that caused the snapshot to be
created. Drivers call devlink_region_snapshot_id_get to obtain a new id
to use for a new event trigger. The id values are tracked per devlink,
so that the same id number can be used if a triggering event creates
multiple snapshots on different regions.
There is no mechanism for snapshot ids to ever be reused. Introduce an
xarray to store the count of how many snapshots are using a given id,
replacing the snapshot_id field previously used for picking the next id.
The devlink_region_snapshot_id_get() function will use xa_alloc to
insert a zero value at an available slot between 0 and INT_MAX.
The new __devlink_snapshot_id_increment() and
__devlink_snapshot_id_decrement() functions will be used to track how
many snapshots currently use an id.
By tracking the total number of snapshots using a given id, it is
possible for the decrement() function to erase the id from the xarray
when it is not in use.
With this method, a snapshot id can become reused again once all
snapshots that referred to it have been deleted via
DEVLINK_CMD_REGION_DEL.
This work also paves the way to introduce a mechanism for userspace to
request a snapshot.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
include/net/devlink.h | 3 +-
net/core/devlink.c | 119 ++++++++++++++++++++++++++++++++++++++++--
2 files changed, 118 insertions(+), 4 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index df9f6ddf6c66..c366777c4f5c 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -18,6 +18,7 @@
#include <net/net_namespace.h>
#include <net/flow_offload.h>
#include <uapi/linux/devlink.h>
+#include <linux/xarray.h>
struct devlink_ops;
@@ -29,13 +30,13 @@ struct devlink {
struct list_head resource_list;
struct list_head param_list;
struct list_head region_list;
- u32 snapshot_id;
struct list_head reporter_list;
struct mutex reporters_lock; /* protects reporter_list */
struct devlink_dpipe_headers *dpipe_headers;
struct list_head trap_list;
struct list_head trap_group_list;
const struct devlink_ops *ops;
+ struct xarray snapshot_ids;
struct device *dev;
possible_net_t _net;
struct mutex lock;
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 62a8566e9851..b3698228a6ed 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3768,21 +3768,115 @@ static void devlink_nl_region_notify(struct devlink_region *region,
nlmsg_free(msg);
}
+/**
+ * __devlink_snapshot_id_increment - Increment number of snapshots using an id
+ * @devlink: devlink instance
+ * @id: the snapshot id
+ *
+ * Track when a new snapshot begins using an id. Load the count for the
+ * given id from the snapshot xarray, increment it, and store it back.
+ *
+ * Called when a new snapshot is created with the given id.
+ *
+ * The id *must* have been previously allocated by
+ * devlink_region_snapshot_id_get().
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static int __devlink_snapshot_id_increment(struct devlink *devlink, u32 id)
+{
+ unsigned long count;
+ int err;
+ void *p;
+
+ lockdep_assert_held(&devlink->lock);
+
+ p = xa_load(&devlink->snapshot_ids, id);
+ if (!p)
+ return -EEXIST;
+
+ if (!xa_is_value(p))
+ return -EINVAL;
+
+ count = xa_to_value(p);
+ count++;
+
+ err = xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
+ GFP_KERNEL));
+ if (err)
+ return err;
+
+ return 0;
+}
+
+/**
+ * __devlink_snapshot_id_decrement - Decrease number of snapshots using an id
+ * @devlink: devlink instance
+ * @id: the snapshot id
+ *
+ * Track when a snapshot is deleted and stops using an id. Load the count
+ * for the given id from the snapshot xarray, decrement it, and store it
+ * back.
+ *
+ * If the count reaches zero, erase this id from the xarray, freeing it
+ * up for future re-use by devlink_region_snapshot_id_get().
+ *
+ * Called when a snapshot using the given id is deleted.
+ */
+static void __devlink_snapshot_id_decrement(struct devlink *devlink, u32 id)
+{
+ unsigned long count;
+ void *p;
+
+ lockdep_assert_held(&devlink->lock);
+
+ p = xa_load(&devlink->snapshot_ids, id);
+ if (WARN_ON(!p))
+ return;
+
+ if (WARN_ON(!xa_is_value(p)))
+ return;
+
+ count = xa_to_value(p);
+
+ if (count > 1) {
+ count--;
+ xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
+ GFP_KERNEL);
+ } else {
+ /* If this was the last user, we can erase this id */
+ xa_erase(&devlink->snapshot_ids, id);
+ }
+}
+
/**
* __devlink_region_snapshot_id_get - get snapshot ID
* @devlink: devlink instance
*
* Returns a new snapshot id or a negative error code on failure. Must be
* called while holding the devlink instance lock.
+ *
+ * Snapshot IDs are tracked using an xarray which stores the number of
+ * snapshots currently using that index.
+ *
+ * When getting a new id, there are no existing snapshots using it yet,
+ * so the count is initialized at zero. Use the associated increment and
+ * decrement functions when the number of snapshots using an id changes.
*/
static int __devlink_region_snapshot_id_get(struct devlink *devlink)
{
+ int err;
+ u32 id;
+
lockdep_assert_held(&devlink->lock);
- if (devlink->snapshot_id >= INT_MAX)
- return -ENOSPC;
+ /* xa_limit_31b ensures the id will be between 0 and INT_MAX */
+ err = xa_alloc(&devlink->snapshot_ids, &id, xa_mk_value(0),
+ xa_limit_31b, GFP_KERNEL);
+ if (err)
+ return err;
- return ++devlink->snapshot_id;
+ return id;
}
/**
@@ -3805,6 +3899,7 @@ __devlink_region_snapshot_create(struct devlink_region *region,
{
struct devlink *devlink = region->devlink;
struct devlink_snapshot *snapshot;
+ int err;
lockdep_assert_held(&devlink->lock);
@@ -3819,6 +3914,10 @@ __devlink_region_snapshot_create(struct devlink_region *region,
if (!snapshot)
return -ENOMEM;
+ err = __devlink_snapshot_id_increment(devlink, snapshot_id);
+ if (err)
+ goto err_free_snapshot;
+
snapshot->id = snapshot_id;
snapshot->region = region;
snapshot->data = data;
@@ -3829,15 +3928,24 @@ __devlink_region_snapshot_create(struct devlink_region *region,
devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
return 0;
+
+err_free_snapshot:
+ kfree(snapshot);
+ return err;
}
static void devlink_region_snapshot_del(struct devlink_region *region,
struct devlink_snapshot *snapshot)
{
+ struct devlink *devlink = region->devlink;
+
+ lockdep_assert_held(&devlink->lock);
+
devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_DEL);
region->cur_snapshots--;
list_del(&snapshot->list);
region->ops->destructor(snapshot->data);
+ __devlink_snapshot_id_decrement(devlink, snapshot->id);
kfree(snapshot);
}
@@ -6490,6 +6598,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
if (!devlink)
return NULL;
devlink->ops = ops;
+ xa_init(&devlink->snapshot_ids);
__devlink_net_set(devlink, &init_net);
INIT_LIST_HEAD(&devlink->port_list);
INIT_LIST_HEAD(&devlink->sb_list);
@@ -6582,6 +6691,10 @@ EXPORT_SYMBOL_GPL(devlink_reload_disable);
*/
void devlink_free(struct devlink *devlink)
{
+ mutex_lock(&devlink->lock);
+ xa_destroy(&devlink->snapshot_ids);
+ mutex_unlock(&devlink->lock);
+
mutex_destroy(&devlink->reporters_lock);
mutex_destroy(&devlink->lock);
WARN_ON(!list_empty(&devlink->trap_group_list));
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (6 preceding siblings ...)
2020-03-24 22:34 ` [PATCH] devlink: track snapshot id usage count using an xarray Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-25 16:46 ` Jiri Pirko
2020-03-24 22:34 ` [PATCH 09/10] netdevsim: support taking immediate snapshot via devlink Jacob Keller
` (3 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller
Implement support for the DEVLINK_CMD_REGION_NEW command for creating
snapshots. This new command parallels the existing
DEVLINK_CMD_REGION_DEL.
In order for DEVLINK_CMD_REGION_NEW to work for a region, the new
".snapshot" operation must be implemented in the region's ops structure.
The desired snapshot id must be provided. This helps avoid confusion on
the purpose of DEVLINK_CMD_REGION_NEW, and keeps the API simpler.
The requested id will be inserted into the xarray tracking the number of
snapshots using each id. If this id is already used by another snapshot
on any region, an error will be returned.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
.../networking/devlink/devlink-region.rst | 8 ++
include/net/devlink.h | 6 +
net/core/devlink.c | 103 ++++++++++++++++++
3 files changed, 117 insertions(+)
diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
index 8b46e8591fe0..9d2d4c95a5c4 100644
--- a/Documentation/networking/devlink/devlink-region.rst
+++ b/Documentation/networking/devlink/devlink-region.rst
@@ -20,6 +20,11 @@ address regions that are otherwise inaccessible to the user.
Regions may also be used to provide an additional way to debug complex error
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.
+
example usage
-------------
@@ -40,6 +45,9 @@ example usage
# Delete a snapshot using:
$ 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
+
# Dump a snapshot:
$ devlink region dump pci/0000:00:05.0/fw-health snapshot 1
0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 30306e62fe73..ae894e073050 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -502,10 +502,16 @@ struct devlink_info_req;
* struct devlink_region_ops - Region operations
* @name: region name
* @destructor: callback used to free snapshot memory when deleting
+ * @snapshot: callback to request an immediate snapshot. On success,
+ * the data variable must be updated to point to the snapshot data.
+ * The function will be called while the devlink instance lock is
+ * held.
*/
struct devlink_region_ops {
const char *name;
void (*destructor)(const void *data);
+ int (*snapshot)(struct devlink *devlink, struct netlink_ext_ack *extack,
+ u8 **data);
};
struct devlink_fmsg;
diff --git a/net/core/devlink.c b/net/core/devlink.c
index b3698228a6ed..16129ec27913 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3849,6 +3849,35 @@ static void __devlink_snapshot_id_decrement(struct devlink *devlink, u32 id)
}
}
+/**
+ * __devlink_snapshot_id_insert - Insert a specific snapshot ID
+ * @devlink: devlink instance
+ * @id: the snapshot id
+ *
+ * Mark the given snapshot id as used by inserting a zero value into the
+ * snapshot xarray.
+ *
+ * Returns zero on success, or an error code if the snapshot id could not
+ * be inserted.
+ */
+static int __devlink_snapshot_id_insert(struct devlink *devlink, u32 id)
+{
+ int err;
+
+ lockdep_assert_held(&devlink->lock);
+
+ /* Check to make sure it's empty first */
+ if (xa_load(&devlink->snapshot_ids, id))
+ return -EBUSY;
+
+ err = xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(0),
+ GFP_KERNEL));
+ if (err)
+ return err;
+
+ return 0;
+}
+
/**
* __devlink_region_snapshot_id_get - get snapshot ID
* @devlink: devlink instance
@@ -4048,6 +4077,73 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
return 0;
}
+static int
+devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
+{
+ struct devlink *devlink = info->user_ptr[0];
+ struct devlink_region *region;
+ const char *region_name;
+ u32 snapshot_id;
+ u8 *data;
+ int err;
+
+ if (!info->attrs[DEVLINK_ATTR_REGION_NAME]) {
+ NL_SET_ERR_MSG_MOD(info->extack, "No region name provided");
+ 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) {
+ NL_SET_ERR_MSG_MOD(info->extack, "The requested region does not exist");
+ return -EINVAL;
+ }
+
+ if (!region->ops->snapshot) {
+ NL_SET_ERR_MSG_MOD(info->extack, "The requested region does not support taking an immediate snapshot");
+ return -EOPNOTSUPP;
+ }
+
+ if (region->cur_snapshots == region->max_snapshots) {
+ NL_SET_ERR_MSG_MOD(info->extack, "The region has reached the maximum number of stored snapshots");
+ return -ENOMEM;
+ }
+
+ snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
+
+ 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) {
+ NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in used");
+ return err;
+ }
+
+ err = region->ops->snapshot(devlink, info->extack, &data);
+ if (err)
+ goto err_decrement_snapshot_count;
+
+ err = __devlink_region_snapshot_create(region, data, snapshot_id);
+ if (err)
+ goto err_free_snapshot_data;
+
+ return 0;
+
+err_decrement_snapshot_count:
+ __devlink_snapshot_id_decrement(devlink, snapshot_id);
+err_free_snapshot_data:
+ region->ops->destructor(data);
+ return err;
+}
+
static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
struct devlink *devlink,
u8 *chunk, u32 chunk_size,
@@ -6455,6 +6551,13 @@ static const struct genl_ops devlink_nl_ops[] = {
.flags = GENL_ADMIN_PERM,
.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
},
+ {
+ .cmd = DEVLINK_CMD_REGION_NEW,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+ .doit = devlink_nl_cmd_region_new,
+ .flags = GENL_ADMIN_PERM,
+ .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+ },
{
.cmd = DEVLINK_CMD_REGION_DEL,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 09/10] netdevsim: support taking immediate snapshot via devlink
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (7 preceding siblings ...)
2020-03-24 22:34 ` [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-25 16:50 ` Jiri Pirko
2020-03-24 22:34 ` [PATCH 10/10] ice: add a devlink region for dumping NVM contents Jacob Keller
` (2 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller
Implement the .snapshot region operation for the dummy data region. This
enables a region snapshot to be taken upon request via the new
DEVLINK_CMD_REGION_SNAPSHOT command.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/netdevsim/dev.c | 27 +++++++++++++++----
.../drivers/net/netdevsim/devlink.sh | 15 +++++++++++
2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index f9420b77e5fd..876efe71efff 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -39,13 +39,11 @@ static struct dentry *nsim_dev_ddir;
#define NSIM_DEV_DUMMY_REGION_SIZE (1024 * 32)
-static ssize_t nsim_dev_take_snapshot_write(struct file *file,
- const char __user *data,
- size_t count, loff_t *ppos)
+static int
+nsim_dev_take_snapshot(struct devlink *devlink, struct netlink_ext_ack *extack,
+ u8 **data)
{
- struct nsim_dev *nsim_dev = file->private_data;
void *dummy_data;
- int err, id;
dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
if (!dummy_data)
@@ -53,6 +51,24 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
+ *data = dummy_data;
+
+ return 0;
+}
+
+static ssize_t nsim_dev_take_snapshot_write(struct file *file,
+ const char __user *data,
+ size_t count, loff_t *ppos)
+{
+ struct nsim_dev *nsim_dev = file->private_data;
+ u8 *dummy_data;
+ int err, id;
+
+ err = nsim_dev_take_snapshot(priv_to_devlink(nsim_dev), NULL,
+ &dummy_data);
+ if (err)
+ return err;
+
id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
if (id < 0) {
pr_err("Failed to get snapshot id\n");
@@ -346,6 +362,7 @@ static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink)
static const struct devlink_region_ops dummy_region_ops = {
.name = "dummy",
.destructor = &kfree,
+ .snapshot = nsim_dev_take_snapshot,
};
static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
index 025a84c2ab5a..f23383fd108c 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
@@ -141,6 +141,21 @@ regions_test()
check_region_snapshot_count dummy post-first-delete 2
+ devlink region new $DL_HANDLE/dummy
+ check_err $? "Failed to create a new a snapshot"
+
+ check_region_snapshot_count dummy post-request 3
+
+ devlink region new $DL_HANDLE/dummy snapshot 25
+ check_err $? "Failed to create a new snapshot with id 25"
+
+ check_region_snapshot_count dummy post-request 4
+
+ devlink region del $DL_HANDLE/dummy snapshot 25
+ check_err $? "Failed to delete snapshot with id 25"
+
+ check_region_snapshot_count dummy post-request 3
+
log_test "regions test"
}
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 10/10] ice: add a devlink region for dumping NVM contents
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (8 preceding siblings ...)
2020-03-24 22:34 ` [PATCH 09/10] netdevsim: support taking immediate snapshot via devlink Jacob Keller
@ 2020-03-24 22:34 ` Jacob Keller
2020-03-25 17:18 ` Jiri Pirko
2020-03-25 13:49 ` [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jiri Pirko
2020-03-25 13:50 ` Jiri Pirko
11 siblings, 1 reply; 29+ messages in thread
From: Jacob Keller @ 2020-03-24 22:34 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Jacob Keller
Add a devlink region for exposing the device's Non Volatime Memory flash
contents.
Support the recently added .snapshot operation, enabling userspace to
request a snapshot of the NVM contents via DEVLINK_CMD_REGION_NEW.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Documentation/networking/devlink/ice.rst | 26 +++++
drivers/net/ethernet/intel/ice/ice.h | 2 +
drivers/net/ethernet/intel/ice/ice_devlink.c | 99 ++++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_devlink.h | 3 +
drivers/net/ethernet/intel/ice/ice_main.c | 4 +
5 files changed, 134 insertions(+)
diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index 37fbbd40a5e5..f3d6a3b50342 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -69,3 +69,29 @@ The ``ice`` driver reports the following versions
- The version of the DDP package that is active in the device. Note
that both the name (as reported by ``fw.app.name``) and version are
required to uniquely identify the package.
+
+Regions
+=======
+
+The ``ice`` driver enables access to the contents of the Non Volatile Memory
+flash chip via the ``nvm-flash`` region.
+
+Users can request an immediate capture of a snapshot via the
+``DEVLINK_CMD_REGION_NEW``
+
+.. code:: shell
+
+ $ devlink region new pci/0000:01:00.0/nvm-flash snapshot 1
+ $ devlink region dump pci/0000:01:00.0/nvm-flash snapshot 1
+
+ $ devlink region dump pci/0000:01:00.0/nvm-flash snapshot 1
+ 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
+ 0000000000000010 0000 0000 ffff ff04 0029 8c00 0028 8cc8
+ 0000000000000020 0016 0bb8 0016 1720 0000 0000 c00f 3ffc
+ 0000000000000030 bada cce5 bada cce5 bada cce5 bada cce5
+
+ $ devlink region read pci/0000:01:00.0/nvm-flash snapshot 1 address 0
+ length 16
+ 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
+
+ $ devlink region delete pci/0000:01:00.0/nvm-flash snapshot 1
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 8ce3afcfeca0..5c11448bfbb3 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -351,6 +351,8 @@ struct ice_pf {
/* devlink port data */
struct devlink_port devlink_port;
+ struct devlink_region *nvm_region;
+
/* OS reserved IRQ details */
struct msix_entry *msix_entries;
struct ice_res_tracker *irq_tracker;
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 27c5034c039a..9dd1b21820bf 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -318,3 +318,102 @@ void ice_devlink_destroy_port(struct ice_pf *pf)
devlink_port_type_clear(&pf->devlink_port);
devlink_port_unregister(&pf->devlink_port);
}
+
+/**
+ * ice_devlink_nvm_snapshot - Capture a snapshot of the Shadow RAM contents
+ * @devlink: the devlink instance
+ * @extack: extended ACK response structure
+ * @data: on exit points to snapshot data buffer
+ *
+ * This function is called in response to the DEVLINK_CMD_REGION_TRIGGER for
+ * the shadow-ram devlink region. It captures a snapshot of the shadow ram
+ * contents. This snapshot can later be viewed via the devlink-region
+ * interface.
+ *
+ * @returns zero on success, and updates the data pointer. Returns a non-zero
+ * error code on failure.
+ */
+static int
+ice_devlink_nvm_snapshot(struct devlink *devlink, struct netlink_ext_ack *extack,
+ u8 **data)
+{
+ struct ice_pf *pf = devlink_priv(devlink);
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_hw *hw = &pf->hw;
+ enum ice_status status;
+ void *nvm_data;
+ u32 nvm_size;
+
+ nvm_size = hw->nvm.flash_size;
+ nvm_data = vzalloc(nvm_size);
+ if (!nvm_data) {
+ NL_SET_ERR_MSG_MOD(extack, "Out of memory");
+ return -ENOMEM;
+ }
+
+ status = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (status) {
+ dev_dbg(dev, "ice_acquire_nvm failed, err %d aq_err %d\n",
+ status, hw->adminq.sq_last_status);
+ NL_SET_ERR_MSG_MOD(extack, "Failed to acquire NVM semaphore");
+ vfree(nvm_data);
+ return -EIO;
+ }
+
+ status = ice_read_flat_nvm(hw, 0, &nvm_size, nvm_data, false);
+ if (status) {
+ dev_dbg(dev, "ice_read_flat_nvm failed after reading %u bytes, err %d aq_err %d\n",
+ nvm_size, status, hw->adminq.sq_last_status);
+ NL_SET_ERR_MSG_MOD(extack, "Failed to read NVM contents");
+ ice_release_nvm(hw);
+ vfree(nvm_data);
+ return -EIO;
+ }
+
+ ice_release_nvm(hw);
+
+ *data = nvm_data;
+
+ return 0;
+}
+
+static const struct devlink_region_ops ice_nvm_region_ops = {
+ .name = "nvm-flash",
+ .destructor = vfree,
+ .snapshot = ice_devlink_nvm_snapshot,
+};
+
+/**
+ * ice_devlink_init_regions - Initialize devlink regions
+ * @pf: the PF device structure
+ *
+ * Create devlink regions used to enable access to dump the contents of the
+ * flash memory on the device.
+ */
+void ice_devlink_init_regions(struct ice_pf *pf)
+{
+ struct devlink *devlink = priv_to_devlink(pf);
+ struct device *dev = ice_pf_to_dev(pf);
+ u64 nvm_size;
+
+ nvm_size = pf->hw.nvm.flash_size;
+ pf->nvm_region = devlink_region_create(devlink, &ice_nvm_region_ops, 1,
+ nvm_size);
+ if (IS_ERR(pf->nvm_region)) {
+ dev_warn(dev, "failed to create NVM devlink region, err %ld\n",
+ PTR_ERR(pf->nvm_region));
+ pf->nvm_region = NULL;
+ }
+}
+
+/**
+ * ice_devlink_destroy_regions - Destroy devlink regions
+ * @pf: the PF device structure
+ *
+ * Remove previously created regions for this PF.
+ */
+void ice_devlink_destroy_regions(struct ice_pf *pf)
+{
+ if (pf->nvm_region)
+ devlink_region_destroy(pf->nvm_region);
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.h b/drivers/net/ethernet/intel/ice/ice_devlink.h
index f94dc93c24c5..6e806a08dc23 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.h
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.h
@@ -11,4 +11,7 @@ void ice_devlink_unregister(struct ice_pf *pf);
int ice_devlink_create_port(struct ice_pf *pf);
void ice_devlink_destroy_port(struct ice_pf *pf);
+void ice_devlink_init_regions(struct ice_pf *pf);
+void ice_devlink_destroy_regions(struct ice_pf *pf);
+
#endif /* _ICE_DEVLINK_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 359ff8544773..306a4e5b2320 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3276,6 +3276,8 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
goto err_init_pf_unroll;
}
+ ice_devlink_init_regions(pf);
+
pf->num_alloc_vsi = hw->func_caps.guar_num_vsi;
if (!pf->num_alloc_vsi) {
err = -EIO;
@@ -3385,6 +3387,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
devm_kfree(dev, pf->vsi);
err_init_pf_unroll:
ice_deinit_pf(pf);
+ ice_devlink_destroy_regions(pf);
ice_deinit_hw(hw);
err_exit_unroll:
ice_devlink_unregister(pf);
@@ -3427,6 +3430,7 @@ static void ice_remove(struct pci_dev *pdev)
ice_vsi_free_q_vectors(pf->vsi[i]);
}
ice_deinit_pf(pf);
+ ice_devlink_destroy_regions(pf);
ice_deinit_hw(&pf->hw);
ice_devlink_unregister(pf);
--
2.24.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (9 preceding siblings ...)
2020-03-24 22:34 ` [PATCH 10/10] ice: add a devlink region for dumping NVM contents Jacob Keller
@ 2020-03-25 13:49 ` Jiri Pirko
2020-03-25 13:50 ` Jiri Pirko
11 siblings, 0 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 13:49 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jakub Kicinski
Tue, Mar 24, 2020 at 11:34:35PM CET, jacob.e.keller@intel.com wrote:
>This series adds support for the DEVLINK_CMD_REGION_NEW operation, used to
>enable userspace requesting a snapshot of a region on demand.
>
>This can be useful to enable adding regions for a driver for which there is
>no trigger to create snapshots. By making this a core part of devlink, there
>is no need for the drivers to use a separate channel such as debugfs.
>
>The primary intent for this kind of region is to expose device information
>that might be useful for diagnostics and information gathering.
>
>The first few patches refactor regions to support a new ops structure for
>extending the available operations that regions can perform. This includes
>converting the destructor into an op from a function argument.
>
>Next, patches refactor the snapshot id allocation to use an xarray which
>tracks the number of current snapshots using a given id. This is done so
>that id lifetime can be determined, and ids can be released when no longer
>in use.
>
>Without this change, snapshot ids remain used forever, until the snapshot_id
>count rolled over UINT_MAX.
>
>Finally, code to enable the previously unused DEVLINK_CMD_REGION_NEW is
>added. This code enforces that the snapshot id is always provided, unlike
>previous revisions of this series.
>
>Finally, a patch is added to enable using this new command via the .snapshot
>callback in both netdevsim and the ice driver.
>
>For the ice driver, a new "nvm-flash" region is added, which will enable
>read access to the NVM flash contents. The intention for this is to allow
>diagnostics tools to gather information about the device. By using a
>snapshot and gathering the NVM contents all at once, the contents can be
>atomic.
>
>General changes since the v2 RFC:
>* Use an xarray instead of IDRs
>* Rebase onto net-next now that the initial ice devlink patches landed.
>
>Patch specific changes:
>* devlink: add functions to take snapshot while locked
>
> Split this into two patches, so that an explanation of why the
> devlink_region_snapshot_id_get is still extracted, even though only one
> caller will remain.
It is much more convenient for reviewing purposes to have the changelog
per-patch.
>
>* devlink: track snapshot ids using an IDR and refcounts
>
> Convert to using an xarray storing the total number of snapshots directly,
> rather than an IDR with a refcount structure. This significantly
> simplifies the code, and avoids the complication of a NULL refcount.
>
>* devlink: implement DEVLINK_CMD_REGION_NEW
>
> As suggested by Jiri, remove the ability for DEVLINK_CMD_REGION_NEW to
> dynamically generate IDs. Instead, always require a snapshot id. This
> aligns with DEVLINK_CMD_REGION_DEL, and helps reduce confusion.
>
> Refactor this patch to use the xarray instead of the IDR, as in the
> previous patch.
>
> Clean up and remove unnecessary new lines on NL_SET_ERR_MSG_MOD
>
>* ice: add a devlink region to dump shadow RAM contents
>
> Remove the code for immediate region read, as this will be worked on in a
> separate series following this one.
>
>Jacob Keller (10):
> devlink: prepare to support region operations
> devlink: convert snapshot destructor callback to region op
> devlink: trivial: fix tab in function documentation
> devlink: add function to take snapshot while locked
> devlink: extract snapshot id allocation to helper function
> devlink: convert snapshot id getter to return an error
> devlink: track snapshot id usage count using an xarray
> devlink: implement DEVLINK_CMD_REGION_NEW
> netdevsim: support taking immediate snapshot via devlink
> ice: add a devlink region for dumping NVM contents
>
> .../networking/devlink/devlink-region.rst | 8 +
> Documentation/networking/devlink/ice.rst | 26 ++
> drivers/net/ethernet/intel/ice/ice.h | 2 +
> drivers/net/ethernet/intel/ice/ice_devlink.c | 99 +++++
> drivers/net/ethernet/intel/ice/ice_devlink.h | 3 +
> drivers/net/ethernet/intel/ice/ice_main.c | 4 +
> drivers/net/ethernet/mellanox/mlx4/crdump.c | 32 +-
> drivers/net/netdevsim/dev.c | 41 +-
> include/net/devlink.h | 31 +-
> net/core/devlink.c | 354 +++++++++++++++---
> .../drivers/net/netdevsim/devlink.sh | 15 +
> 11 files changed, 538 insertions(+), 77 deletions(-)
>
>--
>2.24.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
` (10 preceding siblings ...)
2020-03-25 13:49 ` [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jiri Pirko
@ 2020-03-25 13:50 ` Jiri Pirko
11 siblings, 0 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 13:50 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jakub Kicinski
Re subject. You should indicate what tree you are aiming at:
[patch net-next]
[patch net]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] devlink: extract snapshot id allocation to helper function
2020-03-24 22:34 ` [PATCH 05/10] devlink: extract snapshot id allocation to helper function Jacob Keller
@ 2020-03-25 14:08 ` Jiri Pirko
0 siblings, 0 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 14:08 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jakub Kicinski
Tue, Mar 24, 2020 at 11:34:40PM CET, jacob.e.keller@intel.com wrote:
>A future change is going to implement a new devlink command to request
>a snapshot on demand. As part of this, the logic for handling the
>snapshot ids will be refactored. To simplify the snapshot id allocation
>function, move it to a separate function prefixed by `__`. This helper
>function will assume the lock is held.
>
>While no other callers will exist, it simplifies refactoring the logic
>because there is no need to complicate the function with gotos to handle
>unlocking on failure.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] devlink: track snapshot id usage count using an xarray
2020-03-24 22:34 ` [PATCH] devlink: track snapshot id usage count using an xarray Jacob Keller
@ 2020-03-25 16:08 ` Jiri Pirko
2020-03-26 1:16 ` Jacob Keller
2020-03-26 1:43 ` Jacob Keller
0 siblings, 2 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 16:08 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jakub Kicinski
You somehow missed "07/10" from the subject :O
Tue, Mar 24, 2020 at 11:34:42PM CET, jacob.e.keller@intel.com wrote:
>Each snapshot created for a devlink region must have an id. These ids
>are supposed to be unique per "event" that caused the snapshot to be
>created. Drivers call devlink_region_snapshot_id_get to obtain a new id
>to use for a new event trigger. The id values are tracked per devlink,
>so that the same id number can be used if a triggering event creates
>multiple snapshots on different regions.
>
>There is no mechanism for snapshot ids to ever be reused. Introduce an
>xarray to store the count of how many snapshots are using a given id,
>replacing the snapshot_id field previously used for picking the next id.
>
>The devlink_region_snapshot_id_get() function will use xa_alloc to
>insert a zero value at an available slot between 0 and INT_MAX.
>
>The new __devlink_snapshot_id_increment() and
>__devlink_snapshot_id_decrement() functions will be used to track how
>many snapshots currently use an id.
>
>By tracking the total number of snapshots using a given id, it is
>possible for the decrement() function to erase the id from the xarray
>when it is not in use.
>
>With this method, a snapshot id can become reused again once all
>snapshots that referred to it have been deleted via
>DEVLINK_CMD_REGION_DEL.
>
>This work also paves the way to introduce a mechanism for userspace to
>request a snapshot.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
> include/net/devlink.h | 3 +-
> net/core/devlink.c | 119 ++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 118 insertions(+), 4 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index df9f6ddf6c66..c366777c4f5c 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -18,6 +18,7 @@
> #include <net/net_namespace.h>
> #include <net/flow_offload.h>
> #include <uapi/linux/devlink.h>
>+#include <linux/xarray.h>
>
> struct devlink_ops;
>
>@@ -29,13 +30,13 @@ struct devlink {
> struct list_head resource_list;
> struct list_head param_list;
> struct list_head region_list;
>- u32 snapshot_id;
> struct list_head reporter_list;
> struct mutex reporters_lock; /* protects reporter_list */
> struct devlink_dpipe_headers *dpipe_headers;
> struct list_head trap_list;
> struct list_head trap_group_list;
> const struct devlink_ops *ops;
>+ struct xarray snapshot_ids;
> struct device *dev;
> possible_net_t _net;
> struct mutex lock;
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 62a8566e9851..b3698228a6ed 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3768,21 +3768,115 @@ static void devlink_nl_region_notify(struct devlink_region *region,
> nlmsg_free(msg);
> }
>
>+/**
>+ * __devlink_snapshot_id_increment - Increment number of snapshots using an id
>+ * @devlink: devlink instance
>+ * @id: the snapshot id
>+ *
>+ * Track when a new snapshot begins using an id. Load the count for the
>+ * given id from the snapshot xarray, increment it, and store it back.
>+ *
>+ * Called when a new snapshot is created with the given id.
>+ *
>+ * The id *must* have been previously allocated by
>+ * devlink_region_snapshot_id_get().
>+ *
>+ * Returns 0 on success, or an error on failure.
>+ */
>+static int __devlink_snapshot_id_increment(struct devlink *devlink, u32 id)
>+{
>+ unsigned long count;
>+ int err;
>+ void *p;
>+
>+ lockdep_assert_held(&devlink->lock);
>+
>+ p = xa_load(&devlink->snapshot_ids, id);
>+ if (!p)
>+ return -EEXIST;
This is confusing. You should return rather -ENOTEXIST, if it existed :)
-EINVAL and WARN_ON. This should never happen
>+
>+ if (!xa_is_value(p))
>+ return -EINVAL;
>+
>+ count = xa_to_value(p);
>+ count++;
>+
>+ err = xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
>+ GFP_KERNEL));
Just return here and remove err variable.
>+ if (err)
>+ return err;
>+
>+ return 0;
>+}
>+
>+/**
>+ * __devlink_snapshot_id_decrement - Decrease number of snapshots using an id
>+ * @devlink: devlink instance
>+ * @id: the snapshot id
>+ *
>+ * Track when a snapshot is deleted and stops using an id. Load the count
>+ * for the given id from the snapshot xarray, decrement it, and store it
>+ * back.
>+ *
>+ * If the count reaches zero, erase this id from the xarray, freeing it
>+ * up for future re-use by devlink_region_snapshot_id_get().
>+ *
>+ * Called when a snapshot using the given id is deleted.
>+ */
>+static void __devlink_snapshot_id_decrement(struct devlink *devlink, u32 id)
>+{
>+ unsigned long count;
>+ void *p;
>+
>+ lockdep_assert_held(&devlink->lock);
>+
>+ p = xa_load(&devlink->snapshot_ids, id);
>+ if (WARN_ON(!p))
>+ return;
>+
>+ if (WARN_ON(!xa_is_value(p)))
>+ return;
>+
>+ count = xa_to_value(p);
>+
>+ if (count > 1) {
>+ count--;
>+ xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
>+ GFP_KERNEL);
>+ } else {
>+ /* If this was the last user, we can erase this id */
>+ xa_erase(&devlink->snapshot_ids, id);
>+ }
>+}
>+
> /**
> * __devlink_region_snapshot_id_get - get snapshot ID
> * @devlink: devlink instance
> *
> * Returns a new snapshot id or a negative error code on failure. Must be
> * called while holding the devlink instance lock.
>+ *
>+ * Snapshot IDs are tracked using an xarray which stores the number of
>+ * snapshots currently using that index.
>+ *
>+ * When getting a new id, there are no existing snapshots using it yet,
>+ * so the count is initialized at zero. Use the associated increment and
>+ * decrement functions when the number of snapshots using an id changes.
> */
> static int __devlink_region_snapshot_id_get(struct devlink *devlink)
> {
>+ int err;
>+ u32 id;
>+
> lockdep_assert_held(&devlink->lock);
>
>- if (devlink->snapshot_id >= INT_MAX)
>- return -ENOSPC;
>+ /* xa_limit_31b ensures the id will be between 0 and INT_MAX */
Well, currently the snapshot_id is u32. Even the netlink attr is u32.
I believe we should not limit it here.
Please have this as xa_limit_32b.
>+ err = xa_alloc(&devlink->snapshot_ids, &id, xa_mk_value(0),
>+ xa_limit_31b, GFP_KERNEL);
>+ if (err)
>+ return err;
>
>- return ++devlink->snapshot_id;
>+ return id;
> }
>
> /**
>@@ -3805,6 +3899,7 @@ __devlink_region_snapshot_create(struct devlink_region *region,
> {
> struct devlink *devlink = region->devlink;
> struct devlink_snapshot *snapshot;
>+ int err;
>
> lockdep_assert_held(&devlink->lock);
>
>@@ -3819,6 +3914,10 @@ __devlink_region_snapshot_create(struct devlink_region *region,
> if (!snapshot)
> return -ENOMEM;
>
>+ err = __devlink_snapshot_id_increment(devlink, snapshot_id);
>+ if (err)
>+ goto err_free_snapshot;
>+
> snapshot->id = snapshot_id;
> snapshot->region = region;
> snapshot->data = data;
>@@ -3829,15 +3928,24 @@ __devlink_region_snapshot_create(struct devlink_region *region,
>
> devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
> return 0;
>+
>+err_free_snapshot:
>+ kfree(snapshot);
>+ return err;
> }
>
> static void devlink_region_snapshot_del(struct devlink_region *region,
> struct devlink_snapshot *snapshot)
> {
>+ struct devlink *devlink = region->devlink;
>+
>+ lockdep_assert_held(&devlink->lock);
>+
> devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_DEL);
> region->cur_snapshots--;
> list_del(&snapshot->list);
> region->ops->destructor(snapshot->data);
>+ __devlink_snapshot_id_decrement(devlink, snapshot->id);
> kfree(snapshot);
> }
>
>@@ -6490,6 +6598,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
> if (!devlink)
> return NULL;
> devlink->ops = ops;
>+ xa_init(&devlink->snapshot_ids);
> __devlink_net_set(devlink, &init_net);
> INIT_LIST_HEAD(&devlink->port_list);
> INIT_LIST_HEAD(&devlink->sb_list);
>@@ -6582,6 +6691,10 @@ EXPORT_SYMBOL_GPL(devlink_reload_disable);
> */
> void devlink_free(struct devlink *devlink)
> {
>+ mutex_lock(&devlink->lock);
>+ xa_destroy(&devlink->snapshot_ids);
>+ mutex_unlock(&devlink->lock);
I don't follow, why are you locking this?
>+
> mutex_destroy(&devlink->reporters_lock);
> mutex_destroy(&devlink->lock);
> WARN_ON(!list_empty(&devlink->trap_group_list));
>--
>2.24.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW
2020-03-24 22:34 ` [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW Jacob Keller
@ 2020-03-25 16:46 ` Jiri Pirko
2020-03-25 17:18 ` Jakub Kicinski
2020-03-26 1:30 ` Jacob Keller
0 siblings, 2 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 16:46 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jakub Kicinski
Tue, Mar 24, 2020 at 11:34:43PM CET, jacob.e.keller@intel.com wrote:
>Implement support for the DEVLINK_CMD_REGION_NEW command for creating
>snapshots. This new command parallels the existing
>DEVLINK_CMD_REGION_DEL.
>
>In order for DEVLINK_CMD_REGION_NEW to work for a region, the new
>".snapshot" operation must be implemented in the region's ops structure.
>
>The desired snapshot id must be provided. This helps avoid confusion on
>the purpose of DEVLINK_CMD_REGION_NEW, and keeps the API simpler.
>
>The requested id will be inserted into the xarray tracking the number of
>snapshots using each id. If this id is already used by another snapshot
>on any region, an error will be returned.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
> .../networking/devlink/devlink-region.rst | 8 ++
> include/net/devlink.h | 6 +
> net/core/devlink.c | 103 ++++++++++++++++++
> 3 files changed, 117 insertions(+)
>
>diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
>index 8b46e8591fe0..9d2d4c95a5c4 100644
>--- a/Documentation/networking/devlink/devlink-region.rst
>+++ b/Documentation/networking/devlink/devlink-region.rst
>@@ -20,6 +20,11 @@ address regions that are otherwise inaccessible to the user.
> Regions may also be used to provide an additional way to debug complex error
> 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.
>+
> example usage
> -------------
>
>@@ -40,6 +45,9 @@ example usage
> # Delete a snapshot using:
> $ 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
>+
> # Dump a snapshot:
> $ devlink region dump pci/0000:00:05.0/fw-health snapshot 1
> 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 30306e62fe73..ae894e073050 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -502,10 +502,16 @@ struct devlink_info_req;
> * struct devlink_region_ops - Region operations
> * @name: region name
> * @destructor: callback used to free snapshot memory when deleting
>+ * @snapshot: callback to request an immediate snapshot. On success,
>+ * the data variable must be updated to point to the snapshot data.
>+ * The function will be called while the devlink instance lock is
>+ * held.
> */
> struct devlink_region_ops {
> const char *name;
> void (*destructor)(const void *data);
>+ int (*snapshot)(struct devlink *devlink, struct netlink_ext_ack *extack,
>+ u8 **data);
> };
>
> struct devlink_fmsg;
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index b3698228a6ed..16129ec27913 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3849,6 +3849,35 @@ static void __devlink_snapshot_id_decrement(struct devlink *devlink, u32 id)
> }
> }
>
>+/**
>+ * __devlink_snapshot_id_insert - Insert a specific snapshot ID
>+ * @devlink: devlink instance
>+ * @id: the snapshot id
>+ *
>+ * Mark the given snapshot id as used by inserting a zero value into the
>+ * snapshot xarray.
>+ *
>+ * Returns zero on success, or an error code if the snapshot id could not
>+ * be inserted.
>+ */
>+static int __devlink_snapshot_id_insert(struct devlink *devlink, u32 id)
>+{
>+ int err;
>+
>+ lockdep_assert_held(&devlink->lock);
>+
>+ /* Check to make sure it's empty first */
>+ if (xa_load(&devlink->snapshot_ids, id))
How this can happen? The entry was just allocated. WARN_ON.
>+ return -EBUSY;
>+
>+ err = xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(0),
>+ GFP_KERNEL));
Just return and avoid err variable.
>+ if (err)
>+ return err;
>+
>+ return 0;
>+}
>+
> /**
> * __devlink_region_snapshot_id_get - get snapshot ID
> * @devlink: devlink instance
>@@ -4048,6 +4077,73 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
> return 0;
> }
>
>+static int
>+devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct devlink *devlink = info->user_ptr[0];
>+ struct devlink_region *region;
>+ const char *region_name;
>+ u32 snapshot_id;
>+ u8 *data;
>+ int err;
>+
>+ if (!info->attrs[DEVLINK_ATTR_REGION_NAME]) {
>+ NL_SET_ERR_MSG_MOD(info->extack, "No region name provided");
>+ 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) {
>+ NL_SET_ERR_MSG_MOD(info->extack, "The requested region does not exist");
>+ return -EINVAL;
>+ }
>+
>+ if (!region->ops->snapshot) {
>+ NL_SET_ERR_MSG_MOD(info->extack, "The requested region does not support taking an immediate snapshot");
>+ return -EOPNOTSUPP;
>+ }
>+
>+ if (region->cur_snapshots == region->max_snapshots) {
>+ NL_SET_ERR_MSG_MOD(info->extack, "The region has reached the maximum number of stored snapshots");
>+ return -ENOMEM;
Maybe ENOBUFS or ENOSPC? ENOMEM seems odd as it is related to memory
allocation fails which this is not.
>+ }
>+
>+ snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
>+
>+ 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) {
>+ NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in used");
Different message would be appropriate.
>+ return err;
>+ }
>+
>+ err = region->ops->snapshot(devlink, info->extack, &data);
>+ if (err)
>+ goto err_decrement_snapshot_count;
>+
>+ err = __devlink_region_snapshot_create(region, data, snapshot_id);
>+ if (err)
>+ goto err_free_snapshot_data;
>+
>+ return 0;
>+
>+err_decrement_snapshot_count:
>+ __devlink_snapshot_id_decrement(devlink, snapshot_id);
>+err_free_snapshot_data:
In devlink the error labers are named according to actions that failed.
Please align.
>+ region->ops->destructor(data);
>+ return err;
>+}
>+
> static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
> struct devlink *devlink,
> u8 *chunk, u32 chunk_size,
>@@ -6455,6 +6551,13 @@ static const struct genl_ops devlink_nl_ops[] = {
> .flags = GENL_ADMIN_PERM,
> .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
> },
>+ {
>+ .cmd = DEVLINK_CMD_REGION_NEW,
>+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>+ .doit = devlink_nl_cmd_region_new,
>+ .flags = GENL_ADMIN_PERM,
>+ .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>+ },
> {
> .cmd = DEVLINK_CMD_REGION_DEL,
> .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>--
>2.24.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] netdevsim: support taking immediate snapshot via devlink
2020-03-24 22:34 ` [PATCH 09/10] netdevsim: support taking immediate snapshot via devlink Jacob Keller
@ 2020-03-25 16:50 ` Jiri Pirko
0 siblings, 0 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 16:50 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jakub Kicinski
Tue, Mar 24, 2020 at 11:34:44PM CET, jacob.e.keller@intel.com wrote:
>Implement the .snapshot region operation for the dummy data region. This
>enables a region snapshot to be taken upon request via the new
>DEVLINK_CMD_REGION_SNAPSHOT command.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
> drivers/net/netdevsim/dev.c | 27 +++++++++++++++----
> .../drivers/net/netdevsim/devlink.sh | 15 +++++++++++
> 2 files changed, 37 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>index f9420b77e5fd..876efe71efff 100644
>--- a/drivers/net/netdevsim/dev.c
>+++ b/drivers/net/netdevsim/dev.c
>@@ -39,13 +39,11 @@ static struct dentry *nsim_dev_ddir;
>
> #define NSIM_DEV_DUMMY_REGION_SIZE (1024 * 32)
>
>-static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>- const char __user *data,
>- size_t count, loff_t *ppos)
>+static int
>+nsim_dev_take_snapshot(struct devlink *devlink, struct netlink_ext_ack *extack,
>+ u8 **data)
> {
>- struct nsim_dev *nsim_dev = file->private_data;
> void *dummy_data;
>- int err, id;
>
> dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
> if (!dummy_data)
>@@ -53,6 +51,24 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>
> get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
>
>+ *data = dummy_data;
>+
>+ return 0;
>+}
>+
>+static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>+ const char __user *data,
>+ size_t count, loff_t *ppos)
>+{
>+ struct nsim_dev *nsim_dev = file->private_data;
>+ u8 *dummy_data;
>+ int err, id;
>+
>+ err = nsim_dev_take_snapshot(priv_to_devlink(nsim_dev), NULL,
>+ &dummy_data);
>+ if (err)
>+ return err;
>+
> id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
> if (id < 0) {
> pr_err("Failed to get snapshot id\n");
>@@ -346,6 +362,7 @@ static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink)
> static const struct devlink_region_ops dummy_region_ops = {
> .name = "dummy",
> .destructor = &kfree,
>+ .snapshot = nsim_dev_take_snapshot,
> };
>
> static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
>diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>index 025a84c2ab5a..f23383fd108c 100755
>--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>@@ -141,6 +141,21 @@ regions_test()
>
> check_region_snapshot_count dummy post-first-delete 2
>
>+ devlink region new $DL_HANDLE/dummy
Looks like you haven't run the selftest with the current patchset
version.
>+ check_err $? "Failed to create a new a snapshot"
>+
>+ check_region_snapshot_count dummy post-request 3
>+
>+ devlink region new $DL_HANDLE/dummy snapshot 25
>+ check_err $? "Failed to create a new snapshot with id 25"
>+
>+ check_region_snapshot_count dummy post-request 4
>+
>+ devlink region del $DL_HANDLE/dummy snapshot 25
>+ check_err $? "Failed to delete snapshot with id 25"
>+
>+ check_region_snapshot_count dummy post-request 3
>+
> log_test "regions test"
> }
>
>--
>2.24.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW
2020-03-25 16:46 ` Jiri Pirko
@ 2020-03-25 17:18 ` Jakub Kicinski
2020-03-25 17:20 ` Jiri Pirko
2020-03-26 1:30 ` Jacob Keller
1 sibling, 1 reply; 29+ messages in thread
From: Jakub Kicinski @ 2020-03-25 17:18 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Jacob Keller, netdev
On Wed, 25 Mar 2020 17:46:22 +0100 Jiri Pirko wrote:
> >+ err = region->ops->snapshot(devlink, info->extack, &data);
> >+ if (err)
> >+ goto err_decrement_snapshot_count;
> >+
> >+ err = __devlink_region_snapshot_create(region, data, snapshot_id);
> >+ if (err)
> >+ goto err_free_snapshot_data;
> >+
> >+ return 0;
> >+
> >+err_decrement_snapshot_count:
> >+ __devlink_snapshot_id_decrement(devlink, snapshot_id);
> >+err_free_snapshot_data:
>
> In devlink the error labers are named according to actions that failed.
Can we leave this to the author of the code to decide?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] ice: add a devlink region for dumping NVM contents
2020-03-24 22:34 ` [PATCH 10/10] ice: add a devlink region for dumping NVM contents Jacob Keller
@ 2020-03-25 17:18 ` Jiri Pirko
0 siblings, 0 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 17:18 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jakub Kicinski
Tue, Mar 24, 2020 at 11:34:45PM CET, jacob.e.keller@intel.com wrote:
>Add a devlink region for exposing the device's Non Volatime Memory flash
>contents.
>
>Support the recently added .snapshot operation, enabling userspace to
>request a snapshot of the NVM contents via DEVLINK_CMD_REGION_NEW.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
> Documentation/networking/devlink/ice.rst | 26 +++++
> drivers/net/ethernet/intel/ice/ice.h | 2 +
> drivers/net/ethernet/intel/ice/ice_devlink.c | 99 ++++++++++++++++++++
> drivers/net/ethernet/intel/ice/ice_devlink.h | 3 +
> drivers/net/ethernet/intel/ice/ice_main.c | 4 +
> 5 files changed, 134 insertions(+)
>
>diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
>index 37fbbd40a5e5..f3d6a3b50342 100644
>--- a/Documentation/networking/devlink/ice.rst
>+++ b/Documentation/networking/devlink/ice.rst
>@@ -69,3 +69,29 @@ The ``ice`` driver reports the following versions
> - The version of the DDP package that is active in the device. Note
> that both the name (as reported by ``fw.app.name``) and version are
> required to uniquely identify the package.
>+
>+Regions
>+=======
>+
>+The ``ice`` driver enables access to the contents of the Non Volatile Memory
>+flash chip via the ``nvm-flash`` region.
>+
>+Users can request an immediate capture of a snapshot via the
>+``DEVLINK_CMD_REGION_NEW``
>+
>+.. code:: shell
>+
>+ $ devlink region new pci/0000:01:00.0/nvm-flash snapshot 1
>+ $ devlink region dump pci/0000:01:00.0/nvm-flash snapshot 1
>+
>+ $ devlink region dump pci/0000:01:00.0/nvm-flash snapshot 1
>+ 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
>+ 0000000000000010 0000 0000 ffff ff04 0029 8c00 0028 8cc8
>+ 0000000000000020 0016 0bb8 0016 1720 0000 0000 c00f 3ffc
>+ 0000000000000030 bada cce5 bada cce5 bada cce5 bada cce5
>+
>+ $ devlink region read pci/0000:01:00.0/nvm-flash snapshot 1 address 0
>+ length 16
>+ 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
>+
>+ $ devlink region delete pci/0000:01:00.0/nvm-flash snapshot 1
>diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>index 8ce3afcfeca0..5c11448bfbb3 100644
>--- a/drivers/net/ethernet/intel/ice/ice.h
>+++ b/drivers/net/ethernet/intel/ice/ice.h
>@@ -351,6 +351,8 @@ struct ice_pf {
> /* devlink port data */
> struct devlink_port devlink_port;
>
>+ struct devlink_region *nvm_region;
>+
> /* OS reserved IRQ details */
> struct msix_entry *msix_entries;
> struct ice_res_tracker *irq_tracker;
>diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>index 27c5034c039a..9dd1b21820bf 100644
>--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>@@ -318,3 +318,102 @@ void ice_devlink_destroy_port(struct ice_pf *pf)
> devlink_port_type_clear(&pf->devlink_port);
> devlink_port_unregister(&pf->devlink_port);
> }
>+
>+/**
>+ * ice_devlink_nvm_snapshot - Capture a snapshot of the Shadow RAM contents
>+ * @devlink: the devlink instance
>+ * @extack: extended ACK response structure
>+ * @data: on exit points to snapshot data buffer
>+ *
>+ * This function is called in response to the DEVLINK_CMD_REGION_TRIGGER for
>+ * the shadow-ram devlink region. It captures a snapshot of the shadow ram
>+ * contents. This snapshot can later be viewed via the devlink-region
>+ * interface.
>+ *
>+ * @returns zero on success, and updates the data pointer. Returns a non-zero
>+ * error code on failure.
>+ */
>+static int
>+ice_devlink_nvm_snapshot(struct devlink *devlink, struct netlink_ext_ack *extack,
>+ u8 **data)
>+{
>+ struct ice_pf *pf = devlink_priv(devlink);
>+ struct device *dev = ice_pf_to_dev(pf);
>+ struct ice_hw *hw = &pf->hw;
>+ enum ice_status status;
>+ void *nvm_data;
>+ u32 nvm_size;
>+
>+ nvm_size = hw->nvm.flash_size;
>+ nvm_data = vzalloc(nvm_size);
>+ if (!nvm_data) {
>+ NL_SET_ERR_MSG_MOD(extack, "Out of memory");
>+ return -ENOMEM;
>+ }
>+
>+ status = ice_acquire_nvm(hw, ICE_RES_READ);
>+ if (status) {
>+ dev_dbg(dev, "ice_acquire_nvm failed, err %d aq_err %d\n",
>+ status, hw->adminq.sq_last_status);
>+ NL_SET_ERR_MSG_MOD(extack, "Failed to acquire NVM semaphore");
>+ vfree(nvm_data);
>+ return -EIO;
>+ }
>+
>+ status = ice_read_flat_nvm(hw, 0, &nvm_size, nvm_data, false);
>+ if (status) {
>+ dev_dbg(dev, "ice_read_flat_nvm failed after reading %u bytes, err %d aq_err %d\n",
>+ nvm_size, status, hw->adminq.sq_last_status);
>+ NL_SET_ERR_MSG_MOD(extack, "Failed to read NVM contents");
>+ ice_release_nvm(hw);
>+ vfree(nvm_data);
>+ return -EIO;
>+ }
>+
>+ ice_release_nvm(hw);
>+
>+ *data = nvm_data;
>+
>+ return 0;
>+}
>+
>+static const struct devlink_region_ops ice_nvm_region_ops = {
>+ .name = "nvm-flash",
>+ .destructor = vfree,
>+ .snapshot = ice_devlink_nvm_snapshot,
>+};
>+
>+/**
>+ * ice_devlink_init_regions - Initialize devlink regions
>+ * @pf: the PF device structure
>+ *
>+ * Create devlink regions used to enable access to dump the contents of the
>+ * flash memory on the device.
>+ */
>+void ice_devlink_init_regions(struct ice_pf *pf)
>+{
>+ struct devlink *devlink = priv_to_devlink(pf);
>+ struct device *dev = ice_pf_to_dev(pf);
>+ u64 nvm_size;
>+
>+ nvm_size = pf->hw.nvm.flash_size;
>+ pf->nvm_region = devlink_region_create(devlink, &ice_nvm_region_ops, 1,
>+ nvm_size);
>+ if (IS_ERR(pf->nvm_region)) {
>+ dev_warn(dev, "failed to create NVM devlink region, err %ld\n",
Shouldn't this be err?
>+ PTR_ERR(pf->nvm_region));
>+ pf->nvm_region = NULL;
>+ }
>+}
>+
>+/**
>+ * ice_devlink_destroy_regions - Destroy devlink regions
>+ * @pf: the PF device structure
>+ *
>+ * Remove previously created regions for this PF.
>+ */
>+void ice_devlink_destroy_regions(struct ice_pf *pf)
>+{
>+ if (pf->nvm_region)
>+ devlink_region_destroy(pf->nvm_region);
>+}
>diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.h b/drivers/net/ethernet/intel/ice/ice_devlink.h
>index f94dc93c24c5..6e806a08dc23 100644
>--- a/drivers/net/ethernet/intel/ice/ice_devlink.h
>+++ b/drivers/net/ethernet/intel/ice/ice_devlink.h
>@@ -11,4 +11,7 @@ void ice_devlink_unregister(struct ice_pf *pf);
> int ice_devlink_create_port(struct ice_pf *pf);
> void ice_devlink_destroy_port(struct ice_pf *pf);
>
>+void ice_devlink_init_regions(struct ice_pf *pf);
>+void ice_devlink_destroy_regions(struct ice_pf *pf);
>+
> #endif /* _ICE_DEVLINK_H_ */
>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>index 359ff8544773..306a4e5b2320 100644
>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>@@ -3276,6 +3276,8 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
> goto err_init_pf_unroll;
> }
>
>+ ice_devlink_init_regions(pf);
>+
> pf->num_alloc_vsi = hw->func_caps.guar_num_vsi;
> if (!pf->num_alloc_vsi) {
> err = -EIO;
>@@ -3385,6 +3387,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
> devm_kfree(dev, pf->vsi);
> err_init_pf_unroll:
> ice_deinit_pf(pf);
>+ ice_devlink_destroy_regions(pf);
> ice_deinit_hw(hw);
> err_exit_unroll:
> ice_devlink_unregister(pf);
>@@ -3427,6 +3430,7 @@ static void ice_remove(struct pci_dev *pdev)
> ice_vsi_free_q_vectors(pf->vsi[i]);
> }
> ice_deinit_pf(pf);
>+ ice_devlink_destroy_regions(pf);
> ice_deinit_hw(&pf->hw);
> ice_devlink_unregister(pf);
>
>--
>2.24.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW
2020-03-25 17:18 ` Jakub Kicinski
@ 2020-03-25 17:20 ` Jiri Pirko
2020-03-25 17:46 ` Jakub Kicinski
0 siblings, 1 reply; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 17:20 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jacob Keller, netdev
Wed, Mar 25, 2020 at 06:18:04PM CET, kuba@kernel.org wrote:
>On Wed, 25 Mar 2020 17:46:22 +0100 Jiri Pirko wrote:
>> >+ err = region->ops->snapshot(devlink, info->extack, &data);
>> >+ if (err)
>> >+ goto err_decrement_snapshot_count;
>> >+
>> >+ err = __devlink_region_snapshot_create(region, data, snapshot_id);
>> >+ if (err)
>> >+ goto err_free_snapshot_data;
>> >+
>> >+ return 0;
>> >+
>> >+err_decrement_snapshot_count:
>> >+ __devlink_snapshot_id_decrement(devlink, snapshot_id);
>> >+err_free_snapshot_data:
>>
>> In devlink the error labers are named according to actions that failed.
>
>Can we leave this to the author of the code to decide?
Well, if you look at 1 .c file, the reader should see one style. So...
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW
2020-03-25 17:20 ` Jiri Pirko
@ 2020-03-25 17:46 ` Jakub Kicinski
2020-03-25 18:41 ` Jiri Pirko
0 siblings, 1 reply; 29+ messages in thread
From: Jakub Kicinski @ 2020-03-25 17:46 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Jacob Keller, netdev
On Wed, 25 Mar 2020 18:20:36 +0100 Jiri Pirko wrote:
> Wed, Mar 25, 2020 at 06:18:04PM CET, kuba@kernel.org wrote:
> >On Wed, 25 Mar 2020 17:46:22 +0100 Jiri Pirko wrote:
> >> >+ err = region->ops->snapshot(devlink, info->extack, &data);
> >> >+ if (err)
> >> >+ goto err_decrement_snapshot_count;
> >> >+
> >> >+ err = __devlink_region_snapshot_create(region, data, snapshot_id);
> >> >+ if (err)
> >> >+ goto err_free_snapshot_data;
> >> >+
> >> >+ return 0;
> >> >+
> >> >+err_decrement_snapshot_count:
> >> >+ __devlink_snapshot_id_decrement(devlink, snapshot_id);
> >> >+err_free_snapshot_data:
> >>
> >> In devlink the error labers are named according to actions that failed.
> >
> >Can we leave this to the author of the code to decide?
>
> Well, if you look at 1 .c file, the reader should see one style. So...
Fine :)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 04/10] devlink: add function to take snapshot while locked
2020-03-24 22:34 ` [PATCH 04/10] devlink: add function to take snapshot while locked Jacob Keller
@ 2020-03-25 17:56 ` Jakub Kicinski
0 siblings, 0 replies; 29+ messages in thread
From: Jakub Kicinski @ 2020-03-25 17:56 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jiri Pirko, Jiri Pirko
On Tue, 24 Mar 2020 15:34:39 -0700 Jacob Keller wrote:
> A future change is going to add a new devlink command to request
> a snapshot on demand. This function will want to call the
> devlink_region_snapshot_create function while already holding the
> devlink instance lock.
>
> Extract the logic of this function into a static function prefixed by
> `__` to indicate that it is an internal helper function. Modify the
> original function to be implemented in terms of the new locked
> function.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] devlink: convert snapshot id getter to return an error
2020-03-24 22:34 ` [PATCH 06/10] devlink: convert snapshot id getter to return an error Jacob Keller
@ 2020-03-25 18:04 ` Jakub Kicinski
2020-03-25 19:13 ` Jiri Pirko
2020-03-26 1:33 ` Jacob Keller
0 siblings, 2 replies; 29+ messages in thread
From: Jakub Kicinski @ 2020-03-25 18:04 UTC (permalink / raw)
To: Jacob Keller; +Cc: netdev, Jiri Pirko, Jiri Pirko
On Tue, 24 Mar 2020 15:34:41 -0700 Jacob Keller wrote:
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index f7621ccb7b88..f9420b77e5fd 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -45,8 +45,7 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
> {
> struct nsim_dev *nsim_dev = file->private_data;
> void *dummy_data;
> - int err;
> - u32 id;
> + int err, id;
>
> dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
> if (!dummy_data)
> @@ -55,6 +54,10 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
> get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
>
> id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
> + if (id < 0) {
> + pr_err("Failed to get snapshot id\n");
> + return id;
> + }
> err = devlink_region_snapshot_create(nsim_dev->dummy_region,
> dummy_data, id);
> if (err) {
Hmm... next patch introduces some ref counting on the ID AFAICT,
should there be some form of snapshot_id_put(), once the driver is
done creating the regions it wants?
First what if driver wants to create two snapshots with the same ID but
user space manages to delete the first one before second one is created.
Second what if create fails, won't the snapshot ID just stay in XA with
count of 0 forever?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW
2020-03-25 17:46 ` Jakub Kicinski
@ 2020-03-25 18:41 ` Jiri Pirko
0 siblings, 0 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 18:41 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jacob Keller, netdev
Wed, Mar 25, 2020 at 06:46:41PM CET, kuba@kernel.org wrote:
>On Wed, 25 Mar 2020 18:20:36 +0100 Jiri Pirko wrote:
>> Wed, Mar 25, 2020 at 06:18:04PM CET, kuba@kernel.org wrote:
>> >On Wed, 25 Mar 2020 17:46:22 +0100 Jiri Pirko wrote:
>> >> >+ err = region->ops->snapshot(devlink, info->extack, &data);
>> >> >+ if (err)
>> >> >+ goto err_decrement_snapshot_count;
>> >> >+
>> >> >+ err = __devlink_region_snapshot_create(region, data, snapshot_id);
>> >> >+ if (err)
>> >> >+ goto err_free_snapshot_data;
>> >> >+
>> >> >+ return 0;
>> >> >+
>> >> >+err_decrement_snapshot_count:
>> >> >+ __devlink_snapshot_id_decrement(devlink, snapshot_id);
>> >> >+err_free_snapshot_data:
>> >>
>> >> In devlink the error labers are named according to actions that failed.
>> >
>> >Can we leave this to the author of the code to decide?
>>
>> Well, if you look at 1 .c file, the reader should see one style. So...
>
>Fine :)
You know, consistency is important. That is all I care about really :)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] devlink: convert snapshot id getter to return an error
2020-03-25 18:04 ` Jakub Kicinski
@ 2020-03-25 19:13 ` Jiri Pirko
2020-03-26 1:33 ` Jacob Keller
1 sibling, 0 replies; 29+ messages in thread
From: Jiri Pirko @ 2020-03-25 19:13 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jacob Keller, netdev, Jiri Pirko
Wed, Mar 25, 2020 at 07:04:25PM CET, kuba@kernel.org wrote:
>On Tue, 24 Mar 2020 15:34:41 -0700 Jacob Keller wrote:
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index f7621ccb7b88..f9420b77e5fd 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -45,8 +45,7 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>> {
>> struct nsim_dev *nsim_dev = file->private_data;
>> void *dummy_data;
>> - int err;
>> - u32 id;
>> + int err, id;
>>
>> dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
>> if (!dummy_data)
>> @@ -55,6 +54,10 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>> get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
>>
>> id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
>> + if (id < 0) {
>> + pr_err("Failed to get snapshot id\n");
>> + return id;
>> + }
>> err = devlink_region_snapshot_create(nsim_dev->dummy_region,
>> dummy_data, id);
>> if (err) {
>
>Hmm... next patch introduces some ref counting on the ID AFAICT,
>should there be some form of snapshot_id_put(), once the driver is
>done creating the regions it wants?
>
>First what if driver wants to create two snapshots with the same ID but
>user space manages to delete the first one before second one is created.
>
>Second what if create fails, won't the snapshot ID just stay in XA with
>count of 0 forever?
Yeah, that seems like a race condition this is introducing.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] devlink: track snapshot id usage count using an xarray
2020-03-25 16:08 ` Jiri Pirko
@ 2020-03-26 1:16 ` Jacob Keller
2020-03-26 1:43 ` Jacob Keller
1 sibling, 0 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-26 1:16 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, Jakub Kicinski
On 3/25/2020 9:08 AM, Jiri Pirko wrote:
> You somehow missed "07/10" from the subject :O
Huh. I sent using git-send-email, so I'm not sure how that happened.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW
2020-03-25 16:46 ` Jiri Pirko
2020-03-25 17:18 ` Jakub Kicinski
@ 2020-03-26 1:30 ` Jacob Keller
1 sibling, 0 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-26 1:30 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, Jakub Kicinski
On 3/25/2020 9:46 AM, Jiri Pirko wrote:
> Tue, Mar 24, 2020 at 11:34:43PM CET, jacob.e.keller@intel.com wrote:
>> +
>> + /* Check to make sure it's empty first */
>> + if (xa_load(&devlink->snapshot_ids, id))
>
> How this can happen? The entry was just allocated. WARN_ON.
>
Sure, I'll add WARN_ON. I think the return should still be kept, since
it causes the caller to fail instead of accidentally overwriting the
snapshot count.
>
>> + return -EBUSY;
>> +
>> + err = xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(0),
>> + GFP_KERNEL));
>
> Just return and avoid err variable.
>
Yep, done.
>> +
>> + if (region->cur_snapshots == region->max_snapshots) {
>> + NL_SET_ERR_MSG_MOD(info->extack, "The region has reached the maximum number of stored snapshots");
>> + return -ENOMEM;
>
> Maybe ENOBUFS or ENOSPC? ENOMEM seems odd as it is related to memory
> allocation fails which this is not.
>
Hmmm. This actually appears to be duplicated from the snapshot_create
function which used ENOMEM. Will add a patch to clean that up first.
It seems like we end up duplicating checks from within the
__devlink_region_snapshot_create merely because we have the extack
pointer here...
>
>> + }
>> +
>> + snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
>> +
>> + 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) {
>> + NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in used");
>
> Different message would be appropriate.
>
Right. This is the "this shouldn't happen" case from above I think.
>
>> + return err;
>> + }
>> +
>> + err = region->ops->snapshot(devlink, info->extack, &data);
>> + if (err)
>> + goto err_decrement_snapshot_count;
>> +
>> + err = __devlink_region_snapshot_create(region, data, snapshot_id);
>> + if (err)
>> + goto err_free_snapshot_data;
>> +
>> + return 0;
>> +
>> +err_decrement_snapshot_count:
>> + __devlink_snapshot_id_decrement(devlink, snapshot_id);
>> +err_free_snapshot_data:
>
> In devlink the error labers are named according to actions that failed.
> Please align.
>
Sure.
Thanks,
Jake
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] devlink: convert snapshot id getter to return an error
2020-03-25 18:04 ` Jakub Kicinski
2020-03-25 19:13 ` Jiri Pirko
@ 2020-03-26 1:33 ` Jacob Keller
1 sibling, 0 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-26 1:33 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, Jiri Pirko, Jiri Pirko
On 3/25/2020 11:04 AM, Jakub Kicinski wrote:
> On Tue, 24 Mar 2020 15:34:41 -0700 Jacob Keller wrote:
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index f7621ccb7b88..f9420b77e5fd 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -45,8 +45,7 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>> {
>> struct nsim_dev *nsim_dev = file->private_data;
>> void *dummy_data;
>> - int err;
>> - u32 id;
>> + int err, id;
>>
>> dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
>> if (!dummy_data)
>> @@ -55,6 +54,10 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>> get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
>>
>> id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
>> + if (id < 0) {
>> + pr_err("Failed to get snapshot id\n");
>> + return id;
>> + }
>> err = devlink_region_snapshot_create(nsim_dev->dummy_region,
>> dummy_data, id);
>> if (err) {
>
> Hmm... next patch introduces some ref counting on the ID AFAICT,
> should there be some form of snapshot_id_put(), once the driver is
> done creating the regions it wants?
>
> First what if driver wants to create two snapshots with the same ID but
> user space manages to delete the first one before second one is created.
>
> Second what if create fails, won't the snapshot ID just stay in XA with
> count of 0 forever?
>
Ah, yep good catch. I'll add a _put* function and make drivers call this.
Thanks,
Jake
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] devlink: track snapshot id usage count using an xarray
2020-03-25 16:08 ` Jiri Pirko
2020-03-26 1:16 ` Jacob Keller
@ 2020-03-26 1:43 ` Jacob Keller
1 sibling, 0 replies; 29+ messages in thread
From: Jacob Keller @ 2020-03-26 1:43 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, Jakub Kicinski
On 3/25/2020 9:08 AM, Jiri Pirko wrote:
> Tue, Mar 24, 2020 at 11:34:42PM CET, jacob.e.keller@intel.com wrote:
>> +static int __devlink_snapshot_id_increment(struct devlink *devlink, u32 id)
>> +{
>> + unsigned long count;
>> + int err;
>> + void *p;
>> +
>> + lockdep_assert_held(&devlink->lock);
>> +
>> + p = xa_load(&devlink->snapshot_ids, id);
>> + if (!p)
>> + return -EEXIST;
>
> This is confusing. You should return rather -ENOTEXIST, if it existed :)
> -EINVAL and WARN_ON. This should never happen
>
Yea this is confusing. I'll add a WARN_ON, and use EINVAL.
>
>> +
>> + if (!xa_is_value(p))
>> + return -EINVAL;
>> +
>> + count = xa_to_value(p);
>> + count++;
>> +
>> + err = xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
>> + GFP_KERNEL));
>
> Just return here and remove err variable.
Yep.
>> - if (devlink->snapshot_id >= INT_MAX)
>> - return -ENOSPC;
>> + /* xa_limit_31b ensures the id will be between 0 and INT_MAX */
>
> Well, currently the snapshot_id is u32. Even the netlink attr is u32.
> I believe we should not limit it here.
>
> Please have this as xa_limit_32b.
>
Currently we can't do that. Negative values are used to represent
errors, and allowing up to u32 would break the get function, because
large IDs would be interpreted as errors.
I'll clean this up in a patch first before the xarray stuff.
Thanks,
Jake
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2020-03-26 1:43 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 22:34 [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jacob Keller
2020-03-24 22:34 ` [PATCH 01/10] devlink: prepare to support region operations Jacob Keller
2020-03-24 22:34 ` [PATCH 02/10] devlink: convert snapshot destructor callback to region op Jacob Keller
2020-03-24 22:34 ` [PATCH 03/10] devlink: trivial: fix tab in function documentation Jacob Keller
2020-03-24 22:34 ` [PATCH 04/10] devlink: add function to take snapshot while locked Jacob Keller
2020-03-25 17:56 ` Jakub Kicinski
2020-03-24 22:34 ` [PATCH 05/10] devlink: extract snapshot id allocation to helper function Jacob Keller
2020-03-25 14:08 ` Jiri Pirko
2020-03-24 22:34 ` [PATCH 06/10] devlink: convert snapshot id getter to return an error Jacob Keller
2020-03-25 18:04 ` Jakub Kicinski
2020-03-25 19:13 ` Jiri Pirko
2020-03-26 1:33 ` Jacob Keller
2020-03-24 22:34 ` [PATCH] devlink: track snapshot id usage count using an xarray Jacob Keller
2020-03-25 16:08 ` Jiri Pirko
2020-03-26 1:16 ` Jacob Keller
2020-03-26 1:43 ` Jacob Keller
2020-03-24 22:34 ` [PATCH 08/10] devlink: implement DEVLINK_CMD_REGION_NEW Jacob Keller
2020-03-25 16:46 ` Jiri Pirko
2020-03-25 17:18 ` Jakub Kicinski
2020-03-25 17:20 ` Jiri Pirko
2020-03-25 17:46 ` Jakub Kicinski
2020-03-25 18:41 ` Jiri Pirko
2020-03-26 1:30 ` Jacob Keller
2020-03-24 22:34 ` [PATCH 09/10] netdevsim: support taking immediate snapshot via devlink Jacob Keller
2020-03-25 16:50 ` Jiri Pirko
2020-03-24 22:34 ` [PATCH 10/10] ice: add a devlink region for dumping NVM contents Jacob Keller
2020-03-25 17:18 ` Jiri Pirko
2020-03-25 13:49 ` [PATCH 00/10] implement DEVLINK_CMD_REGION_NEW Jiri Pirko
2020-03-25 13:50 ` Jiri Pirko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).