| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7fc12b09-1897-1938-bd58-9b1b5bbd4be5@oracle.com>
Date: Tue, 10 May 2022 10:58:29 -0700
From: Si-Wei Liu <si-wei.liu@...cle.com>
To: Eli Cohen <elic@...dia.com>, "mst@...hat.com" <mst@...hat.com>,
"jasowang@...hat.com" <jasowang@...hat.com>
Cc: "virtualization@...ts.linux-foundation.org"
<virtualization@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
On 5/8/2022 12:44 AM, Eli Cohen wrote:
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@...cle.com>
>> Sent: Tuesday, May 3, 2022 2:48 AM
>> To: Eli Cohen <elic@...dia.com>; mst@...hat.com; jasowang@...hat.com
>> Cc: virtualization@...ts.linux-foundation.org; linux-kernel@...r.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>> Allows to read vendor statistics of a vdpa device. The specific
>>> statistics data are received from the upstream driver in the form of an
>>> (attribute name, attribute value) pairs.
>>>
>>> An example of statistics for mlx5_vdpa device are:
>>>
>>> received_desc - number of descriptors received by the virtqueue
>>> completed_desc - number of descriptors completed by the virtqueue
>>>
>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>> N chained descriptors are counted correctly N times as one would expect.
>>>
>>> A new callback was added to vdpa_config_ops which provides the means for
>>> the vdpa driver to return statistics results.
>>>
>>> The interface allows for reading all the supported virtqueues, including
>>> the control virtqueue if it exists.
>>>
>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>> following patch:
>>>
>>> 1. Read statistics for the virtqueue at index 1
>>>
>>> $ vdpa dev vstats show vdpa-a qidx 1
>>> vdpa-a:
>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>
>>> 2. Read statistics for the virtqueue at index 32
>>> $ vdpa dev vstats show vdpa-a qidx 32
>>> vdpa-a:
>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>
>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>> {"vstats":{"vdpa-a":{
>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>> "name":"completed_desc","value":417548}}}
>>>
>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>> {
>>> "vstats": {
>>> "vdpa-a": {
>>>
>>> "queue_type": "rx",
>>> "queue_index": 0,
>>> "name": "received_desc",
>>> "value": 417776,
>>> "name": "completed_desc",
>>> "value": 417548
>>> }
>>> }
>>> }
>>>
>>> Signed-off-by: Eli Cohen <elic@...dia.com>
>>> ---
>>> drivers/vdpa/vdpa.c | 129 ++++++++++++++++++++++++++++++++++++++
>>> include/linux/vdpa.h | 5 ++
>>> include/uapi/linux/vdpa.h | 6 ++
>>> 3 files changed, 140 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 2b75c00b1005..933466f61ca8 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>> return err;
>>> }
>>>
>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>> + struct genl_info *info, u32 index)
>>> +{
>>> + int err;
>>> +
>>> + err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>> + if (err)
>>> + return err;
>>> +
>>> + if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>> + return -EMSGSIZE;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>> + struct genl_info *info, u32 index)
>>> +{
>>> + int err;
>>> +
>>> + if (!vdev->config->get_vendor_vq_stats)
>>> + return -EOPNOTSUPP;
>>> +
>>> + err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>> + if (err)
>>> + return err;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>> + struct sk_buff *msg,
>>> + struct genl_info *info, u32 index)
>>> +{
>>> + u32 device_id;
>>> + void *hdr;
>>> + int err;
>>> + u32 portid = info->snd_portid;
>>> + u32 seq = info->snd_seq;
>>> + u32 flags = 0;
>>> +
>>> + hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>> + VDPA_CMD_DEV_VSTATS_GET);
>>> + if (!hdr)
>>> + return -EMSGSIZE;
>>> +
>>> + if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>> + err = -EMSGSIZE;
>>> + goto undo_msg;
>>> + }
>>> +
>>> + device_id = vdev->config->get_device_id(vdev);
>>> + if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>> + err = -EMSGSIZE;
>>> + goto undo_msg;
>>> + }
>>> +
>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>> otherwise I can't image how you can ensure the atomicity to infer
>> queue_type for control vq.
>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>> into vendor_stats_fill()?
>>
>>> + err = vendor_stats_fill(vdev, msg, info, index);
>>> +
>>> + genlmsg_end(msg, hdr);
>>> +
>>> + return err;
>>> +
>>> +undo_msg:
>>> + genlmsg_cancel(msg, hdr);
>>> + return err;
>>> +}
>>> +
>>> static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>> {
>>> struct vdpa_device *vdev;
>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>> return msg->len;
>>> }
>>>
>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>> + struct genl_info *info)
>>> +{
>>> + struct vdpa_device *vdev;
>>> + struct sk_buff *msg;
>>> + const char *devname;
>>> + struct device *dev;
>>> + u32 index;
>>> + int err;
>>> +
>>> + if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>> + return -EINVAL;
>>> +
>>> + if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>> + return -EINVAL;
>>> +
>>> + devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>> + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> + if (!msg)
>>> + return -ENOMEM;
>>> +
>>> + index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>> + mutex_lock(&vdpa_dev_mutex);
>> Given that stats_get() is a read-only operation that might happen quite
>> often from time to time, I wonder if it is now a good timing to convert
>> vdpa_dev_mutex to a semaphore?
> No sure I follow you. You still need that the process that took the lock will
> release it. So in that respect we should still use a mutex.
>
> Did you mean use readers/writers lock?
Yeah, I meant Reader/Writer semaphore, sorry.
-Siwei
>
>>> + dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>> + if (!dev) {
>>> + NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>> + err = -ENODEV;
>>> + goto dev_err;
>> Missing nlmsg_free().
>>> + }
>>> + vdev = container_of(dev, struct vdpa_device, dev);
>>> + if (!vdev->mdev) {
>>> + NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>> + err = -EINVAL;
>>> + goto mdev_err;
>> Missing nlmsg_free().
> Thanks will fix.
>
>> Otherwise looks fine.
>>
>> Acked-by: Si-Wei Liu <si-wei.liu@...cle.com>
>>
>>
>> -Siwei
>>> + }
>>> + err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>> + if (!err)
>>> + err = genlmsg_reply(msg, info);
>>> +
>>> + put_device(dev);
>>> + mutex_unlock(&vdpa_dev_mutex);
>>> +
>>> + if (err)
>>> + nlmsg_free(msg);
>>> +
>>> + return err;
>>> +
>>> +mdev_err:
>>> + put_device(dev);
>>> +dev_err:
>>> + mutex_unlock(&vdpa_dev_mutex);
>>> + return err;
>>> +}
>>> +
>>> static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>> [VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>> [VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>> [VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>> /* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>> [VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>> + [VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>> };
>>>
>>> static const struct genl_ops vdpa_nl_ops[] = {
>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>> .doit = vdpa_nl_cmd_dev_config_get_doit,
>>> .dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>> },
>>> + {
>>> + .cmd = VDPA_CMD_DEV_VSTATS_GET,
>>> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>> + .doit = vdpa_nl_cmd_dev_stats_get_doit,
>>> + .flags = GENL_ADMIN_PERM,
>>> + },
>>> };
>>>
>>> static struct genl_family vdpa_nl_family __ro_after_init = {
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index 8943a209202e..48ed1fc00830 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>> const struct vdpa_vq_state *state);
>>> int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>> struct vdpa_vq_state *state);
>>> + int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>> + struct sk_buff *msg,
>>> + struct netlink_ext_ack *extack);
>>> struct vdpa_notification_area
>>> (*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>> /* vq irq is not expected to be changed once DRIVER_OK is set */
>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>> int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>> void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>
>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>> +
>>> #endif /* _LINUX_VDPA_H */
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>> VDPA_CMD_DEV_DEL,
>>> VDPA_CMD_DEV_GET, /* can dump */
>>> VDPA_CMD_DEV_CONFIG_GET, /* can dump */
>>> + VDPA_CMD_DEV_VSTATS_GET,
>>> };
>>>
>>> enum vdpa_attr {
>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */
>>> +
>>> + VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */
>>> + VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
>>> + VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */
>>> +
>>> /* new attributes must be added above here */
>>> VDPA_ATTR_MAX,
>>> };
Powered by blists - more mailing lists