diff mbox series

[v3,6/7] vduse: Update the vq_info in ioctl VDUSE_VQ_GET_INFO

Message ID 20231205083444.3029239-7-lulu@redhat.com
State New
Headers
Series vduse: Add support for reconnection |

Commit Message

Cindy Lu Dec. 5, 2023, 8:34 a.m. UTC 
  Once the reconnect memory pages are mapped to userspace, the userspace
application will update the "reconnected" bit in the
"struct vduse_dev_reconnect_data".
The kernel will then check this bit. If it is not set to
"VDUSE_NOT_RECONNECT", it means that the application has been
reconnected, and the kernel will synchronize the vq information.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

Comments

Jason Wang Dec. 7, 2023, 8:46 a.m. UTC | #1 
On Tue, Dec 5, 2023 at 4:35 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Once the reconnect memory pages are mapped to userspace, the userspace
> application will update the "reconnected" bit in the
> "struct vduse_dev_reconnect_data".
> The kernel will then check this bit. If it is not set to
> "VDUSE_NOT_RECONNECT", it means that the application has been
> reconnected, and the kernel will synchronize the vq information.

Can you explain why we need such a flag?

>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f55f415629de..422f1aedebac 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1193,6 +1193,9 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 struct vduse_vq_info vq_info;
>                 struct vduse_virtqueue *vq;
>                 u32 index;
> +               unsigned long vaddr;
> +               struct vduse_vq_state *vq_reconnect;
> +               struct vduse_dev_reconnect_data *dev_reconnect;
>
>                 ret = -EFAULT;
>                 if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> @@ -1209,6 +1212,12 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 vq_info.device_addr = vq->device_addr;
>                 vq_info.num = vq->num;
>
> +               vaddr = dev->vdpa_reconnect_vaddr;
> +               dev_reconnect = (struct vduse_dev_reconnect_data *)vaddr;
> +
> +               vaddr = vq->vdpa_reconnect_vaddr;
> +               vq_reconnect = (struct vduse_vq_state *)vaddr;
> +
>                 if (dev->driver_features & BIT_ULL(VIRTIO_F_RING_PACKED)) {
>                         vq_info.packed.last_avail_counter =
>                                 vq->state.packed.last_avail_counter;
> @@ -1218,9 +1227,22 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                                 vq->state.packed.last_used_counter;
>                         vq_info.packed.last_used_idx =
>                                 vq->state.packed.last_used_idx;
> -               } else
> +                       /*check if the vq is reconnect, if yes then update the info*/
> +                       if (dev_reconnect->reconnected != VDUSE_NOT_RECONNECT) {

So the dev_reconnect is shared, how to synchronize between them?

> +                               vq_info.packed.last_avail_idx =
> +                                       vq_reconnect->packed.last_avail_idx;
> +                               vq_info.packed.last_avail_counter =
> +                                       vq_reconnect->packed.last_avail_counter;
> +                       }
> +               } else {
>                         vq_info.split.avail_index =
>                                 vq->state.split.avail_index;
> +                       /*check if the vq is reconnect, if yes then update the info*/
> +                       if (dev_reconnect->reconnected != VDUSE_NOT_RECONNECT) {

If this only check is !=, I'd define it as VDUSE_RECONNECT to ease the
code logic.

> +                               vq_info.split.avail_index =
> +                                       vq_reconnect->split.avail_index;
> +                       }
> +               }

It looks like I miss something here

If I read the code correctly, vq_reconnect is written by userspace, so
userspace query it again via the get_vq_info()? If this is true,
userspace can just ignore the index via GET_VQ_INFO() or is it just
for consistency?

Thanks


>
>                 vq_info.ready = vq->ready;
>
> --
> 2.34.3
>
diff mbox series

Patch

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index f55f415629de..422f1aedebac 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1193,6 +1193,9 @@  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 		struct vduse_vq_info vq_info;
 		struct vduse_virtqueue *vq;
 		u32 index;
+		unsigned long vaddr;
+		struct vduse_vq_state *vq_reconnect;
+		struct vduse_dev_reconnect_data *dev_reconnect;
 
 		ret = -EFAULT;
 		if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
@@ -1209,6 +1212,12 @@  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 		vq_info.device_addr = vq->device_addr;
 		vq_info.num = vq->num;
 
+		vaddr = dev->vdpa_reconnect_vaddr;
+		dev_reconnect = (struct vduse_dev_reconnect_data *)vaddr;
+
+		vaddr = vq->vdpa_reconnect_vaddr;
+		vq_reconnect = (struct vduse_vq_state *)vaddr;
+
 		if (dev->driver_features & BIT_ULL(VIRTIO_F_RING_PACKED)) {
 			vq_info.packed.last_avail_counter =
 				vq->state.packed.last_avail_counter;
@@ -1218,9 +1227,22 @@  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 				vq->state.packed.last_used_counter;
 			vq_info.packed.last_used_idx =
 				vq->state.packed.last_used_idx;
-		} else
+			/*check if the vq is reconnect, if yes then update the info*/
+			if (dev_reconnect->reconnected != VDUSE_NOT_RECONNECT) {
+				vq_info.packed.last_avail_idx =
+					vq_reconnect->packed.last_avail_idx;
+				vq_info.packed.last_avail_counter =
+					vq_reconnect->packed.last_avail_counter;
+			}
+		} else {
 			vq_info.split.avail_index =
 				vq->state.split.avail_index;
+			/*check if the vq is reconnect, if yes then update the info*/
+			if (dev_reconnect->reconnected != VDUSE_NOT_RECONNECT) {
+				vq_info.split.avail_index =
+					vq_reconnect->split.avail_index;
+			}
+		}
 
 		vq_info.ready = vq->ready;