Re: [patch net-next] netdevsim: implement support for devlink region and snapshots

[Jiri Pirko <jiri@resnulli.us>]

Tue, Aug 13, 2019 at 02:58:59AM CEST, jakub.kicinski@netronome.com wrote:
>On Mon, 12 Aug 2019 12:16:20 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Implement dummy region of size 32K and allow user to create snapshots
>> or random data using debugfs file trigger.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>I'm nacking all the netdevsim patches unless the selftest 
>is posted at the same time :/
>
>You're leaking those features one by one what if you get distracted 
>and the tests never materialize :/
>
>This is all dead code.

Okay, fair enough. Will add it now.


>
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index 08ca59fc189b..e76ea6a3cb60 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -27,6 +27,41 @@
>>  
>>  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)
>> +{
>> +	struct nsim_dev *nsim_dev = file->private_data;
>> +	void *dummy_data;
>> +	u32 id;
>> +	int err;
>> +
>> +	dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
>> +	if (!dummy_data) {
>> +		pr_err("Failed to allocate memory for region snapshot\n");
>> +		goto out;
>> +	}
>> +
>> +	get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
>> +
>> +	id = devlink_region_shapshot_id_get(priv_to_devlink(nsim_dev));
>> +	err = devlink_region_snapshot_create(nsim_dev->dummy_region,
>> +					     dummy_data, id, kfree);
>> +	if (err)
>> +		pr_err("Failed to create region snapshot\n");
>> +
>> +out:
>> +	return count;
>
>why not return an error?

Okay.


>
>> +}
>> +
>> +static const struct file_operations nsim_dev_take_snapshot_fops = {
>> +	.open = simple_open,
>> +	.write = nsim_dev_take_snapshot_write,
>> +	.llseek = generic_file_llseek,
>> +};
>> +
>>  static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev)
>>  {
>>  	char dev_ddir_name[16];
>> @@ -44,6 +79,8 @@ static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev)
>>  			   &nsim_dev->max_macs);
>>  	debugfs_create_bool("test1", 0600, nsim_dev->ddir,
>>  			    &nsim_dev->test1);
>> +	debugfs_create_file("take_snapshot", 0200, nsim_dev->ddir, nsim_dev,
>> +			    &nsim_dev_take_snapshot_fops);
>>  	return 0;
>>  }
>>  
>> @@ -248,6 +285,26 @@ static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink)
>>  		nsim_dev->test1 = saved_value.vbool;
>>  }
>>  
>> +#define NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX 16
>> +
>> +static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
>> +				      struct devlink *devlink)
>> +{
>> +	nsim_dev->dummy_region =
>> +		devlink_region_create(devlink, "dummy",
>> +				      NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
>> +				      NSIM_DEV_DUMMY_REGION_SIZE);
>> +	if (IS_ERR(nsim_dev->dummy_region))
>> +		return PTR_ERR(nsim_dev->dummy_region);
>> +
>> +	return 0;
>
>PTR_ERR_OR_ZERO()

Okay.


>
>> +}
>> +
>> +static void nsim_dev_dummy_region_exit(struct nsim_dev *nsim_dev)
>> +{
>> +	devlink_region_destroy(nsim_dev->dummy_region);
>> +}
>> +
>>  static int nsim_dev_reload(struct devlink *devlink,
>>  			   struct netlink_ext_ack *extack)
>>  {
>