From: Jiri Pirko <jiri@resnulli.us>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: netdev@vger.kernel.org, valex@mellanox.com,
linyunsheng@huawei.com, lihong.yang@intel.com, kuba@kernel.org
Subject: Re: [RFC PATCH v2 14/22] devlink: implement DEVLINK_CMD_REGION_NEW
Date: Wed, 4 Mar 2020 12:58:18 +0100 [thread overview]
Message-ID: <20200304115818.GA4558@nanopsycho> (raw)
In-Reply-To: <861a914b-1e3c-fdb6-a452-96a0b5a5c2c0@intel.com>
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.
next prev parent reply other threads:[~2020-03-04 11:58 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-14 23:21 [RFC PATCH v2 00/22] devlink region updates Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 01/22] ice: use __le16 types for explicitly Little Endian values Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 02/22] ice: create function to read a section of the NVM and Shadow RAM Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 03/22] ice: implement full NVM read from ETHTOOL_GEEPROM Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 04/22] ice: enable initial devlink support Jacob Keller
2020-03-02 16:30 ` Jiri Pirko
2020-03-02 19:29 ` Jacob Keller
2020-03-03 13:47 ` Jiri Pirko
2020-03-03 17:53 ` Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 05/22] ice: rename variables used for Option ROM version Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 06/22] ice: add basic handler for devlink .info_get Jacob Keller
2020-02-19 2:45 ` Jakub Kicinski
2020-02-19 17:33 ` Jacob Keller
2020-02-19 19:57 ` Jakub Kicinski
2020-02-19 21:37 ` Jacob Keller
2020-02-19 23:47 ` Jakub Kicinski
2020-02-20 0:06 ` Jacob Keller
2020-02-21 22:11 ` Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 07/22] ice: add board identifier info to " Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 08/22] devlink: prepare to support region operations Jacob Keller
2020-03-02 17:42 ` Jiri Pirko
2020-02-14 23:22 ` [RFC PATCH v2 09/22] devlink: convert snapshot destructor callback to region op Jacob Keller
2020-03-02 17:42 ` Jiri Pirko
2020-02-14 23:22 ` [RFC PATCH v2 10/22] devlink: trivial: fix tab in function documentation Jacob Keller
2020-03-02 17:42 ` Jiri Pirko
2020-02-14 23:22 ` [RFC PATCH v2 11/22] devlink: add functions to take snapshot while locked Jacob Keller
2020-03-02 17:43 ` Jiri Pirko
2020-03-02 22:25 ` Jacob Keller
2020-03-03 8:41 ` Jiri Pirko
2020-02-14 23:22 ` [RFC PATCH v2 12/22] devlink: convert snapshot id getter to return an error Jacob Keller
2020-03-02 17:44 ` Jiri Pirko
2020-02-14 23:22 ` [RFC PATCH v2 13/22] devlink: track snapshot ids using an IDR and refcounts Jacob Keller
2020-02-18 21:44 ` Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 14/22] devlink: implement DEVLINK_CMD_REGION_NEW Jacob Keller
2020-03-02 17:41 ` Jiri Pirko
2020-03-02 19:38 ` Jacob Keller
2020-03-03 9:30 ` Jiri Pirko
2020-03-03 17:51 ` Jacob Keller
2020-03-04 11:58 ` Jiri Pirko [this message]
2020-03-04 17:43 ` Jacob Keller
2020-03-05 6:41 ` Jiri Pirko
2020-03-05 22:33 ` Jacob Keller
2020-03-06 6:16 ` Jiri Pirko
2020-03-02 22:11 ` Jacob Keller
2020-03-02 22:14 ` Jacob Keller
2020-03-02 22:35 ` Jacob Keller
2020-03-03 9:31 ` Jiri Pirko
2020-02-14 23:22 ` [RFC PATCH v2 15/22] netdevsim: support taking immediate snapshot via devlink Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 16/22] devlink: simplify arguments for read_snapshot_fill Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 17/22] devlink: use min_t to calculate data_size Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 18/22] devlink: report extended error message in region_read_dumpit Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 19/22] devlink: remove unnecessary parameter from chunk_fill function Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 20/22] devlink: refactor region_read_snapshot_fill to use a callback function Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 21/22] devlink: support directly reading from region memory Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 22/22] ice: add a devlink region to dump shadow RAM contents Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 1/2] devlink: add support for DEVLINK_CMD_REGION_NEW Jacob Keller
2020-02-14 23:22 ` [RFC PATCH v2 2/2] devlink: stop requiring snapshot for regions Jacob Keller
2020-03-02 16:27 ` [RFC PATCH v2 00/22] devlink region updates Jiri Pirko
2020-03-02 19:27 ` Jacob Keller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200304115818.GA4558@nanopsycho \
--to=jiri@resnulli.us \
--cc=jacob.e.keller@intel.com \
--cc=kuba@kernel.org \
--cc=lihong.yang@intel.com \
--cc=linyunsheng@huawei.com \
--cc=netdev@vger.kernel.org \
--cc=valex@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).