Re: [PATCH 04/15] netdevsim: support taking immediate snapshot via devlink
From: Jakub Kicinski <kuba@kernel.org>
Date: 2020-01-31 18:07:15
On Thu, 30 Jan 2020 14:58:59 -0800, Jacob Keller wrote:
quoted hunk ↗ jump to hunk
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 | 38 +++++++++++++++---- .../drivers/net/netdevsim/devlink.sh | 5 +++ 2 files changed, 36 insertions(+), 7 deletions(-)diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index d521b7bfe007..924cd328037f 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c@@ -38,24 +38,47 @@ static struct dentry *nsim_dev_ddir; #define NSIM_DEV_DUMMY_REGION_SIZE (1024 * 32) +static int nsim_dev_take_snapshot(struct devlink *devlink,
nit: break the line after static int, you've done it in other patches
so I trust you agree it's a superior formatting style :)
+ struct netlink_ext_ack *extack,
+ u8 **data,
+ devlink_snapshot_data_dest_t **destructor)
+{
+ void *dummy_data;
+
+ dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
+ if (!dummy_data) {
+ NL_SET_ERR_MSG(extack, "Out of memory");Unnecessary, there will be an OOM splat, and ENOMEM is basically exactly the same as the message.
+ return -ENOMEM; + } + + get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE); + + *data = dummy_data; + *destructor = kfree;
Is there any driver which uses different destructor for different snapshots? Looks like something we could put in ops, maybe?
+ 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;
- void *dummy_data;
+ devlink_snapshot_data_dest_t *destructor;
+ u8 *dummy_data;
int err;
u32 id;
- dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
- if (!dummy_data)
- return -ENOMEM;
-
- get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
+ err = nsim_dev_take_snapshot(priv_to_devlink(nsim_dev), NULL,
+ &dummy_data, &destructor);
+ if (err) {
+ pr_err("Failed to capture region snapshot\n");Also not a very useful message for netdevsim IMHO give the caller clearly requested a snapshot and will get a more informative error from errno.
+ return err;
+ }
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, destructor);
if (err) {
pr_err("Failed to create region snapshot\n");
kfree(dummy_data);