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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Mar 04, 2020 at 06:43:02PM CET, jacob.e.keller@intel.com wrote:
>
>
>On 3/4/2020 3:58 AM, Jiri Pirko wrote:
>> Tue, Mar 03, 2020 at 06:51:37PM CET, jacob.e.keller@intel.com wrote:
>>>
>>> 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.
>> 
>
>I'm open to suggestions for better alternatives, or ways to improve code
>legibility.
>
>I want to preserve the following properties:
>
>* devlink_region_snapshot_id_get must choose IDs globally for the whole
>devlink, so that the ID can safely be re-used across multiple regions.
>
>* IDs must be reusable once all snapshots associated with the IDs have
>been removed
>
>* the new DEVLINK_CMD_REGION_NEW must allow userspace to select IDs
>
>* selecting IDs via DEVLINK_CMD_REGION_NEW should not really require the
>user to check more than the current interested snapshot
>
>* userspace should be able to re-use the same ID across multiple regions
>just like devlink_region_snapshot_id_get and driver triggered snapshots

Nope. I believe this is not desired. The point of having the same id for
the multiple regions is that the driver can obtain multiple region
snapshots during single FW event. For user, that it not the case.
For user, it would be 2 separate snapshots in 2 separate times. They
should not have the same ID.


>
>So, in a sense, the IDs must be a combination of both global and local
>to the region. When using an ID, the region must ensure that no more
>than one snapshot on that region uses the id.
>
>However, when selecting a new ID for use via the
>devlink_region_snapshot_id_get(), it must select one that is not yet
>used by *any* region.
>
>That's where the IDR came into use. I'm not a huge fan of this, so maybe
>there's something simpler.
>
>We could just do a brute force search across all regions to find an ID
>that isn't in use by any region snapshot....
>
>Thanks,
>Jake