Re: [PATCH] devlink: track snapshot id usage count using an xarray

[Jacob Keller <jacob.e.keller@intel.com>]

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