On Tue, Jan 11, 2022 at 9:30 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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.
I'm fine to remove this if others agree on this.
>
> >
> >
> >> 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?
After some thought I don't see a case. I can think of NEEDS_RESET but
it should come with a config interrupt so we're probably fine without
the mutex here.
Thanks
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
