Message-ID: <3c593821-2123-9756-fc53-7c61fece015a@intel.com>
Date:   Thu, 5 Mar 2020 14:33:17 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, valex@...lanox.com, linyunsheng@...wei.com,
        lihong.yang@...el.com, kuba@...nel.org
Subject: Re: [RFC PATCH v2 14/22] devlink: implement DEVLINK_CMD_REGION_NEW



On 3/4/2020 10:41 PM, Jiri Pirko wrote:
> Wed, Mar 04, 2020 at 06:43:02PM CET, jacob.e.keller@...el.com wrote:
>>
>>
>> On 3/4/2020 3:58 AM, Jiri Pirko wrote:
>>> Tue, Mar 03, 2020 at 06:51:37PM CET, jacob.e.keller@...el.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 users would have to pick an ID that's unique across all regions. Ok.

I think we still need a reference count of how many snapshots are using
an ID (so that it can be released once all region snapshots using that
ID are destroyed).

We basically add this complexity even in cases where regions are totally
independent and never taken together.

One alternative would be to instead create some sort of grouping system,
but that has even more complication.

Ok. So I think we still need to track IDs using something like the IDR
with a reference count or similar structure.

Using only an IDA doesn't give us the ability to release previously used
IDs. Because on snapshot delete it has no idea whether another region
used that ID, so it can't remove it.

Maybe something like IDR with a refcount.. but we'd really like
something that can exist for some time with a refcount of zero. That's
what I basically used the NULL trick for in this version.

We can first check if the IDR has the ID when responding to
DEVLINK_CMD_REGION_NEW, and bail if so. That would enforce that users
must specify IDs which are unused by any region on the device.

Thanks,
Jake

Powered by blists - more mailing lists