Re: [RFC PATCH v2 14/22] devlink: implement DEVLINK_CMD_REGION_NEW

[Jiri Pirko <jiri@resnulli.us>]

Tue, Mar 03, 2020 at 06:51:37PM CET, jacob.e.keller@intel.com wrote:
>
>
>On 3/3/2020 1:30 AM, Jiri Pirko wrote:
>> Mon, Mar 02, 2020 at 08:38:12PM CET, jacob.e.keller@intel.com wrote:
>>> On 3/2/2020 9:41 AM, Jiri Pirko wrote:
>>>> Without ID? I would personally require snapshot id always. Without it,
>>>> it looks like you are creating region.
>>>>
>>>
>>> Not specifying an ID causes the ID to be auto-selected. I suppose
>>> support for that doesn't need to be kept.
>> 
>> Yeah, I would avoid it.
>> 
>> 
>
>Done.
>
>>>> Please have the same type here and for destructor. "u8 *" I guess.
>>>>
>>> Sure. My only concern would be if that causes a compiler warning when
>>> passing kfree/vfree to the destructor pointer. Alternatively we could
>>> use void **data, but it's definitely interpreted as a byte stream by the
>>> devlink core code.
>> 
>> I see. Leave it as is then.
>> 
>
>Ok.
>
>
>>>> In devlink.c, please don't wrap here.
>>>>
>>>
>>> For any of these?
>> 
>> Yep.
>> 
>
>Done.
>
>> 
>>>
>>>>
>>>>> +				   "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;
>>>>> +	}
>>>>> +
>>>>> +	if (info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
>>>>> +		/* __devlink_region_snapshot_create will take care of
>>>>> +		 * inserting the snapshot id into the IDR if necessary.
>>>>> +		 */
>>>>> +		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;
>>>>> +		}
>>>>> +	} else {
>>>>> +		snapshot_id = __devlink_region_snapshot_id_get(devlink);
>>>>> +	}
>>>>> +
>>>>> +	err = region->ops->snapshot(devlink, info->extack, &data);
>>>>
>>>> Don't you put the "id"? Looks like a leak.
>>>>
>>>
>>> The id is put into the devlink_region_snapshot_create, the driver code
>>> doesn't need to know about it as far as I can tell.
>>>
>>> Currently the ids are managed by an IDR which stores a reference count
>>> of how many snapshots use it.
>>>
>>> Use of "NULL" is done so that devlink_region_snapshot_id_get can
>>> "pre-allocate" the ID without assigning snapshots, assuming that a later
>>> call to the devlink_region_snapshot_create will find that id and create
>>> or increment it's refcount.
>>>
>>> This complexity comes from the fact that the current code requires the
>>> ability to re-use the same snapshot id for different regions in the same
>>> devlink. This devlink_region_snapshot_id_get must return IDs which are
>>> unique across all regions. If a user does DEVLINK_CMD_REGION_NEW with an
>>> ID, it would only be used by a single snapshot. We need to make sure
>>> that this doesn't confuse devlink_region_snapshot_id_get. Additionally,
>>> I wanted to make sure that the snapshot IDs could be re-used once the
>>> related snapshots have been deleted.
>> 
>> Okay, I see. I'm just worried about possible scenario when user does
>> alloc up to max of u32 and always hits the error path.
>> 
>
>Hm. The flow here was about supporting both with and without snapshot
>IDs. That will be gone in the next revision and should make the code clear.
>
>The IDs are stored in the IDR with either a NULL, or a pointer to a
>refcount of the number of snapshots currently using them.
>
>On devlink_region_snapshot_create, the id must have been allocated by
>the devlink_region_snapshot_id_get ahead of time by the driver.
>
>When devlink_region_snapshot_id_get is called, a NULL is inserted into
>the IDR at a suitable ID number (i.e. one that does not yet have a
>refcount).
>
>On devlink_region_snapshot_new, the callback for the new command, the ID
>must be specified by userspace.
>
>Both cases, the ID is confirmed to not be in use for that region by
>looping over all snapshots and checking to see if one can be found that
>has the ID.
>
>In __devlink_region_snapshot_create, the IDR is checked to see if it is
>already used. If so, the refcount is incremented. If there is no
>refcount (i.e. the IDR returns NULL), a new refcount is created, set to
>1, and inserted.
>
>The basic idea is the refcount is "how many snapshots are actually using
>this ID". Use of devlink_region_snapshot_id_get can "pre-allocate" an ID
>value so that future calls to devlink_region_id_get won't re-use the
>same ID number even if no snapshot with that ID has yet been created.
>
>The refcount isn't actually incremented until the snapshot is created
>with that ID.
>
>Userspace never uses devlink_region_snapshot_id_get now, since it always
>requires an ID to be chosen.
>
>On snapshot delete, the id refcount is reduced, and when it hits zero
>the ID is released from the IDR. This way, IDs can be re-used as long as
>no remaining snapshots on any region point to them.
>
>This system enables userspace to simply treat snapshot ids as unique to
>each region, and to provide their own values on the command line. It
>also preserves the behavior that devlink_region_snapshot_id_get will
>never select an ID that is used by any region on that devlink, so that
>the id can be safely used for multiple snapshots triggered at the same time.
>
>This will hopefully be more clear in the next revision.

Okay, I see. The code is a bit harder to follow.