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: Tue, 3 Mar 2020 10:30:43 +0100 [thread overview]
Message-ID: <20200303093043.GB2178@nanopsycho> (raw)
In-Reply-To: <6f0023d0-a151-87f0-db3a-df01b7e6c045@intel.com>
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:
>> Sat, Feb 15, 2020 at 12:22:13AM CET, jacob.e.keller@intel.com wrote:
>>> Implement support for the DEVLINK_CMD_REGION_NEW command for creating
>>> snapshots. This new command parallels the existing
>>> DEVLINK_CMD_REGION_DEL.
>>>
>>> In order for DEVLINK_CMD_REGION_NEW to work for a region, the new
>>> ".snapshot" operation must be implemented in the region's ops structure.
>>>
>>> The desired snapshot id may be provided. If the requested id is already
>>> in use, an error will be reported. If no id is provided one will be
>>> selected in the same way as a triggered snapshot.
>>>
>>> In either case, the reference count for that id will be incremented
>>> in the snapshot IDR.
>>>
>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>>> ---
>>> .../networking/devlink/devlink-region.rst | 12 +++-
>>> include/net/devlink.h | 6 ++
>>> net/core/devlink.c | 72 +++++++++++++++++++
>>> 3 files changed, 88 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
>>> index 1a7683e7acb2..a24faf2b6b7a 100644
>>> --- a/Documentation/networking/devlink/devlink-region.rst
>>> +++ b/Documentation/networking/devlink/devlink-region.rst
>>> @@ -20,6 +20,11 @@ address regions that are otherwise inaccessible to the user.
>>> Regions may also be used to provide an additional way to debug complex error
>>> states, but see also :doc:`devlink-health`
>>>
>>> +Regions may optionally support capturing a snapshot on demand via the
>>> +``DEVLINK_CMD_REGION_NEW`` netlink message. A driver wishing to allow
>>> +requested snapshots must implement the ``.snapshot`` callback for the region
>>> +in its ``devlink_region_ops`` structure.
>>> +
>>> example usage
>>> -------------
>>>
>>> @@ -40,8 +45,11 @@ example usage
>>> # Delete a snapshot using:
>>> $ devlink region del pci/0000:00:05.0/cr-space snapshot 1
>>>
>>> - # Trigger (request) a snapshot be taken:
>>> - $ devlink region trigger pci/0000:00:05.0/cr-space
>>
>> Odd. It is actually "devlink region dump". There is no trigger.
>>
>>
>>> + # Request an immediate snapshot, if supported by the region
>>> + $ devlink region new pci/0000:00:05.0/cr-space
>>
>> 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.
>
>>
>>> +
>>> + # Request an immediate snapshot with a specific id
>>> + $ devlink region new pci/0000:00:05.0/cr-space snapshot 5
>>>
>>> # Dump a snapshot:
>>> $ devlink region dump pci/0000:00:05.0/fw-health snapshot 1
>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>> index 3a5ff6bea143..3cd0ff2040b2 100644
>>> --- a/include/net/devlink.h
>>> +++ b/include/net/devlink.h
>>> @@ -498,10 +498,16 @@ struct devlink_info_req;
>>> * struct devlink_region_ops - Region operations
>>> * @name: region name
>>> * @destructor: callback used to free snapshot memory when deleting
>>> + * @snapshot: callback to request an immediate snapshot. On success,
>>> + * the data variable must be updated to point to the snapshot data.
>>> + * The function will be called while the devlink instance lock is
>>> + * held.
>>> */
>>> struct devlink_region_ops {
>>> const char *name;
>>> void (*destructor)(const void *data);
>>> + int (*snapshot)(struct devlink *devlink, struct netlink_ext_ack *extack,
>>> + u8 **data);
>>
>> 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.
>
>>
>>> };
>>>
>>> struct devlink_fmsg;
>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>> index 9571063846cc..b5d1b21e5178 100644
>>> --- a/net/core/devlink.c
>>> +++ b/net/core/devlink.c
>>> @@ -4045,6 +4045,71 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
>>> return 0;
>>> }
>>>
>>> +static int
>>> +devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
>>> +{
>>> + struct devlink *devlink = info->user_ptr[0];
>>> + struct devlink_region *region;
>>> + const char *region_name;
>>> + u32 snapshot_id;
>>> + u8 *data;
>>> + int err;
>>> +
>>> + if (!info->attrs[DEVLINK_ATTR_REGION_NAME]) {
>>> + NL_SET_ERR_MSG_MOD(info->extack, "No region name provided");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
>>> + region = devlink_region_get_by_name(devlink, region_name);
>>> + if (!region) {
>>> + NL_SET_ERR_MSG_MOD(info->extack,
>>
>> In devlink.c, please don't wrap here.
>>
>
>For any of these?
Yep.
>
>>
>>> + "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.
>
>>
>>> + if (err)
>>> + return err;
>>> +
>>> + err = __devlink_region_snapshot_create(region, data, snapshot_id);
>>> + if (err)
>>> + goto err_free_snapshot_data;
>>> +
>>> + return 0;
>>> +
>>> +err_free_snapshot_data:
>>> + region->ops->destructor(data);
>>> + return err;
>>> +}
>>> +
>>> static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
>>> struct devlink *devlink,
>>> u8 *chunk, u32 chunk_size,
>>> @@ -6358,6 +6423,13 @@ static const struct genl_ops devlink_nl_ops[] = {
>>> .flags = GENL_ADMIN_PERM,
>>> .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>> },
>>> + {
>>> + .cmd = DEVLINK_CMD_REGION_NEW,
>>> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>> + .doit = devlink_nl_cmd_region_new,
>>> + .flags = GENL_ADMIN_PERM,
>>> + .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>> + },
>>> {
>>> .cmd = DEVLINK_CMD_REGION_DEL,
>>> .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>> --
>>> 2.25.0.368.g28a2d05eebfb
>>>
next prev parent reply other threads:[~2020-03-03 9:30 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 [this message]
2020-03-03 17:51 ` Jacob Keller
2020-03-04 11:58 ` Jiri Pirko
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=20200303093043.GB2178@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).