2023-09-12 20:38:22

by Cindy Lu

[permalink] [raw]
Subject: [RFC v2 3/4] vduse: update the vq_info in ioctl

In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
with reconnect info, After mapping the reconnect pages to userspace
The userspace App will update the reconnect_time in
struct vhost_reconnect_vring, If this is not 0 then it means this
vq is reconnected and will update the last_avail_idx

Signed-off-by: Cindy Lu <[email protected]>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
include/uapi/linux/vduse.h | 6 ++++++
2 files changed, 19 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 2c69f4004a6e..680b23dbdde2 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
struct vduse_vq_info vq_info;
struct vduse_virtqueue *vq;
u32 index;
+ struct vdpa_reconnect_info *area;
+ struct vhost_reconnect_vring *vq_reconnect;

ret = -EFAULT;
if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
@@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,

vq_info.ready = vq->ready;

+ area = &vq->reconnect_info;
+
+ vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
+ /*check if the vq is reconnect, if yes then update the last_avail_idx*/
+ if ((vq_reconnect->last_avail_idx !=
+ vq_info.split.avail_index) &&
+ (vq_reconnect->reconnect_time != 0)) {
+ vq_info.split.avail_index =
+ vq_reconnect->last_avail_idx;
+ }
+
ret = -EFAULT;
if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
break;
diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
index 11bd48c72c6c..d585425803fd 100644
--- a/include/uapi/linux/vduse.h
+++ b/include/uapi/linux/vduse.h
@@ -350,4 +350,10 @@ struct vduse_dev_response {
};
};

+struct vhost_reconnect_vring {
+ __u16 reconnect_time;
+ __u16 last_avail_idx;
+ _Bool avail_wrap_counter;
+};
+
#endif /* _UAPI_VDUSE_H_ */
--
2.34.3


2023-09-13 20:47:29

by Jason Wang

[permalink] [raw]
Subject: Re: [RFC v2 3/4] vduse: update the vq_info in ioctl

On Tue, Sep 12, 2023 at 11:00 AM Cindy Lu <[email protected]> wrote:
>
> In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
> with reconnect info, After mapping the reconnect pages to userspace
> The userspace App will update the reconnect_time in
> struct vhost_reconnect_vring, If this is not 0 then it means this
> vq is reconnected and will update the last_avail_idx
>
> Signed-off-by: Cindy Lu <[email protected]>
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
> include/uapi/linux/vduse.h | 6 ++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 2c69f4004a6e..680b23dbdde2 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> struct vduse_vq_info vq_info;
> struct vduse_virtqueue *vq;
> u32 index;
> + struct vdpa_reconnect_info *area;
> + struct vhost_reconnect_vring *vq_reconnect;
>
> ret = -EFAULT;
> if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>
> vq_info.ready = vq->ready;
>
> + area = &vq->reconnect_info;
> +
> + vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
> + /*check if the vq is reconnect, if yes then update the last_avail_idx*/
> + if ((vq_reconnect->last_avail_idx !=
> + vq_info.split.avail_index) &&
> + (vq_reconnect->reconnect_time != 0)) {
> + vq_info.split.avail_index =
> + vq_reconnect->last_avail_idx;
> + }
> +
> ret = -EFAULT;
> if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
> break;
> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> index 11bd48c72c6c..d585425803fd 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -350,4 +350,10 @@ struct vduse_dev_response {
> };
> };
>
> +struct vhost_reconnect_vring {
> + __u16 reconnect_time;
> + __u16 last_avail_idx;
> + _Bool avail_wrap_counter;

Please add a comment for each field.

And I never saw _Bool is used in uapi before, maybe it's better to
pack it with last_avail_idx into a __u32.

Btw, do we need to track inflight descriptors as well?

Thanks

> +};
> +
> #endif /* _UAPI_VDUSE_H_ */
> --
> 2.34.3
>

2023-09-25 07:30:30

by Cindy Lu

[permalink] [raw]
Subject: Re: [RFC v2 3/4] vduse: update the vq_info in ioctl

On Tue, Sep 12, 2023 at 3:39 PM Jason Wang <[email protected]> wrote:
>
> On Tue, Sep 12, 2023 at 11:00 AM Cindy Lu <[email protected]> wrote:
> >
> > In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
> > with reconnect info, After mapping the reconnect pages to userspace
> > The userspace App will update the reconnect_time in
> > struct vhost_reconnect_vring, If this is not 0 then it means this
> > vq is reconnected and will update the last_avail_idx
> >
> > Signed-off-by: Cindy Lu <[email protected]>
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
> > include/uapi/linux/vduse.h | 6 ++++++
> > 2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 2c69f4004a6e..680b23dbdde2 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > struct vduse_vq_info vq_info;
> > struct vduse_virtqueue *vq;
> > u32 index;
> > + struct vdpa_reconnect_info *area;
> > + struct vhost_reconnect_vring *vq_reconnect;
> >
> > ret = -EFAULT;
> > if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> > @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >
> > vq_info.ready = vq->ready;
> >
> > + area = &vq->reconnect_info;
> > +
> > + vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
> > + /*check if the vq is reconnect, if yes then update the last_avail_idx*/
> > + if ((vq_reconnect->last_avail_idx !=
> > + vq_info.split.avail_index) &&
> > + (vq_reconnect->reconnect_time != 0)) {
> > + vq_info.split.avail_index =
> > + vq_reconnect->last_avail_idx;
> > + }
> > +
> > ret = -EFAULT;
> > if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
> > break;
> > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > index 11bd48c72c6c..d585425803fd 100644
> > --- a/include/uapi/linux/vduse.h
> > +++ b/include/uapi/linux/vduse.h
> > @@ -350,4 +350,10 @@ struct vduse_dev_response {
> > };
> > };
> >
> > +struct vhost_reconnect_vring {
> > + __u16 reconnect_time;
> > + __u16 last_avail_idx;
> > + _Bool avail_wrap_counter;
>
> Please add a comment for each field.
>
Sure will do

> And I never saw _Bool is used in uapi before, maybe it's better to
> pack it with last_avail_idx into a __u32.
>
Thanks will fix this
> Btw, do we need to track inflight descriptors as well?
>
I will check this
Thanks

cindy
> Thanks
>
> > +};
> > +
> > #endif /* _UAPI_VDUSE_H_ */
> > --
> > 2.34.3
> >
>

2023-09-29 11:38:50

by Maxime Coquelin

[permalink] [raw]
Subject: Re: [RFC v2 3/4] vduse: update the vq_info in ioctl



On 9/25/23 06:15, Cindy Lu wrote:
> On Tue, Sep 12, 2023 at 3:39 PM Jason Wang <[email protected]> wrote:
>>
>> On Tue, Sep 12, 2023 at 11:00 AM Cindy Lu <[email protected]> wrote:
>>>
>>> In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
>>> with reconnect info, After mapping the reconnect pages to userspace
>>> The userspace App will update the reconnect_time in
>>> struct vhost_reconnect_vring, If this is not 0 then it means this
>>> vq is reconnected and will update the last_avail_idx
>>>
>>> Signed-off-by: Cindy Lu <[email protected]>
>>> ---
>>> drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
>>> include/uapi/linux/vduse.h | 6 ++++++
>>> 2 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> index 2c69f4004a6e..680b23dbdde2 100644
>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> @@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>>> struct vduse_vq_info vq_info;
>>> struct vduse_virtqueue *vq;
>>> u32 index;
>>> + struct vdpa_reconnect_info *area;
>>> + struct vhost_reconnect_vring *vq_reconnect;
>>>
>>> ret = -EFAULT;
>>> if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
>>> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>>>
>>> vq_info.ready = vq->ready;
>>>
>>> + area = &vq->reconnect_info;
>>> +
>>> + vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
>>> + /*check if the vq is reconnect, if yes then update the last_avail_idx*/
>>> + if ((vq_reconnect->last_avail_idx !=
>>> + vq_info.split.avail_index) &&
>>> + (vq_reconnect->reconnect_time != 0)) {
>>> + vq_info.split.avail_index =
>>> + vq_reconnect->last_avail_idx;
>>> + }
>>> +
>>> ret = -EFAULT;
>>> if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
>>> break;
>>> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
>>> index 11bd48c72c6c..d585425803fd 100644
>>> --- a/include/uapi/linux/vduse.h
>>> +++ b/include/uapi/linux/vduse.h
>>> @@ -350,4 +350,10 @@ struct vduse_dev_response {
>>> };
>>> };
>>>
>>> +struct vhost_reconnect_vring {
>>> + __u16 reconnect_time;
>>> + __u16 last_avail_idx;
>>> + _Bool avail_wrap_counter;
>>
>> Please add a comment for each field.
>>
> Sure will do
>
>> And I never saw _Bool is used in uapi before, maybe it's better to
>> pack it with last_avail_idx into a __u32.
>>
> Thanks will fix this
>> Btw, do we need to track inflight descriptors as well?
>>
> I will check this

For existing networking implemenation, this is not necessary.
But it should be for block devices.

Maxime

> Thanks
>
> cindy
>> Thanks
>>
>>> +};
>>> +
>>> #endif /* _UAPI_VDUSE_H_ */
>>> --
>>> 2.34.3
>>>
>>
>

2023-09-29 14:32:15

by Maxime Coquelin

[permalink] [raw]
Subject: Re: [RFC v2 3/4] vduse: update the vq_info in ioctl



On 9/12/23 09:39, Jason Wang wrote:
> On Tue, Sep 12, 2023 at 11:00 AM Cindy Lu <[email protected]> wrote:
>>
>> In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
>> with reconnect info, After mapping the reconnect pages to userspace
>> The userspace App will update the reconnect_time in
>> struct vhost_reconnect_vring, If this is not 0 then it means this
>> vq is reconnected and will update the last_avail_idx
>>
>> Signed-off-by: Cindy Lu <[email protected]>
>> ---
>> drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
>> include/uapi/linux/vduse.h | 6 ++++++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 2c69f4004a6e..680b23dbdde2 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>> struct vduse_vq_info vq_info;
>> struct vduse_virtqueue *vq;
>> u32 index;
>> + struct vdpa_reconnect_info *area;
>> + struct vhost_reconnect_vring *vq_reconnect;
>>
>> ret = -EFAULT;
>> if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
>> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>>
>> vq_info.ready = vq->ready;
>>
>> + area = &vq->reconnect_info;
>> +
>> + vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
>> + /*check if the vq is reconnect, if yes then update the last_avail_idx*/
>> + if ((vq_reconnect->last_avail_idx !=
>> + vq_info.split.avail_index) &&
>> + (vq_reconnect->reconnect_time != 0)) {
>> + vq_info.split.avail_index =
>> + vq_reconnect->last_avail_idx;
>> + }
>> +
>> ret = -EFAULT;
>> if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
>> break;
>> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
>> index 11bd48c72c6c..d585425803fd 100644
>> --- a/include/uapi/linux/vduse.h
>> +++ b/include/uapi/linux/vduse.h
>> @@ -350,4 +350,10 @@ struct vduse_dev_response {
>> };
>> };
>>
>> +struct vhost_reconnect_vring {
>> + __u16 reconnect_time;
>> + __u16 last_avail_idx;
>> + _Bool avail_wrap_counter;
>
> Please add a comment for each field.
>
> And I never saw _Bool is used in uapi before, maybe it's better to
> pack it with last_avail_idx into a __u32.

Better as two distincts __u16 IMHO.

Thanks,
Maxime

>
> Btw, do we need to track inflight descriptors as well?
>
> Thanks
>
>> +};
>> +
>> #endif /* _UAPI_VDUSE_H_ */
>> --
>> 2.34.3
>>
>

2023-10-08 05:18:44

by Jason Wang

[permalink] [raw]
Subject: Re: [RFC v2 3/4] vduse: update the vq_info in ioctl

On Fri, Sep 29, 2023 at 5:12 PM Maxime Coquelin
<[email protected]> wrote:
>
>
>
> On 9/12/23 09:39, Jason Wang wrote:
> > On Tue, Sep 12, 2023 at 11:00 AM Cindy Lu <[email protected]> wrote:
> >>
> >> In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
> >> with reconnect info, After mapping the reconnect pages to userspace
> >> The userspace App will update the reconnect_time in
> >> struct vhost_reconnect_vring, If this is not 0 then it means this
> >> vq is reconnected and will update the last_avail_idx
> >>
> >> Signed-off-by: Cindy Lu <[email protected]>
> >> ---
> >> drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
> >> include/uapi/linux/vduse.h | 6 ++++++
> >> 2 files changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> index 2c69f4004a6e..680b23dbdde2 100644
> >> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> >> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> @@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >> struct vduse_vq_info vq_info;
> >> struct vduse_virtqueue *vq;
> >> u32 index;
> >> + struct vdpa_reconnect_info *area;
> >> + struct vhost_reconnect_vring *vq_reconnect;
> >>
> >> ret = -EFAULT;
> >> if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> >> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >>
> >> vq_info.ready = vq->ready;
> >>
> >> + area = &vq->reconnect_info;
> >> +
> >> + vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
> >> + /*check if the vq is reconnect, if yes then update the last_avail_idx*/
> >> + if ((vq_reconnect->last_avail_idx !=
> >> + vq_info.split.avail_index) &&
> >> + (vq_reconnect->reconnect_time != 0)) {
> >> + vq_info.split.avail_index =
> >> + vq_reconnect->last_avail_idx;
> >> + }
> >> +
> >> ret = -EFAULT;
> >> if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
> >> break;
> >> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> >> index 11bd48c72c6c..d585425803fd 100644
> >> --- a/include/uapi/linux/vduse.h
> >> +++ b/include/uapi/linux/vduse.h
> >> @@ -350,4 +350,10 @@ struct vduse_dev_response {
> >> };
> >> };
> >>
> >> +struct vhost_reconnect_vring {
> >> + __u16 reconnect_time;
> >> + __u16 last_avail_idx;
> >> + _Bool avail_wrap_counter;
> >
> > Please add a comment for each field.
> >
> > And I never saw _Bool is used in uapi before, maybe it's better to
> > pack it with last_avail_idx into a __u32.
>
> Better as two distincts __u16 IMHO.

Fine with me.

Thanks

>
> Thanks,
> Maxime
>
> >
> > Btw, do we need to track inflight descriptors as well?
> >
> > Thanks
> >
> >> +};
> >> +
> >> #endif /* _UAPI_VDUSE_H_ */
> >> --
> >> 2.34.3
> >>
> >
>

2023-10-08 12:57:24

by Cindy Lu

[permalink] [raw]
Subject: Re: [RFC v2 3/4] vduse: update the vq_info in ioctl

On Sun, Oct 8, 2023 at 1:17 PM Jason Wang <[email protected]> wrote:
>
> On Fri, Sep 29, 2023 at 5:12 PM Maxime Coquelin
> <[email protected]> wrote:
> >
> >
> >
> > On 9/12/23 09:39, Jason Wang wrote:
> > > On Tue, Sep 12, 2023 at 11:00 AM Cindy Lu <[email protected]> wrote:
> > >>
> > >> In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
> > >> with reconnect info, After mapping the reconnect pages to userspace
> > >> The userspace App will update the reconnect_time in
> > >> struct vhost_reconnect_vring, If this is not 0 then it means this
> > >> vq is reconnected and will update the last_avail_idx
> > >>
> > >> Signed-off-by: Cindy Lu <[email protected]>
> > >> ---
> > >> drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
> > >> include/uapi/linux/vduse.h | 6 ++++++
> > >> 2 files changed, 19 insertions(+)
> > >>
> > >> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > >> index 2c69f4004a6e..680b23dbdde2 100644
> > >> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > >> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > >> @@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > >> struct vduse_vq_info vq_info;
> > >> struct vduse_virtqueue *vq;
> > >> u32 index;
> > >> + struct vdpa_reconnect_info *area;
> > >> + struct vhost_reconnect_vring *vq_reconnect;
> > >>
> > >> ret = -EFAULT;
> > >> if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> > >> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > >>
> > >> vq_info.ready = vq->ready;
> > >>
> > >> + area = &vq->reconnect_info;
> > >> +
> > >> + vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
> > >> + /*check if the vq is reconnect, if yes then update the last_avail_idx*/
> > >> + if ((vq_reconnect->last_avail_idx !=
> > >> + vq_info.split.avail_index) &&
> > >> + (vq_reconnect->reconnect_time != 0)) {
> > >> + vq_info.split.avail_index =
> > >> + vq_reconnect->last_avail_idx;
> > >> + }
> > >> +
> > >> ret = -EFAULT;
> > >> if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
> > >> break;
> > >> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > >> index 11bd48c72c6c..d585425803fd 100644
> > >> --- a/include/uapi/linux/vduse.h
> > >> +++ b/include/uapi/linux/vduse.h
> > >> @@ -350,4 +350,10 @@ struct vduse_dev_response {
> > >> };
> > >> };
> > >>
> > >> +struct vhost_reconnect_vring {
> > >> + __u16 reconnect_time;
> > >> + __u16 last_avail_idx;
> > >> + _Bool avail_wrap_counter;
> > >
> > > Please add a comment for each field.
> > >
> > > And I never saw _Bool is used in uapi before, maybe it's better to
> > > pack it with last_avail_idx into a __u32.
> >
> > Better as two distincts __u16 IMHO.
>
> Fine with me.
>
> Thanks
>

sure will fix this
Thanks
Cindy
> >
> > Thanks,
> > Maxime
> >
> > >
> > > Btw, do we need to track inflight descriptors as well?
> > >
> > > Thanks
> > >
> > >> +};
> > >> +
> > >> #endif /* _UAPI_VDUSE_H_ */
> > >> --
> > >> 2.34.3
> > >>
> > >
> >
>