Re: [RFC v2 4/4] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO

[Cindy Lu]

On Mon, Sep 25, 2023 at 10:58 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> On Thu, Sep 21, 2023 at 10:07 PM Cindy Lu <lulu@xxxxxxxxxx> wrote:
> >
> > On Mon, Sep 18, 2023 at 4:49 PM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Sep 12, 2023 at 11:01 AM Cindy Lu <lulu@xxxxxxxxxx> wrote:
> > > >
> > > > In VDUSE_GET_RECONNECT_INFO, the Userspace App can get the map size
> > > > and The number of mapping memory pages from the kernel. The userspace
> > > > App can use this information to map the pages.
> > > >
> > > > Signed-off-by: Cindy Lu <lulu@xxxxxxxxxx>
> > > > ---
> > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 15 +++++++++++++++
> > > >  include/uapi/linux/vduse.h         | 15 +++++++++++++++
> > > >  2 files changed, 30 insertions(+)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index 680b23dbdde2..c99f99892b5c 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -1368,6 +1368,21 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > >                 ret = 0;
> > > >                 break;
> > > >         }
> > > > +       case VDUSE_GET_RECONNECT_INFO: {
> > > > +               struct vduse_reconnect_mmap_info info;
> > > > +
> > > > +               ret = -EFAULT;
> > > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > > +                       break;
> > > > +
> > > > +               info.size = PAGE_SIZE;
> > > > +               info.max_index = dev->vq_num + 1;
> > > > +
> > > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > > +                       break;
> > > > +               ret = 0;
> > > > +               break;
> > > > +       }
> > > >         default:
> > > >                 ret = -ENOIOCTLCMD;
> > > >                 break;
> > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > index d585425803fd..ce55e34f63d7 100644
> > > > --- a/include/uapi/linux/vduse.h
> > > > +++ b/include/uapi/linux/vduse.h
> > > > @@ -356,4 +356,19 @@ struct vhost_reconnect_vring {
> > > >         _Bool avail_wrap_counter;
> > > >  };
> > > >
> > > > +/**
> > > > + * struct vduse_reconnect_mmap_info
> > > > + * @size: mapping memory size, always page_size here
> > > > + * @max_index: the number of pages allocated in kernel,just
> > > > + * use for check
> > > > + */
> > > > +
> > > > +struct vduse_reconnect_mmap_info {
> > > > +       __u32 size;
> > > > +       __u32 max_index;
> > > > +};
> > >
> > > One thing I didn't understand is that, aren't the things we used to
> > > store connection info belong to uAPI? If not, how can we make sure the
> > > connections work across different vendors/implementations. If yes,
> > > where?
> > >
> > > Thanks
> > >
> > The process for this reconnecttion  is
> > A.The first-time connection
> > 1> The userland app checks if the device exists
> > 2>  use the ioctl to create the vduse device
> > 3> Mapping the kernel page to userland and save the
> > App-version/features/other information to this page
> > 4>  if the Userland app needs to exit, then the Userland app will only
> > unmap the page and then exit
> >
> > B, the re-connection
> > 1> the userland app finds the device is existing
> > 2> Mapping the kernel page to userland
> > 3> check if the information in shared memory is satisfied to
> > reconnect,if ok then continue to reconnect
> > 4> continue working
> >
> >  For now these information are all from userland,So here the page will
> > be maintained by the userland App
> > in the previous code we only saved the api-version by uAPI .  if  we
> > need to support reconnection maybe we need to add 2 new uAPI for this,
> > one of the uAPI is to save the reconnect  information and another is
> > to get the information
> >
> > maybe something like
> >
> > struct vhost_reconnect_data {
> > uint32_t version;
> > uint64_t features;
> > uint8_t status;
> > struct virtio_net_config config;
> > uint32_t nr_vrings;
> > };
>
> Probably, then we can make sure the re-connection works across
> different vduse-daemon implementations.
>
> >
> > #define VDUSE_GET_RECONNECT_INFO _IOR (VDUSE_BASE, 0x1c, struct
> > vhost_reconnect_data)
> >
> > #define VDUSE_SET_RECONNECT_INFO  _IOWR(VDUSE_BASE, 0x1d, struct
> > vhost_reconnect_data)
>
> Not sure I get this, but the idea is to map those pages to user space,
> any reason we need this uAPI?
>
> Thanks
>
Sorry， I didn't write it clearly, I mean if I we don't want to use the
mmap to sync/check the vduse status, we need to add these 2 new uAPIs,
 these 2 methods are all ok for me.
For the vq related information still need to use the mmap to sync
Thanks
cindy
> >
> > Thanks
> > Cindy
> >
> >
> >
> >
> > > > +
> > > > +#define VDUSE_GET_RECONNECT_INFO \
> > > > +       _IOWR(VDUSE_BASE, 0x1b, struct vduse_reconnect_mmap_info)
> > > > +
> > > >  #endif /* _UAPI_VDUSE_H_ */
> > > > --
> > > > 2.34.3
> > > >
> > >
> >
>