All of lore.kernel.org
 help / color / mirror / Atom feed
From: Si-Wei Liu <si-wei.liu@oracle.com>
To: Jason Wang <jasowang@redhat.com>, Eli Cohen <elic@nvidia.com>,
	mst@redhat.com, virtualization@lists.linux-foundation.org
Cc: lvivier@redhat.com, eperezma@redhat.com
Subject: Re: [PATCH v7 03/14] vdpa: Sync calls set/get config/status with cf_mutex
Date: Mon, 10 Jan 2022 17:30:24 -0800	[thread overview]
Message-ID: <a4fefb05-958b-743c-87da-652420b98e70@oracle.com> (raw)
In-Reply-To: <3af55c17-49ad-bc9f-1b96-0a59b2ba1856@redhat.com>



On 1/9/2022 10:05 PM, Jason Wang wrote:
>
> 在 2022/1/8 上午9:23, Si-Wei Liu 写道:
>>
>>
>> On 1/6/2022 9:08 PM, Jason Wang wrote:
>>>
>>> 在 2022/1/7 上午8:33, Si-Wei Liu 写道:
>>>>
>>>>
>>>> On 1/5/2022 3:46 AM, Eli Cohen wrote:
>>>>> Add wrappers to get/set status and protect these operations with
>>>>> cf_mutex to serialize these operations with respect to get/set config
>>>>> operations.
>>>>>
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>> ---
>>>>>   drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++
>>>>>   drivers/vhost/vdpa.c         |  7 +++----
>>>>>   drivers/virtio/virtio_vdpa.c |  3 +--
>>>>>   include/linux/vdpa.h         |  3 +++
>>>>>   4 files changed, 26 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>> index 42d71d60d5dc..5134c83c4a22 100644
>>>>> --- a/drivers/vdpa/vdpa.c
>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>> @@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
>>>>>   static DEFINE_MUTEX(vdpa_dev_mutex);
>>>>>   static DEFINE_IDA(vdpa_index_ida);
>>>>>   +u8 vdpa_get_status(struct vdpa_device *vdev)
>>>>> +{
>>>>> +    u8 status;
>>>>> +
>>>>> +    mutex_lock(&vdev->cf_mutex);
>>>>> +    status = vdev->config->get_status(vdev);
>>>>> +    mutex_unlock(&vdev->cf_mutex);
>>>>> +    return status;
>>>>> +}
>>>>> +EXPORT_SYMBOL(vdpa_get_status);
>>>>> +
>>>>> +void vdpa_set_status(struct vdpa_device *vdev, u8 status)
>>>>> +{
>>>>> +    mutex_lock(&vdev->cf_mutex);
>>>>> +    vdev->config->set_status(vdev, status);
>>>>> +    mutex_unlock(&vdev->cf_mutex);
>>>>> +}
>>>>> +EXPORT_SYMBOL(vdpa_set_status);
>>>>> +
>>>>>   static struct genl_family vdpa_nl_family;
>>>>>     static int vdpa_dev_probe(struct device *d)
>>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>>>> index ebaa373e9b82..d9d499465e2e 100644
>>>>> --- a/drivers/vhost/vdpa.c
>>>>> +++ b/drivers/vhost/vdpa.c
>>>>> @@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct 
>>>>> vhost_vdpa *v, u8 __user *argp)
>>>>>   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 
>>>>> __user *statusp)
>>>>>   {
>>>>>       struct vdpa_device *vdpa = v->vdpa;
>>>>> -    const struct vdpa_config_ops *ops = vdpa->config;
>>>>>       u8 status;
>>>>>   -    status = ops->get_status(vdpa);
>>>>> +    status = vdpa_get_status(vdpa);
>>>> Not sure why we need to take cf_mutex here. Appears to me only 
>>>> setters (set_status and reset) need to take the lock in this function.
>>>
>>>
>>> You may be right but it doesn't harm and it is guaranteed to be 
>>> correct if we protect it with mutex here.
>> Is it more for future proof?
>
>
> I think so.

I guess in this situation it would be better defer to the future patch 
to add such locking or wrapper, although right now there are just two 
additional calls taking the lock needlessly when vhost_vdpa_get_status 
is called. Maybe it's just me but I'm worried some future patch may 
calls the locked API wrapper thousands of times unintendedly, then the 
overhead becomes quite obvious.

>
>
>> Ok, but IMHO it might be better to get some comment here, otherwise 
>> it's quite confusing why the lock needs to be held. vhost_vdpa had 
>> done its own serialization in vhost_vdpa_unlocked_ioctl() through 
>> vhost_dev`mutex.
>
>
> Right, but they are done for different levels, one is for vhost_dev 
> antoher is for vdpa_dev.
The cf_mutex is introduced to serialize the command line configure 
thread (via netlink) and the upstream driver calls from either the 
vhost_dev or virtio_dev. I don't see a case, even in future, where the 
netlink thread needs to update the virtio status on the fly. Can you 
enlighten me why that is at all possible?

Thanks,
-Siwei
>
> Thanks
>
>
>>
>> -Siwei
>>
>>>
>>> Thanks
>>>
>>>
>>>>
>>>>>         if (copy_to_user(statusp, &status, sizeof(status)))
>>>>>           return -EFAULT;
>>>>> @@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct 
>>>>> vhost_vdpa *v, u8 __user *statusp)
>>>>>       if (copy_from_user(&status, statusp, sizeof(status)))
>>>>>           return -EFAULT;
>>>>>   -    status_old = ops->get_status(vdpa);
>>>>> +    status_old = vdpa_get_status(vdpa);
>>>> Ditto.
>>>>
>>>>>         /*
>>>>>        * Userspace shouldn't remove status bits unless reset the
>>>>> @@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct 
>>>>> vhost_vdpa *v, u8 __user *statusp)
>>>>>           if (ret)
>>>>>               return ret;
>>>>>       } else
>>>>> -        ops->set_status(vdpa, status);
>>>>> +        vdpa_set_status(vdpa, status);
>>>> The reset() call in the if branch above needs to take the cf_mutex, 
>>>> the same way as that for set_status(). The reset() is effectively 
>>>> set_status(vdpa, 0).
>>>>
>>>> Thanks,
>>>> -Siwei
>>>>
>>>>>         if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & 
>>>>> VIRTIO_CONFIG_S_DRIVER_OK))
>>>>>           for (i = 0; i < nvqs; i++)
>>>>> diff --git a/drivers/virtio/virtio_vdpa.c 
>>>>> b/drivers/virtio/virtio_vdpa.c
>>>>> index a84b04ba3195..76504559bc25 100644
>>>>> --- a/drivers/virtio/virtio_vdpa.c
>>>>> +++ b/drivers/virtio/virtio_vdpa.c
>>>>> @@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct 
>>>>> virtio_device *vdev)
>>>>>   static void virtio_vdpa_set_status(struct virtio_device *vdev, 
>>>>> u8 status)
>>>>>   {
>>>>>       struct vdpa_device *vdpa = vd_get_vdpa(vdev);
>>>>> -    const struct vdpa_config_ops *ops = vdpa->config;
>>>>>   -    return ops->set_status(vdpa, status);
>>>>> +    return vdpa_set_status(vdpa, status);
>>>>>   }
>>>>>     static void virtio_vdpa_reset(struct virtio_device *vdev)
>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>>> index 9cc4291a79b3..ae047fae2603 100644
>>>>> --- a/include/linux/vdpa.h
>>>>> +++ b/include/linux/vdpa.h
>>>>> @@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev, 
>>>>> unsigned int offset,
>>>>>                void *buf, unsigned int len);
>>>>>   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
>>>>>                const void *buf, unsigned int length);
>>>>> +u8 vdpa_get_status(struct vdpa_device *vdev);
>>>>> +void vdpa_set_status(struct vdpa_device *vdev, u8 status);
>>>>> +
>>>>>   /**
>>>>>    * struct vdpa_mgmtdev_ops - vdpa device ops
>>>>>    * @dev_add: Add a vdpa device using alloc and register
>>>>
>>>
>>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

  reply	other threads:[~2022-01-11  1:30 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20220105114646.577224-1-elic@nvidia.com>
     [not found] ` <20220105114646.577224-4-elic@nvidia.com>
2022-01-07  0:33   ` [PATCH v7 03/14] vdpa: Sync calls set/get config/status with cf_mutex Si-Wei Liu
2022-01-07  5:08     ` Jason Wang
2022-01-08  1:23       ` Si-Wei Liu
2022-01-10  6:05         ` Jason Wang
2022-01-11  1:30           ` Si-Wei Liu [this message]
2022-01-11  4:46             ` Jason Wang
2022-01-11  6:26               ` Parav Pandit via Virtualization
2022-01-11  9:15                 ` Si-Wei Liu
2022-01-11  9:23               ` Si-Wei Liu
     [not found]     ` <20220109140956.GA70879@mtl-vdi-166.wap.labs.mlnx>
2022-01-11  1:14       ` Si-Wei Liu
     [not found] ` <20220105114646.577224-6-elic@nvidia.com>
2022-01-07  1:25   ` [PATCH v7 05/14] vdpa: Allow to configure max data virtqueues Si-Wei Liu
     [not found] ` <20220105114646.577224-8-elic@nvidia.com>
2022-01-07  1:27   ` [PATCH v7 07/14] vdpa/mlx5: Support configuring max data virtqueue Si-Wei Liu
2022-01-07  1:50     ` Si-Wei Liu
2022-01-07  5:43       ` Jason Wang
2022-01-08  1:38         ` Si-Wei Liu
     [not found]       ` <20220109141023.GB70879@mtl-vdi-166.wap.labs.mlnx>
2022-01-11  1:00         ` Si-Wei Liu
     [not found]           ` <20220111073416.GB149570@mtl-vdi-166.wap.labs.mlnx>
2022-01-11  8:28             ` Jason Wang
2022-01-11 12:05               ` Michael S. Tsirkin
2022-01-12  2:36                 ` Jason Wang
2022-01-11  8:52             ` Si-Wei Liu
     [not found]               ` <20220111152154.GA165838@mtl-vdi-166.wap.labs.mlnx>
2022-01-11 15:31                 ` Michael S. Tsirkin
2022-01-11 22:21                 ` Si-Wei Liu
2022-01-07 18:01     ` Nathan Chancellor
2022-01-08  1:43       ` Si-Wei Liu
2022-01-08  1:43         ` Si-Wei Liu
2022-01-10  6:53         ` Michael S. Tsirkin
2022-01-10  6:53           ` Michael S. Tsirkin
2022-01-10  6:58           ` Eli Cohen
     [not found] ` <20220105114646.577224-11-elic@nvidia.com>
2022-01-07  2:12   ` [PATCH v7 10/14] vdpa: Support reporting max device capabilities Si-Wei Liu
     [not found] ` <20220105114646.577224-12-elic@nvidia.com>
2022-01-07  2:12   ` [PATCH v7 11/14] vdpa/mlx5: Report " Si-Wei Liu
2022-01-07  5:49     ` Jason Wang
     [not found] ` <20220105114646.577224-5-elic@nvidia.com>
2022-01-07  5:14   ` [PATCH v7 04/14] vdpa: Read device configuration only if FEATURES_OK Jason Wang
     [not found] ` <20220105114646.577224-9-elic@nvidia.com>
2022-01-07  5:46   ` [PATCH v7 08/14] vdpa: Add support for returning device configuration information Jason Wang
     [not found] ` <20220105114646.577224-13-elic@nvidia.com>
2022-01-07  5:50   ` [PATCH v7 12/14] vdpa/vdpa_sim: Configure max supported virtqueues Jason Wang
     [not found] ` <20220105114646.577224-14-elic@nvidia.com>
2022-01-07  5:51   ` [PATCH v7 13/14] vdpa: Use BIT_ULL for bit operations Jason Wang
     [not found] ` <20220105114646.577224-15-elic@nvidia.com>
2022-01-07  5:51   ` [PATCH v7 14/14] vdpa/vdpa_sim_net: Report max device capabilities Jason Wang
2022-01-10  7:04 ` [PATCH v7 00/14] Allow for configuring max number of virtqueue pairs Michael S. Tsirkin
2022-01-10  7:09   ` Jason Wang
2022-01-11  1:59   ` Si-Wei Liu
     [not found]   ` <20220110074958.GA105688@mtl-vdi-166.wap.labs.mlnx>
2022-01-11  2:02     ` Si-Wei Liu

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=a4fefb05-958b-743c-87da-652420b98e70@oracle.com \
    --to=si-wei.liu@oracle.com \
    --cc=elic@nvidia.com \
    --cc=eperezma@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=lvivier@redhat.com \
    --cc=mst@redhat.com \
    --cc=virtualization@lists.linux-foundation.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.