[PATCH v5 0/5] vduse: Add support for reconnection

[PATCH v5 0/5] vduse: Add support for reconnection

[Cindy Lu]

Here is the reconnect support in vduse

Kernel will allocate pages for reconnection.
Userspace needs to use mmap to map the memory to userspace and use these pages to
save the reconnect information.

test passd in vduse+dpdk-testpmd

change in V2
1. Address the comments from v1
2. Add the document for reconnect process

change in V3
1. Move the vdpa_vq_state to the uAPI.  vduse will use this to synchronize the vq info between the kernel and userspace app.
2. Add a new ioctl VDUSE_DEV_GET_CONFIG. userspace app use this to get config space
3. Rewrite the commit message.
4. Only save the address for the page address and remove the index.
5. remove the ioctl VDUSE_GET_RECONNECT_INFO, userspace app will use uAPI VDUSE_RECONNCT_MMAP_SIZE to mmap
6. Rewrite the document for the reconnect process to make it clearer.

change in v4
1. Change the number of map pages to VQ numbers. UserSpace APP can define and maintain the structure for saving reconnection information in userspace. The kernel will not maintain this information.
2. Rewrite the document for the reconnect process to make it clearer.
3. add the new ioctl for VDUSE_DEV_GET_CONFIG/VDUSE_DEV_GET_STATUS

change in V5
1. update the Documentation for vduse reconnection 
2. merge the patch from Dan Carpenter <dan.carpenter@linaro.org> vduse: Fix off by one in vduse_dev_mmap()
3. fix the wrong comment in the code 

Signed-off-by: Cindy Lu <lulu@redhat.com>

Cindy Lu (5):
  vduse: Add new ioctl VDUSE_DEV_GET_CONFIG
  vduse: Add new ioctl VDUSE_DEV_GET_STATUS
  vduse: Add function to get/free the pages for reconnection
  vduse: Add file operation for mmap
  Documentation: Add reconnect process for VDUSE

 Documentation/userspace-api/vduse.rst |  41 +++++++++
 drivers/vdpa/vdpa_user/vduse_dev.c    | 125 ++++++++++++++++++++++++++
 include/uapi/linux/vduse.h            |   5 ++
 3 files changed, 171 insertions(+)

-- 
2.43.0

[PATCH v5 1/5] vduse: Add new ioctl VDUSE_DEV_GET_CONFIG

[Cindy Lu]

The ioctl VDUSE_DEV_GET_CONFIG is used by the Userspace App
to get the device configuration space.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 21 +++++++++++++++++++++
 include/uapi/linux/vduse.h         |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 1c1d71d69026..ab246da27616 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1368,6 +1368,27 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 		ret = 0;
 		break;
 	}
+	case VDUSE_DEV_GET_CONFIG: {
+		struct vduse_config_data config;
+		unsigned long size = offsetof(struct vduse_config_data, buffer);
+
+		ret = -EFAULT;
+		if (copy_from_user(&config, argp, size))
+			break;
+
+		ret = -EINVAL;
+		if (config.offset > dev->config_size || config.length == 0 ||
+		    config.length > dev->config_size - config.offset)
+			break;
+
+		if (copy_to_user(argp + size, dev->config + config.offset,
+				 config.length)) {
+			ret = -EFAULT;
+			break;
+		}
+		ret = 0;
+		break;
+	}
 	default:
 		ret = -ENOIOCTLCMD;
 		break;
diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
index 11bd48c72c6c..125d7529d91b 100644
--- a/include/uapi/linux/vduse.h
+++ b/include/uapi/linux/vduse.h
@@ -350,4 +350,7 @@ struct vduse_dev_response {
 	};
 };
 
+/* get device configuration space */
+#define VDUSE_DEV_GET_CONFIG _IOR(VDUSE_BASE, 0x1b, struct vduse_config_data)
+
 #endif /* _UAPI_VDUSE_H_ */
-- 
2.43.0

[PATCH v5 2/5] vduse: Add new ioctl VDUSE_DEV_GET_STATUS

[Cindy Lu]

The ioctl VDUSE_DEV_GET_STATUS is used by the Userspace App
to get the device status

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++++++
 include/uapi/linux/vduse.h         | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index ab246da27616..ef3c9681941e 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1389,6 +1389,13 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 		ret = 0;
 		break;
 	}
+
+	case VDUSE_DEV_GET_STATUS:
+		/*
+		 * Returns the status read from device
+		 */
+		ret = put_user(dev->status, (u8 __user *)argp);
+		break;
 	default:
 		ret = -ENOIOCTLCMD;
 		break;
diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
index 125d7529d91b..f501173a9d69 100644
--- a/include/uapi/linux/vduse.h
+++ b/include/uapi/linux/vduse.h
@@ -353,4 +353,6 @@ struct vduse_dev_response {
 /* get device configuration space */
 #define VDUSE_DEV_GET_CONFIG _IOR(VDUSE_BASE, 0x1b, struct vduse_config_data)
 
+#define VDUSE_DEV_GET_STATUS _IOR(VDUSE_BASE, 0x1c, __u8)
+
 #endif /* _UAPI_VDUSE_H_ */
-- 
2.43.0

[PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Cindy Lu]

Add the function vduse_alloc_reconnnect_info_mem
and vduse_alloc_reconnnect_info_mem
These functions allow vduse to allocate and free memory for reconnection
information. The amount of memory allocated is vq_num pages.
Each VQS will map its own page where the reconnection information will be saved

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

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index ef3c9681941e..2da659d5f4a8 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -65,6 +65,7 @@ struct vduse_virtqueue {
 	int irq_effective_cpu;
 	struct cpumask irq_affinity;
 	struct kobject kobj;
+	unsigned long vdpa_reconnect_vaddr;
 };
 
 struct vduse_dev;
@@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
 
 	vq->irq_effective_cpu = curr_cpu;
 }
+static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
+{
+	unsigned long vaddr = 0;
+	struct vduse_virtqueue *vq;
+
+	for (int i = 0; i < dev->vq_num; i++) {
+		/*page 0~ vq_num save the reconnect info for vq*/
+		vq = dev->vqs[i];
+		vaddr = get_zeroed_page(GFP_KERNEL);
+		if (vaddr == 0)
+			return -ENOMEM;
+
+		vq->vdpa_reconnect_vaddr = vaddr;
+	}
+
+	return 0;
+}
+
+static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
+{
+	struct vduse_virtqueue *vq;
+
+	for (int i = 0; i < dev->vq_num; i++) {
+		vq = dev->vqs[i];
+
+		if (vq->vdpa_reconnect_vaddr)
+			free_page(vq->vdpa_reconnect_vaddr);
+		vq->vdpa_reconnect_vaddr = 0;
+	}
+
+	return 0;
+}
 
 static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
@@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
 		mutex_unlock(&dev->lock);
 		return -EBUSY;
 	}
+	vduse_free_reconnnect_info_mem(dev);
+
 	dev->connected = true;
 	mutex_unlock(&dev->lock);
 
@@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
 	ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
 	if (ret)
 		goto err_vqs;
+	ret = vduse_alloc_reconnnect_info_mem(dev);
+	if (ret < 0)
+		goto err_mem;
 
 	__module_get(THIS_MODULE);
 
 	return 0;
 err_vqs:
 	device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
+err_mem:
+	vduse_free_reconnnect_info_mem(dev);
 err_dev:
 	idr_remove(&vduse_idr, dev->minor);
 err_idr:
-- 
2.43.0

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Michael S. Tsirkin]

On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> Add the function vduse_alloc_reconnnect_info_mem
> and vduse_alloc_reconnnect_info_mem
> These functions allow vduse to allocate and free memory for reconnection
> information. The amount of memory allocated is vq_num pages.
> Each VQS will map its own page where the reconnection information will be saved
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index ef3c9681941e..2da659d5f4a8 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -65,6 +65,7 @@ struct vduse_virtqueue {
>  	int irq_effective_cpu;
>  	struct cpumask irq_affinity;
>  	struct kobject kobj;
> +	unsigned long vdpa_reconnect_vaddr;
>  };
>  
>  struct vduse_dev;
> @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
>  
>  	vq->irq_effective_cpu = curr_cpu;
>  }
> +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> +{
> +	unsigned long vaddr = 0;
> +	struct vduse_virtqueue *vq;
> +
> +	for (int i = 0; i < dev->vq_num; i++) {
> +		/*page 0~ vq_num save the reconnect info for vq*/
> +		vq = dev->vqs[i];
> +		vaddr = get_zeroed_page(GFP_KERNEL);


I don't get why you insist on stealing kernel memory for something
that is just used by userspace to store data for its own use.
Userspace does not lack ways to persist data, for example,
create a regular file anywhere in the filesystem.



> +		if (vaddr == 0)
> +			return -ENOMEM;
> +
> +		vq->vdpa_reconnect_vaddr = vaddr;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> +{
> +	struct vduse_virtqueue *vq;
> +
> +	for (int i = 0; i < dev->vq_num; i++) {
> +		vq = dev->vqs[i];
> +
> +		if (vq->vdpa_reconnect_vaddr)
> +			free_page(vq->vdpa_reconnect_vaddr);
> +		vq->vdpa_reconnect_vaddr = 0;
> +	}
> +
> +	return 0;
> +}
>  
>  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>  			    unsigned long arg)
> @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
>  		mutex_unlock(&dev->lock);
>  		return -EBUSY;
>  	}
> +	vduse_free_reconnnect_info_mem(dev);
> +
>  	dev->connected = true;
>  	mutex_unlock(&dev->lock);
>  
> @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>  	ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
>  	if (ret)
>  		goto err_vqs;
> +	ret = vduse_alloc_reconnnect_info_mem(dev);
> +	if (ret < 0)
> +		goto err_mem;
>  
>  	__module_get(THIS_MODULE);
>  
>  	return 0;
>  err_vqs:
>  	device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> +err_mem:
> +	vduse_free_reconnnect_info_mem(dev);
>  err_dev:
>  	idr_remove(&vduse_idr, dev->minor);
>  err_idr:
> -- 
> 2.43.0

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Jason Wang]

On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > Add the function vduse_alloc_reconnnect_info_mem
> > and vduse_alloc_reconnnect_info_mem
> > These functions allow vduse to allocate and free memory for reconnection
> > information. The amount of memory allocated is vq_num pages.
> > Each VQS will map its own page where the reconnection information will be saved
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index ef3c9681941e..2da659d5f4a8 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> >       int irq_effective_cpu;
> >       struct cpumask irq_affinity;
> >       struct kobject kobj;
> > +     unsigned long vdpa_reconnect_vaddr;
> >  };
> >
> >  struct vduse_dev;
> > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> >
> >       vq->irq_effective_cpu = curr_cpu;
> >  }
> > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > +{
> > +     unsigned long vaddr = 0;
> > +     struct vduse_virtqueue *vq;
> > +
> > +     for (int i = 0; i < dev->vq_num; i++) {
> > +             /*page 0~ vq_num save the reconnect info for vq*/
> > +             vq = dev->vqs[i];
> > +             vaddr = get_zeroed_page(GFP_KERNEL);
>
>
> I don't get why you insist on stealing kernel memory for something
> that is just used by userspace to store data for its own use.
> Userspace does not lack ways to persist data, for example,
> create a regular file anywhere in the filesystem.

Good point. So the motivation here is to:

1) be self contained, no dependency for high speed persist data
storage like tmpfs
2) standardize the format in uAPI which allows reconnection from
arbitrary userspace, unfortunately, such effort was removed in new
versions

If the above doesn't make sense, we don't need to offer those pages by VDUSE.

Thanks


>
>
>
> > +             if (vaddr == 0)
> > +                     return -ENOMEM;
> > +
> > +             vq->vdpa_reconnect_vaddr = vaddr;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > +{
> > +     struct vduse_virtqueue *vq;
> > +
> > +     for (int i = 0; i < dev->vq_num; i++) {
> > +             vq = dev->vqs[i];
> > +
> > +             if (vq->vdpa_reconnect_vaddr)
> > +                     free_page(vq->vdpa_reconnect_vaddr);
> > +             vq->vdpa_reconnect_vaddr = 0;
> > +     }
> > +
> > +     return 0;
> > +}
> >
> >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >                           unsigned long arg)
> > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> >               mutex_unlock(&dev->lock);
> >               return -EBUSY;
> >       }
> > +     vduse_free_reconnnect_info_mem(dev);
> > +
> >       dev->connected = true;
> >       mutex_unlock(&dev->lock);
> >
> > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> >       if (ret)
> >               goto err_vqs;
> > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > +     if (ret < 0)
> > +             goto err_mem;
> >
> >       __module_get(THIS_MODULE);
> >
> >       return 0;
> >  err_vqs:
> >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > +err_mem:
> > +     vduse_free_reconnnect_info_mem(dev);
> >  err_dev:
> >       idr_remove(&vduse_idr, dev->minor);
> >  err_idr:
> > --
> > 2.43.0
>

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Michael S. Tsirkin]

On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > Add the function vduse_alloc_reconnnect_info_mem
> > > and vduse_alloc_reconnnect_info_mem
> > > These functions allow vduse to allocate and free memory for reconnection
> > > information. The amount of memory allocated is vq_num pages.
> > > Each VQS will map its own page where the reconnection information will be saved
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > >  1 file changed, 40 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index ef3c9681941e..2da659d5f4a8 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > >       int irq_effective_cpu;
> > >       struct cpumask irq_affinity;
> > >       struct kobject kobj;
> > > +     unsigned long vdpa_reconnect_vaddr;
> > >  };
> > >
> > >  struct vduse_dev;
> > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > >
> > >       vq->irq_effective_cpu = curr_cpu;
> > >  }
> > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > +{
> > > +     unsigned long vaddr = 0;
> > > +     struct vduse_virtqueue *vq;
> > > +
> > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > +             vq = dev->vqs[i];
> > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> >
> >
> > I don't get why you insist on stealing kernel memory for something
> > that is just used by userspace to store data for its own use.
> > Userspace does not lack ways to persist data, for example,
> > create a regular file anywhere in the filesystem.
> 
> Good point. So the motivation here is to:
> 
> 1) be self contained, no dependency for high speed persist data
> storage like tmpfs

No idea what this means.

> 2) standardize the format in uAPI which allows reconnection from
> arbitrary userspace, unfortunately, such effort was removed in new
> versions

And I don't see why that has to live in the kernel tree either.

> If the above doesn't make sense, we don't need to offer those pages by VDUSE.
> 
> Thanks
> 
> 
> >
> >
> >
> > > +             if (vaddr == 0)
> > > +                     return -ENOMEM;
> > > +
> > > +             vq->vdpa_reconnect_vaddr = vaddr;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > > +{
> > > +     struct vduse_virtqueue *vq;
> > > +
> > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > +             vq = dev->vqs[i];
> > > +
> > > +             if (vq->vdpa_reconnect_vaddr)
> > > +                     free_page(vq->vdpa_reconnect_vaddr);
> > > +             vq->vdpa_reconnect_vaddr = 0;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > >
> > >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > >                           unsigned long arg)
> > > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> > >               mutex_unlock(&dev->lock);
> > >               return -EBUSY;
> > >       }
> > > +     vduse_free_reconnnect_info_mem(dev);
> > > +
> > >       dev->connected = true;
> > >       mutex_unlock(&dev->lock);
> > >
> > > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> > >       if (ret)
> > >               goto err_vqs;
> > > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > > +     if (ret < 0)
> > > +             goto err_mem;
> > >
> > >       __module_get(THIS_MODULE);
> > >
> > >       return 0;
> > >  err_vqs:
> > >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > > +err_mem:
> > > +     vduse_free_reconnnect_info_mem(dev);
> > >  err_dev:
> > >       idr_remove(&vduse_idr, dev->minor);
> > >  err_idr:
> > > --
> > > 2.43.0
> >

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Jason Wang]

On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > and vduse_alloc_reconnnect_info_mem
> > > > These functions allow vduse to allocate and free memory for reconnection
> > > > information. The amount of memory allocated is vq_num pages.
> > > > Each VQS will map its own page where the reconnection information will be saved
> > > >
> > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > ---
> > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 40 insertions(+)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > >       int irq_effective_cpu;
> > > >       struct cpumask irq_affinity;
> > > >       struct kobject kobj;
> > > > +     unsigned long vdpa_reconnect_vaddr;
> > > >  };
> > > >
> > > >  struct vduse_dev;
> > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > >
> > > >       vq->irq_effective_cpu = curr_cpu;
> > > >  }
> > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > +{
> > > > +     unsigned long vaddr = 0;
> > > > +     struct vduse_virtqueue *vq;
> > > > +
> > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > +             vq = dev->vqs[i];
> > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > >
> > >
> > > I don't get why you insist on stealing kernel memory for something
> > > that is just used by userspace to store data for its own use.
> > > Userspace does not lack ways to persist data, for example,
> > > create a regular file anywhere in the filesystem.
> >
> > Good point. So the motivation here is to:
> >
> > 1) be self contained, no dependency for high speed persist data
> > storage like tmpfs
>
> No idea what this means.

I mean a regular file may slow down the datapath performance, so
usually the application will try to use tmpfs and other which is a
dependency for implementing the reconnection.

>
> > 2) standardize the format in uAPI which allows reconnection from
> > arbitrary userspace, unfortunately, such effort was removed in new
> > versions
>
> And I don't see why that has to live in the kernel tree either.

I can't find a better place, any idea?

Thanks

>
> > If the above doesn't make sense, we don't need to offer those pages by VDUSE.
> >
> > Thanks
> >
> >
> > >
> > >
> > >
> > > > +             if (vaddr == 0)
> > > > +                     return -ENOMEM;
> > > > +
> > > > +             vq->vdpa_reconnect_vaddr = vaddr;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > > > +{
> > > > +     struct vduse_virtqueue *vq;
> > > > +
> > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > +             vq = dev->vqs[i];
> > > > +
> > > > +             if (vq->vdpa_reconnect_vaddr)
> > > > +                     free_page(vq->vdpa_reconnect_vaddr);
> > > > +             vq->vdpa_reconnect_vaddr = 0;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > >
> > > >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > >                           unsigned long arg)
> > > > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> > > >               mutex_unlock(&dev->lock);
> > > >               return -EBUSY;
> > > >       }
> > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > +
> > > >       dev->connected = true;
> > > >       mutex_unlock(&dev->lock);
> > > >
> > > > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> > > >       if (ret)
> > > >               goto err_vqs;
> > > > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > > > +     if (ret < 0)
> > > > +             goto err_mem;
> > > >
> > > >       __module_get(THIS_MODULE);
> > > >
> > > >       return 0;
> > > >  err_vqs:
> > > >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > > > +err_mem:
> > > > +     vduse_free_reconnnect_info_mem(dev);
> > > >  err_dev:
> > > >       idr_remove(&vduse_idr, dev->minor);
> > > >  err_idr:
> > > > --
> > > > 2.43.0
> > >
>

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Michael S. Tsirkin]

On Tue, Apr 23, 2024 at 11:09:59AM +0800, Jason Wang wrote:
> On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > > and vduse_alloc_reconnnect_info_mem
> > > > > These functions allow vduse to allocate and free memory for reconnection
> > > > > information. The amount of memory allocated is vq_num pages.
> > > > > Each VQS will map its own page where the reconnection information will be saved
> > > > >
> > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > ---
> > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > > >  1 file changed, 40 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > > >       int irq_effective_cpu;
> > > > >       struct cpumask irq_affinity;
> > > > >       struct kobject kobj;
> > > > > +     unsigned long vdpa_reconnect_vaddr;
> > > > >  };
> > > > >
> > > > >  struct vduse_dev;
> > > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > > >
> > > > >       vq->irq_effective_cpu = curr_cpu;
> > > > >  }
> > > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > +{
> > > > > +     unsigned long vaddr = 0;
> > > > > +     struct vduse_virtqueue *vq;
> > > > > +
> > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > > +             vq = dev->vqs[i];
> > > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > > >
> > > >
> > > > I don't get why you insist on stealing kernel memory for something
> > > > that is just used by userspace to store data for its own use.
> > > > Userspace does not lack ways to persist data, for example,
> > > > create a regular file anywhere in the filesystem.
> > >
> > > Good point. So the motivation here is to:
> > >
> > > 1) be self contained, no dependency for high speed persist data
> > > storage like tmpfs
> >
> > No idea what this means.
> 
> I mean a regular file may slow down the datapath performance, so
> usually the application will try to use tmpfs and other which is a
> dependency for implementing the reconnection.

Are we worried about systems without tmpfs now?


> >
> > > 2) standardize the format in uAPI which allows reconnection from
> > > arbitrary userspace, unfortunately, such effort was removed in new
> > > versions
> >
> > And I don't see why that has to live in the kernel tree either.
> 
> I can't find a better place, any idea?
> 
> Thanks


Well anywhere on github really. with libvhost-user maybe?
It's harmless enough in Documentation
if you like but ties you to the kernel release cycle in a way that
is completely unnecessary.

> >
> > > If the above doesn't make sense, we don't need to offer those pages by VDUSE.
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >
> > > >
> > > > > +             if (vaddr == 0)
> > > > > +                     return -ENOMEM;
> > > > > +
> > > > > +             vq->vdpa_reconnect_vaddr = vaddr;
> > > > > +     }
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > +{
> > > > > +     struct vduse_virtqueue *vq;
> > > > > +
> > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > +             vq = dev->vqs[i];
> > > > > +
> > > > > +             if (vq->vdpa_reconnect_vaddr)
> > > > > +                     free_page(vq->vdpa_reconnect_vaddr);
> > > > > +             vq->vdpa_reconnect_vaddr = 0;
> > > > > +     }
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > >
> > > > >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > >                           unsigned long arg)
> > > > > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> > > > >               mutex_unlock(&dev->lock);
> > > > >               return -EBUSY;
> > > > >       }
> > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > +
> > > > >       dev->connected = true;
> > > > >       mutex_unlock(&dev->lock);
> > > > >
> > > > > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> > > > >       if (ret)
> > > > >               goto err_vqs;
> > > > > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > > > > +     if (ret < 0)
> > > > > +             goto err_mem;
> > > > >
> > > > >       __module_get(THIS_MODULE);
> > > > >
> > > > >       return 0;
> > > > >  err_vqs:
> > > > >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > > > > +err_mem:
> > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > >  err_dev:
> > > > >       idr_remove(&vduse_idr, dev->minor);
> > > > >  err_idr:
> > > > > --
> > > > > 2.43.0
> > > >
> >

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Jason Wang]

On Tue, Apr 23, 2024 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 23, 2024 at 11:09:59AM +0800, Jason Wang wrote:
> > On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > > > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > > > and vduse_alloc_reconnnect_info_mem
> > > > > > These functions allow vduse to allocate and free memory for reconnection
> > > > > > information. The amount of memory allocated is vq_num pages.
> > > > > > Each VQS will map its own page where the reconnection information will be saved
> > > > > >
> > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > ---
> > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 40 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > > > >       int irq_effective_cpu;
> > > > > >       struct cpumask irq_affinity;
> > > > > >       struct kobject kobj;
> > > > > > +     unsigned long vdpa_reconnect_vaddr;
> > > > > >  };
> > > > > >
> > > > > >  struct vduse_dev;
> > > > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > > > >
> > > > > >       vq->irq_effective_cpu = curr_cpu;
> > > > > >  }
> > > > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > +{
> > > > > > +     unsigned long vaddr = 0;
> > > > > > +     struct vduse_virtqueue *vq;
> > > > > > +
> > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > > > +             vq = dev->vqs[i];
> > > > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > > > >
> > > > >
> > > > > I don't get why you insist on stealing kernel memory for something
> > > > > that is just used by userspace to store data for its own use.
> > > > > Userspace does not lack ways to persist data, for example,
> > > > > create a regular file anywhere in the filesystem.
> > > >
> > > > Good point. So the motivation here is to:
> > > >
> > > > 1) be self contained, no dependency for high speed persist data
> > > > storage like tmpfs
> > >
> > > No idea what this means.
> >
> > I mean a regular file may slow down the datapath performance, so
> > usually the application will try to use tmpfs and other which is a
> > dependency for implementing the reconnection.
>
> Are we worried about systems without tmpfs now?

Yes.

>
>
> > >
> > > > 2) standardize the format in uAPI which allows reconnection from
> > > > arbitrary userspace, unfortunately, such effort was removed in new
> > > > versions
> > >
> > > And I don't see why that has to live in the kernel tree either.
> >
> > I can't find a better place, any idea?
> >
> > Thanks
>
>
> Well anywhere on github really. with libvhost-user maybe?
> It's harmless enough in Documentation
> if you like but ties you to the kernel release cycle in a way that
> is completely unnecessary.

Ok.

Thanks

>
> > >
> > > > If the above doesn't make sense, we don't need to offer those pages by VDUSE.
> > > >
> > > > Thanks
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > > > +             if (vaddr == 0)
> > > > > > +                     return -ENOMEM;
> > > > > > +
> > > > > > +             vq->vdpa_reconnect_vaddr = vaddr;
> > > > > > +     }
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > +{
> > > > > > +     struct vduse_virtqueue *vq;
> > > > > > +
> > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > +             vq = dev->vqs[i];
> > > > > > +
> > > > > > +             if (vq->vdpa_reconnect_vaddr)
> > > > > > +                     free_page(vq->vdpa_reconnect_vaddr);
> > > > > > +             vq->vdpa_reconnect_vaddr = 0;
> > > > > > +     }
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > >
> > > > > >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > > >                           unsigned long arg)
> > > > > > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> > > > > >               mutex_unlock(&dev->lock);
> > > > > >               return -EBUSY;
> > > > > >       }
> > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > > +
> > > > > >       dev->connected = true;
> > > > > >       mutex_unlock(&dev->lock);
> > > > > >
> > > > > > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> > > > > >       if (ret)
> > > > > >               goto err_vqs;
> > > > > > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > > > > > +     if (ret < 0)
> > > > > > +             goto err_mem;
> > > > > >
> > > > > >       __module_get(THIS_MODULE);
> > > > > >
> > > > > >       return 0;
> > > > > >  err_vqs:
> > > > > >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > > > > > +err_mem:
> > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > >  err_dev:
> > > > > >       idr_remove(&vduse_idr, dev->minor);
> > > > > >  err_idr:
> > > > > > --
> > > > > > 2.43.0
> > > > >
> > >
>

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Cindy Lu]

On Wed, Apr 24, 2024 at 8:44 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Apr 23, 2024 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Apr 23, 2024 at 11:09:59AM +0800, Jason Wang wrote:
> > > On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > > > > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > > > > and vduse_alloc_reconnnect_info_mem
> > > > > > > These functions allow vduse to allocate and free memory for reconnection
> > > > > > > information. The amount of memory allocated is vq_num pages.
> > > > > > > Each VQS will map its own page where the reconnection information will be saved
> > > > > > >
> > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > > ---
> > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 40 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > > > > >       int irq_effective_cpu;
> > > > > > >       struct cpumask irq_affinity;
> > > > > > >       struct kobject kobj;
> > > > > > > +     unsigned long vdpa_reconnect_vaddr;
> > > > > > >  };
> > > > > > >
> > > > > > >  struct vduse_dev;
> > > > > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > > > > >
> > > > > > >       vq->irq_effective_cpu = curr_cpu;
> > > > > > >  }
> > > > > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > +{
> > > > > > > +     unsigned long vaddr = 0;
> > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > +
> > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > > > > +             vq = dev->vqs[i];
> > > > > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > > > > >
> > > > > >
> > > > > > I don't get why you insist on stealing kernel memory for something
> > > > > > that is just used by userspace to store data for its own use.
> > > > > > Userspace does not lack ways to persist data, for example,
> > > > > > create a regular file anywhere in the filesystem.
> > > > >
> > > > > Good point. So the motivation here is to:
> > > > >
> > > > > 1) be self contained, no dependency for high speed persist data
> > > > > storage like tmpfs
> > > >
> > > > No idea what this means.
> > >
> > > I mean a regular file may slow down the datapath performance, so
> > > usually the application will try to use tmpfs and other which is a
> > > dependency for implementing the reconnection.
> >
> > Are we worried about systems without tmpfs now?
>
> Yes.
>
> >
> >
> > > >
> > > > > 2) standardize the format in uAPI which allows reconnection from
> > > > > arbitrary userspace, unfortunately, such effort was removed in new
> > > > > versions
> > > >
> > > > And I don't see why that has to live in the kernel tree either.
> > >
> > > I can't find a better place, any idea?
> > >
> > > Thanks
> >
> >
> > Well anywhere on github really. with libvhost-user maybe?
> > It's harmless enough in Documentation
> > if you like but ties you to the kernel release cycle in a way that
> > is completely unnecessary.
>
> Ok.
>
> Thanks
>
Sure, got it. Do we need to withdraw all the series? maybe we can keep
the patch for support ioctl to get status and configure?

thanks
cindy

> >
> > > >
> > > > > If the above doesn't make sense, we don't need to offer those pages by VDUSE.
> > > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > +             if (vaddr == 0)
> > > > > > > +                     return -ENOMEM;
> > > > > > > +
> > > > > > > +             vq->vdpa_reconnect_vaddr = vaddr;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > +{
> > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > +
> > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > +             vq = dev->vqs[i];
> > > > > > > +
> > > > > > > +             if (vq->vdpa_reconnect_vaddr)
> > > > > > > +                     free_page(vq->vdpa_reconnect_vaddr);
> > > > > > > +             vq->vdpa_reconnect_vaddr = 0;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > >
> > > > > > >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > > > >                           unsigned long arg)
> > > > > > > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> > > > > > >               mutex_unlock(&dev->lock);
> > > > > > >               return -EBUSY;
> > > > > > >       }
> > > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > > > +
> > > > > > >       dev->connected = true;
> > > > > > >       mutex_unlock(&dev->lock);
> > > > > > >
> > > > > > > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> > > > > > >       if (ret)
> > > > > > >               goto err_vqs;
> > > > > > > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > > > > > > +     if (ret < 0)
> > > > > > > +             goto err_mem;
> > > > > > >
> > > > > > >       __module_get(THIS_MODULE);
> > > > > > >
> > > > > > >       return 0;
> > > > > > >  err_vqs:
> > > > > > >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > > > > > > +err_mem:
> > > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > > >  err_dev:
> > > > > > >       idr_remove(&vduse_idr, dev->minor);
> > > > > > >  err_idr:
> > > > > > > --
> > > > > > > 2.43.0
> > > > > >
> > > >
> >
>

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Jason Wang]

On Wed, Apr 24, 2024 at 11:15 AM Cindy Lu <lulu@redhat.com> wrote:
>
> On Wed, Apr 24, 2024 at 8:44 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Apr 23, 2024 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Apr 23, 2024 at 11:09:59AM +0800, Jason Wang wrote:
> > > > On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > > > > > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > > > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > > > > > and vduse_alloc_reconnnect_info_mem
> > > > > > > > These functions allow vduse to allocate and free memory for reconnection
> > > > > > > > information. The amount of memory allocated is vq_num pages.
> > > > > > > > Each VQS will map its own page where the reconnection information will be saved
> > > > > > > >
> > > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > > > ---
> > > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 40 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > > > > > >       int irq_effective_cpu;
> > > > > > > >       struct cpumask irq_affinity;
> > > > > > > >       struct kobject kobj;
> > > > > > > > +     unsigned long vdpa_reconnect_vaddr;
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  struct vduse_dev;
> > > > > > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > > > > > >
> > > > > > > >       vq->irq_effective_cpu = curr_cpu;
> > > > > > > >  }
> > > > > > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > > +{
> > > > > > > > +     unsigned long vaddr = 0;
> > > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > > +
> > > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > > > > > +             vq = dev->vqs[i];
> > > > > > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > > > > > >
> > > > > > >
> > > > > > > I don't get why you insist on stealing kernel memory for something
> > > > > > > that is just used by userspace to store data for its own use.
> > > > > > > Userspace does not lack ways to persist data, for example,
> > > > > > > create a regular file anywhere in the filesystem.
> > > > > >
> > > > > > Good point. So the motivation here is to:
> > > > > >
> > > > > > 1) be self contained, no dependency for high speed persist data
> > > > > > storage like tmpfs
> > > > >
> > > > > No idea what this means.
> > > >
> > > > I mean a regular file may slow down the datapath performance, so
> > > > usually the application will try to use tmpfs and other which is a
> > > > dependency for implementing the reconnection.
> > >
> > > Are we worried about systems without tmpfs now?
> >
> > Yes.
> >
> > >
> > >
> > > > >
> > > > > > 2) standardize the format in uAPI which allows reconnection from
> > > > > > arbitrary userspace, unfortunately, such effort was removed in new
> > > > > > versions
> > > > >
> > > > > And I don't see why that has to live in the kernel tree either.
> > > >
> > > > I can't find a better place, any idea?
> > > >
> > > > Thanks
> > >
> > >
> > > Well anywhere on github really. with libvhost-user maybe?
> > > It's harmless enough in Documentation
> > > if you like but ties you to the kernel release cycle in a way that
> > > is completely unnecessary.
> >
> > Ok.
> >
> > Thanks
> >
> Sure, got it. Do we need to withdraw all the series? maybe we can keep
> the patch for support ioctl to get status and configure?

We can leave the mmap part for future, the rest is still useful unless
I miss anything.

Thanks

>
> thanks
> cindy
>
> > >
> > > > >
> > > > > > If the above doesn't make sense, we don't need to offer those pages by VDUSE.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > +             if (vaddr == 0)
> > > > > > > > +                     return -ENOMEM;
> > > > > > > > +
> > > > > > > > +             vq->vdpa_reconnect_vaddr = vaddr;
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > > +{
> > > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > > +
> > > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > > +             vq = dev->vqs[i];
> > > > > > > > +
> > > > > > > > +             if (vq->vdpa_reconnect_vaddr)
> > > > > > > > +                     free_page(vq->vdpa_reconnect_vaddr);
> > > > > > > > +             vq->vdpa_reconnect_vaddr = 0;
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     return 0;
> > > > > > > > +}
> > > > > > > >
> > > > > > > >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > > > > >                           unsigned long arg)
> > > > > > > > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> > > > > > > >               mutex_unlock(&dev->lock);
> > > > > > > >               return -EBUSY;
> > > > > > > >       }
> > > > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > > > > +
> > > > > > > >       dev->connected = true;
> > > > > > > >       mutex_unlock(&dev->lock);
> > > > > > > >
> > > > > > > > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > > >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> > > > > > > >       if (ret)
> > > > > > > >               goto err_vqs;
> > > > > > > > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > > > > > > > +     if (ret < 0)
> > > > > > > > +             goto err_mem;
> > > > > > > >
> > > > > > > >       __module_get(THIS_MODULE);
> > > > > > > >
> > > > > > > >       return 0;
> > > > > > > >  err_vqs:
> > > > > > > >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > > > > > > > +err_mem:
> > > > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > > > >  err_dev:
> > > > > > > >       idr_remove(&vduse_idr, dev->minor);
> > > > > > > >  err_idr:
> > > > > > > > --
> > > > > > > > 2.43.0
> > > > > > >
> > > > >
> > >
> >
>

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Michael S. Tsirkin]

On Wed, Apr 24, 2024 at 08:44:10AM +0800, Jason Wang wrote:
> On Tue, Apr 23, 2024 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Apr 23, 2024 at 11:09:59AM +0800, Jason Wang wrote:
> > > On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > > > > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > > > > and vduse_alloc_reconnnect_info_mem
> > > > > > > These functions allow vduse to allocate and free memory for reconnection
> > > > > > > information. The amount of memory allocated is vq_num pages.
> > > > > > > Each VQS will map its own page where the reconnection information will be saved
> > > > > > >
> > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > > ---
> > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 40 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > > > > >       int irq_effective_cpu;
> > > > > > >       struct cpumask irq_affinity;
> > > > > > >       struct kobject kobj;
> > > > > > > +     unsigned long vdpa_reconnect_vaddr;
> > > > > > >  };
> > > > > > >
> > > > > > >  struct vduse_dev;
> > > > > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > > > > >
> > > > > > >       vq->irq_effective_cpu = curr_cpu;
> > > > > > >  }
> > > > > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > +{
> > > > > > > +     unsigned long vaddr = 0;
> > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > +
> > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > > > > +             vq = dev->vqs[i];
> > > > > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > > > > >
> > > > > >
> > > > > > I don't get why you insist on stealing kernel memory for something
> > > > > > that is just used by userspace to store data for its own use.
> > > > > > Userspace does not lack ways to persist data, for example,
> > > > > > create a regular file anywhere in the filesystem.
> > > > >
> > > > > Good point. So the motivation here is to:
> > > > >
> > > > > 1) be self contained, no dependency for high speed persist data
> > > > > storage like tmpfs
> > > >
> > > > No idea what this means.
> > >
> > > I mean a regular file may slow down the datapath performance, so
> > > usually the application will try to use tmpfs and other which is a
> > > dependency for implementing the reconnection.
> >
> > Are we worried about systems without tmpfs now?
> 
> Yes.

Why? Who ships these?


> >
> >
> > > >
> > > > > 2) standardize the format in uAPI which allows reconnection from
> > > > > arbitrary userspace, unfortunately, such effort was removed in new
> > > > > versions
> > > >
> > > > And I don't see why that has to live in the kernel tree either.
> > >
> > > I can't find a better place, any idea?
> > >
> > > Thanks
> >
> >
> > Well anywhere on github really. with libvhost-user maybe?
> > It's harmless enough in Documentation
> > if you like but ties you to the kernel release cycle in a way that
> > is completely unnecessary.
> 
> Ok.
> 
> Thanks
> 
> >
> > > >
> > > > > If the above doesn't make sense, we don't need to offer those pages by VDUSE.
> > > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > +             if (vaddr == 0)
> > > > > > > +                     return -ENOMEM;
> > > > > > > +
> > > > > > > +             vq->vdpa_reconnect_vaddr = vaddr;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > +{
> > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > +
> > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > +             vq = dev->vqs[i];
> > > > > > > +
> > > > > > > +             if (vq->vdpa_reconnect_vaddr)
> > > > > > > +                     free_page(vq->vdpa_reconnect_vaddr);
> > > > > > > +             vq->vdpa_reconnect_vaddr = 0;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > >
> > > > > > >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > > > >                           unsigned long arg)
> > > > > > > @@ -1672,6 +1705,8 @@ static int vduse_destroy_dev(char *name)
> > > > > > >               mutex_unlock(&dev->lock);
> > > > > > >               return -EBUSY;
> > > > > > >       }
> > > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > > > +
> > > > > > >       dev->connected = true;
> > > > > > >       mutex_unlock(&dev->lock);
> > > > > > >
> > > > > > > @@ -1855,12 +1890,17 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > >       ret = vduse_dev_init_vqs(dev, config->vq_align, config->vq_num);
> > > > > > >       if (ret)
> > > > > > >               goto err_vqs;
> > > > > > > +     ret = vduse_alloc_reconnnect_info_mem(dev);
> > > > > > > +     if (ret < 0)
> > > > > > > +             goto err_mem;
> > > > > > >
> > > > > > >       __module_get(THIS_MODULE);
> > > > > > >
> > > > > > >       return 0;
> > > > > > >  err_vqs:
> > > > > > >       device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> > > > > > > +err_mem:
> > > > > > > +     vduse_free_reconnnect_info_mem(dev);
> > > > > > >  err_dev:
> > > > > > >       idr_remove(&vduse_idr, dev->minor);
> > > > > > >  err_idr:
> > > > > > > --
> > > > > > > 2.43.0
> > > > > >
> > > >
> >

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Jason Wang]

On Wed, Apr 24, 2024 at 5:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Apr 24, 2024 at 08:44:10AM +0800, Jason Wang wrote:
> > On Tue, Apr 23, 2024 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Apr 23, 2024 at 11:09:59AM +0800, Jason Wang wrote:
> > > > On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > > > > > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > > > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > > > > > and vduse_alloc_reconnnect_info_mem
> > > > > > > > These functions allow vduse to allocate and free memory for reconnection
> > > > > > > > information. The amount of memory allocated is vq_num pages.
> > > > > > > > Each VQS will map its own page where the reconnection information will be saved
> > > > > > > >
> > > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > > > ---
> > > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 40 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > > > > > >       int irq_effective_cpu;
> > > > > > > >       struct cpumask irq_affinity;
> > > > > > > >       struct kobject kobj;
> > > > > > > > +     unsigned long vdpa_reconnect_vaddr;
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  struct vduse_dev;
> > > > > > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > > > > > >
> > > > > > > >       vq->irq_effective_cpu = curr_cpu;
> > > > > > > >  }
> > > > > > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > > +{
> > > > > > > > +     unsigned long vaddr = 0;
> > > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > > +
> > > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > > > > > +             vq = dev->vqs[i];
> > > > > > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > > > > > >
> > > > > > >
> > > > > > > I don't get why you insist on stealing kernel memory for something
> > > > > > > that is just used by userspace to store data for its own use.
> > > > > > > Userspace does not lack ways to persist data, for example,
> > > > > > > create a regular file anywhere in the filesystem.
> > > > > >
> > > > > > Good point. So the motivation here is to:
> > > > > >
> > > > > > 1) be self contained, no dependency for high speed persist data
> > > > > > storage like tmpfs
> > > > >
> > > > > No idea what this means.
> > > >
> > > > I mean a regular file may slow down the datapath performance, so
> > > > usually the application will try to use tmpfs and other which is a
> > > > dependency for implementing the reconnection.
> > >
> > > Are we worried about systems without tmpfs now?
> >
> > Yes.
>
> Why? Who ships these?

Not sure, but it could be disabled or unmounted. I'm not sure make
VDUSE depends on TMPFS is a good idea.

Thanks

Re: [PATCH v5 3/5] vduse: Add function to get/free the pages for reconnection

[Michael S. Tsirkin]

On Thu, Apr 25, 2024 at 09:35:58AM +0800, Jason Wang wrote:
> On Wed, Apr 24, 2024 at 5:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Apr 24, 2024 at 08:44:10AM +0800, Jason Wang wrote:
> > > On Tue, Apr 23, 2024 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Apr 23, 2024 at 11:09:59AM +0800, Jason Wang wrote:
> > > > > On Tue, Apr 23, 2024 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Apr 18, 2024 at 08:57:51AM +0800, Jason Wang wrote:
> > > > > > > On Wed, Apr 17, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Apr 12, 2024 at 09:28:23PM +0800, Cindy Lu wrote:
> > > > > > > > > Add the function vduse_alloc_reconnnect_info_mem
> > > > > > > > > and vduse_alloc_reconnnect_info_mem
> > > > > > > > > These functions allow vduse to allocate and free memory for reconnection
> > > > > > > > > information. The amount of memory allocated is vq_num pages.
> > > > > > > > > Each VQS will map its own page where the reconnection information will be saved
> > > > > > > > >
> > > > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 40 ++++++++++++++++++++++++++++++
> > > > > > > > >  1 file changed, 40 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > index ef3c9681941e..2da659d5f4a8 100644
> > > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > @@ -65,6 +65,7 @@ struct vduse_virtqueue {
> > > > > > > > >       int irq_effective_cpu;
> > > > > > > > >       struct cpumask irq_affinity;
> > > > > > > > >       struct kobject kobj;
> > > > > > > > > +     unsigned long vdpa_reconnect_vaddr;
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > >  struct vduse_dev;
> > > > > > > > > @@ -1105,6 +1106,38 @@ static void vduse_vq_update_effective_cpu(struct vduse_virtqueue *vq)
> > > > > > > > >
> > > > > > > > >       vq->irq_effective_cpu = curr_cpu;
> > > > > > > > >  }
> > > > > > > > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > > > > > > > > +{
> > > > > > > > > +     unsigned long vaddr = 0;
> > > > > > > > > +     struct vduse_virtqueue *vq;
> > > > > > > > > +
> > > > > > > > > +     for (int i = 0; i < dev->vq_num; i++) {
> > > > > > > > > +             /*page 0~ vq_num save the reconnect info for vq*/
> > > > > > > > > +             vq = dev->vqs[i];
> > > > > > > > > +             vaddr = get_zeroed_page(GFP_KERNEL);
> > > > > > > >
> > > > > > > >
> > > > > > > > I don't get why you insist on stealing kernel memory for something
> > > > > > > > that is just used by userspace to store data for its own use.
> > > > > > > > Userspace does not lack ways to persist data, for example,
> > > > > > > > create a regular file anywhere in the filesystem.
> > > > > > >
> > > > > > > Good point. So the motivation here is to:
> > > > > > >
> > > > > > > 1) be self contained, no dependency for high speed persist data
> > > > > > > storage like tmpfs
> > > > > >
> > > > > > No idea what this means.
> > > > >
> > > > > I mean a regular file may slow down the datapath performance, so
> > > > > usually the application will try to use tmpfs and other which is a
> > > > > dependency for implementing the reconnection.
> > > >
> > > > Are we worried about systems without tmpfs now?
> > >
> > > Yes.
> >
> > Why? Who ships these?
> 
> Not sure, but it could be disabled or unmounted. I'm not sure make
> VDUSE depends on TMPFS is a good idea.
> 
> Thanks

Don't disable or unmount it then?
The use-case needs to be much clearer if we are adding a way for
userspace to pin kernel memory for unlimited time.

-- 
MST

[PATCH v5 4/5] vduse: Add file operation for mmap

[Cindy Lu]

Add the operation for mmap, This function  will be used by the user space
application to map the pages to the user space.

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

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 2da659d5f4a8..7abe0c17cf0e 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1465,6 +1465,61 @@ static struct vduse_dev *vduse_dev_get_from_minor(int minor)
 	return dev;
 }
 
+static vm_fault_t vduse_vm_fault(struct vm_fault *vmf)
+{
+	struct vduse_dev *dev = vmf->vma->vm_file->private_data;
+	struct vm_area_struct *vma = vmf->vma;
+	u16 index = vma->vm_pgoff;
+	struct vduse_virtqueue *vq;
+	unsigned long vaddr;
+
+	/* index 0~vq_number-1 page reserved for virtqueue state*/
+	vq = dev->vqs[index];
+	vaddr = vq->vdpa_reconnect_vaddr;
+	if (remap_pfn_range(vma, vmf->address & PAGE_MASK,
+			    PFN_DOWN(virt_to_phys((void *)vaddr)), PAGE_SIZE,
+			    vma->vm_page_prot))
+		return VM_FAULT_SIGBUS;
+	return VM_FAULT_NOPAGE;
+}
+
+static const struct vm_operations_struct vduse_vm_ops = {
+	.fault = vduse_vm_fault,
+};
+
+static int vduse_dev_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct vduse_dev *dev = file->private_data;
+	unsigned long vaddr = 0;
+	unsigned long index = vma->vm_pgoff;
+	struct vduse_virtqueue *vq;
+
+	if (vma->vm_end - vma->vm_start != PAGE_SIZE)
+		return -EINVAL;
+	if ((vma->vm_flags & VM_SHARED) == 0)
+		return -EINVAL;
+	if (index >= dev->vq_num)
+		return -EINVAL;
+
+	index = array_index_nospec(index, dev->vq_num);
+	vq = dev->vqs[index];
+	vaddr = vq->vdpa_reconnect_vaddr;
+	if (vaddr == 0)
+		return -EOPNOTSUPP;
+
+	if (virt_to_phys((void *)vaddr) & (PAGE_SIZE - 1))
+		return -EINVAL;
+
+	/* Check if the Userspace App mapped the correct size */
+	if (vma->vm_end - vma->vm_start != PAGE_SIZE)
+		return -EOPNOTSUPP;
+
+	vm_flags_set(vma, VM_DONTEXPAND);
+	vma->vm_ops = &vduse_vm_ops;
+
+	return 0;
+}
+
 static int vduse_dev_open(struct inode *inode, struct file *file)
 {
 	int ret;
@@ -1497,6 +1552,8 @@ static const struct file_operations vduse_dev_fops = {
 	.unlocked_ioctl	= vduse_dev_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 	.llseek		= noop_llseek,
+	.mmap		= vduse_dev_mmap,
+
 };
 
 static ssize_t irq_cb_affinity_show(struct vduse_virtqueue *vq, char *buf)
-- 
2.43.0

[PATCH v5 5/5] Documentation: Add reconnect process for VDUSE

[Cindy Lu]

Add a document explaining the reconnect process, including what the
Userspace App needs to do and how it works with the kernel.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 Documentation/userspace-api/vduse.rst | 41 +++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/userspace-api/vduse.rst b/Documentation/userspace-api/vduse.rst
index bdb880e01132..7faa83462e78 100644
--- a/Documentation/userspace-api/vduse.rst
+++ b/Documentation/userspace-api/vduse.rst
@@ -231,3 +231,44 @@ able to start the dataplane processing as follows:
    after the used ring is filled.
 
 For more details on the uAPI, please see include/uapi/linux/vduse.h.
+
+HOW VDUSE devices reconnection works
+------------------------------------
+1. What is reconnection?
+
+   When the userspace application loads, it should establish a connection
+   to the vduse kernel device. Sometimes,the userspace application exists,
+   and we want to support its restart and connect to the kernel device again
+
+2. How can I support reconnection in a userspace application?
+
+2.1 During initialization, the userspace application should first verify the
+    existence of the device "/dev/vduse/vduse_name".
+    If it doesn't exist, it means this is the first-time for connection. goto step 2.2
+    If it exists, it means this is a reconnection, and we should goto step 2.3
+
+2.2 Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
+    /dev/vduse/control.
+    When ioctl(VDUSE_CREATE_DEV) is called, kernel allocates memory for
+    the reconnect information. The total memory size is PAGE_SIZE*vq_mumber.
+
+2.3 Check if the information is suitable for reconnect
+    If this is reconnection :
+    Before attempting to reconnect, The userspace application needs to use the
+    ioctl(VDUSE_DEV_GET_CONFIG, VDUSE_DEV_GET_STATUS, VDUSE_DEV_GET_FEATURES...)
+    to get the information from kernel.
+    Please review the information and confirm if it is suitable to reconnect.
+
+2.4 Userspace application needs to mmap the memory to userspace
+    The userspace application requires mapping one page for every vq. These pages
+    should be used to save vq-related information during system running. Additionally,
+    the application must define its own structure to store information for reconnection.
+
+2.5 Completed the initialization and running the application.
+    While the application is running, it is important to store relevant information
+    about reconnections in mapped pages. When calling the ioctl VDUSE_VQ_GET_INFO to
+    get vq information, it's necessary to check whether it's a reconnection. If it is
+    a reconnection, the vq-related information must be get from the mapped pages.
+
+2.6 When the Userspace application exits, it is necessary to unmap all the
+    pages for reconnection
-- 
2.43.0

Re: [PATCH v5 5/5] Documentation: Add reconnect process for VDUSE

[Jason Wang]

On Fri, Apr 12, 2024 at 9:31 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add a document explaining the reconnect process, including what the
> Userspace App needs to do and how it works with the kernel.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  Documentation/userspace-api/vduse.rst | 41 +++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/Documentation/userspace-api/vduse.rst b/Documentation/userspace-api/vduse.rst
> index bdb880e01132..7faa83462e78 100644
> --- a/Documentation/userspace-api/vduse.rst
> +++ b/Documentation/userspace-api/vduse.rst
> @@ -231,3 +231,44 @@ able to start the dataplane processing as follows:
>     after the used ring is filled.
>
>  For more details on the uAPI, please see include/uapi/linux/vduse.h.
> +
> +HOW VDUSE devices reconnection works
> +------------------------------------
> +1. What is reconnection?
> +
> +   When the userspace application loads, it should establish a connection
> +   to the vduse kernel device. Sometimes,the userspace application exists,

I guess you meant "exists"? If yes, it should be better to say "exits
unexpectedly"

> +   and we want to support its restart and connect to the kernel device again
> +
> +2. How can I support reconnection in a userspace application?

Better to say "How reconnection is supported"?

> +
> +2.1 During initialization, the userspace application should first verify the
> +    existence of the device "/dev/vduse/vduse_name".
> +    If it doesn't exist, it means this is the first-time for connection. goto step 2.2
> +    If it exists, it means this is a reconnection, and we should goto step 2.3
> +
> +2.2 Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
> +    /dev/vduse/control.
> +    When ioctl(VDUSE_CREATE_DEV) is called, kernel allocates memory for
> +    the reconnect information. The total memory size is PAGE_SIZE*vq_mumber.

I think we need to mention that this should be part of the previous
"VDUSE devices are created as follows"?

> +
> +2.3 Check if the information is suitable for reconnect
> +    If this is reconnection :
> +    Before attempting to reconnect, The userspace application needs to use the
> +    ioctl(VDUSE_DEV_GET_CONFIG, VDUSE_DEV_GET_STATUS, VDUSE_DEV_GET_FEATURES...)
> +    to get the information from kernel.
> +    Please review the information and confirm if it is suitable to reconnect.

Need to define "review" here and how to decide if it is not suitable
to reconnect.

> +
> +2.4 Userspace application needs to mmap the memory to userspace
> +    The userspace application requires mapping one page for every vq. These pages
> +    should be used to save vq-related information during system running.

Not a native speaker, but it looks better with

"should be used by the userspace to store virtqueue specific information".

> Additionally,
> +    the application must define its own structure to store information for reconnection.
> +
> +2.5 Completed the initialization and running the application.
> +    While the application is running, it is important to store relevant information
> +    about reconnections in mapped pages.

I think we need some link/code examples to demonstrate what needs to be stored.

> When calling the ioctl VDUSE_VQ_GET_INFO to
> +    get vq information, it's necessary to check whether it's a reconnection.

Better with some examples of codes.

> If it is
> +    a reconnection, the vq-related information must be get from the mapped pages.
> +
> +2.6 When the Userspace application exits, it is necessary to unmap all the
> +    pages for reconnection

This seems to be unnecessary, for example there could be an unexpected exit.

Thanks

> --
> 2.43.0
>

Re: [PATCH v5 5/5] Documentation: Add reconnect process for VDUSE

[Cindy Lu]

On Tue, Apr 16, 2024 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Apr 12, 2024 at 9:31 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Add a document explaining the reconnect process, including what the
> > Userspace App needs to do and how it works with the kernel.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  Documentation/userspace-api/vduse.rst | 41 +++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/vduse.rst b/Documentation/userspace-api/vduse.rst
> > index bdb880e01132..7faa83462e78 100644
> > --- a/Documentation/userspace-api/vduse.rst
> > +++ b/Documentation/userspace-api/vduse.rst
> > @@ -231,3 +231,44 @@ able to start the dataplane processing as follows:
> >     after the used ring is filled.
> >
> >  For more details on the uAPI, please see include/uapi/linux/vduse.h.
> > +
> > +HOW VDUSE devices reconnection works
> > +------------------------------------
> > +1. What is reconnection?
> > +
> > +   When the userspace application loads, it should establish a connection
> > +   to the vduse kernel device. Sometimes,the userspace application exists,
>
> I guess you meant "exists"? If yes, it should be better to say "exits
> unexpectedly"
>
> > +   and we want to support its restart and connect to the kernel device again
> > +
> > +2. How can I support reconnection in a userspace application?
>
> Better to say "How reconnection is supported"?
>
> > +
> > +2.1 During initialization, the userspace application should first verify the
> > +    existence of the device "/dev/vduse/vduse_name".
> > +    If it doesn't exist, it means this is the first-time for connection. goto step 2.2
> > +    If it exists, it means this is a reconnection, and we should goto step 2.3
> > +
> > +2.2 Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
> > +    /dev/vduse/control.
> > +    When ioctl(VDUSE_CREATE_DEV) is called, kernel allocates memory for
> > +    the reconnect information. The total memory size is PAGE_SIZE*vq_mumber.
>
> I think we need to mention that this should be part of the previous
> "VDUSE devices are created as follows"?
>
> > +
> > +2.3 Check if the information is suitable for reconnect
> > +    If this is reconnection :
> > +    Before attempting to reconnect, The userspace application needs to use the
> > +    ioctl(VDUSE_DEV_GET_CONFIG, VDUSE_DEV_GET_STATUS, VDUSE_DEV_GET_FEATURES...)
> > +    to get the information from kernel.
> > +    Please review the information and confirm if it is suitable to reconnect.
>
> Need to define "review" here and how to decide if it is not suitable
> to reconnect.
>
> > +
> > +2.4 Userspace application needs to mmap the memory to userspace
> > +    The userspace application requires mapping one page for every vq. These pages
> > +    should be used to save vq-related information during system running.
>
> Not a native speaker, but it looks better with
>
> "should be used by the userspace to store virtqueue specific information".
>
> > Additionally,
> > +    the application must define its own structure to store information for reconnection.
> > +
> > +2.5 Completed the initialization and running the application.
> > +    While the application is running, it is important to store relevant information
> > +    about reconnections in mapped pages.
>
> I think we need some link/code examples to demonstrate what needs to be stored.
>
> > When calling the ioctl VDUSE_VQ_GET_INFO to
> > +    get vq information, it's necessary to check whether it's a reconnection.
>
> Better with some examples of codes.
>
> > If it is
> > +    a reconnection, the vq-related information must be get from the mapped pages.
> > +
> > +2.6 When the Userspace application exits, it is necessary to unmap all the
> > +    pages for reconnection
>
> This seems to be unnecessary, for example there could be an unexpected exit.
>
> Thanks
>
Thanks Jason, I will send a new version
Thanks
Cindy
> > --
> > 2.43.0
> >
>