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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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
>