Messages in this thread | | | From | Jason Wang <> | Date | Wed, 18 Oct 2023 10:56:43 +0800 | Subject | Re: [PATCH v1 1/4] vduse: Add function to get/free the pages for reconnection |
| |
On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote: > > Add the function vduse_alloc_reconnnect_info_mem > and vduse_alloc_reconnnect_info_mem > In this 2 function, vduse will get/free (vq_num + 1)*page > Page 0 will be used to save the reconnection information, The > Userspace App will maintain this. Page 1 ~ vq_num + 1 will save > the reconnection information for vqs.
I'd align this with the spec. E.g spec starts virtqueue index from 0.
> > Signed-off-by: Cindy Lu <lulu@redhat.com> > --- > drivers/vdpa/vdpa_user/vduse_dev.c | 78 ++++++++++++++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > index 26b7e29cb900..565126a9ab01 100644 > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > @@ -30,6 +30,10 @@ > #include <uapi/linux/virtio_blk.h> > #include <linux/mod_devicetable.h> > > +#ifdef CONFIG_X86 > +#include <asm/set_memory.h> > +#endif
Why do we need special care for x86?
> + > #include "iova_domain.h" > > #define DRV_AUTHOR "Yongji Xie <xieyongji@bytedance.com>" > @@ -41,6 +45,19 @@ > #define VDUSE_IOVA_SIZE (128 * 1024 * 1024) > #define VDUSE_MSG_DEFAULT_TIMEOUT 30 > > +/* struct vdpa_reconnect_info saved the alloc pages info > + * these pages will mmaped to userspace for reconnection > + */ > +struct vdpa_reconnect_info { > + /* Offset (within vm_file) in PAGE_SIZE > + */ > + u32 index; > + /* virtual address for this page*/ > + unsigned long vaddr; > + /* allocated memory size, */ > + phys_addr_t size;
Anything wrong with using "unsigned long" here.
Btw, what is "allocated memory size" since I think we can use up to PAGE_SIZE here?
> +}; > + > struct vduse_virtqueue { > u16 index; > u16 num_max; > @@ -57,6 +74,7 @@ struct vduse_virtqueue { > struct vdpa_callback cb; > struct work_struct inject; > struct work_struct kick; > + struct vdpa_reconnect_info reconnect_info; > }; > > struct vduse_dev; > @@ -106,6 +124,7 @@ struct vduse_dev { > u32 vq_align; > struct vduse_umem *umem; > struct mutex mem_lock; > + struct vdpa_reconnect_info reconnect_status; > }; > > struct vduse_dev_msg { > @@ -1030,6 +1049,61 @@ static int vduse_dev_reg_umem(struct vduse_dev *dev, > return ret; > } > > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev) > +{ > + struct vdpa_reconnect_info *info; > + struct vduse_virtqueue *vq; > + void *addr; > + > + /*page 0 is use to save status,dpdk will use this to save the information > + *needed in reconnection,kernel don't need to maintain this > + */
Let's avoid mentioning any specific userspace like DPDK in the kernel source. A good uAPI should not be designed for a specific userspace.
> + info = &dev->reconnect_status; > + addr = (void *)get_zeroed_page(GFP_KERNEL); > + if (!addr) > + return -1;
Let's don't use magic number here but something like -ENOMEM.
> + > + info->vaddr = (unsigned long)(addr); > + info->size = PAGE_SIZE; > + /* index is vm Offset in PAGE_SIZE */ > + info->index = 0; > + > + /*page 1~ vq_num + 1 save the reconnect info for vqs*/
There's no need to explain what the code does.
> + for (int i = 0; i < dev->vq_num + 1; i++) { > + vq = &dev->vqs[i]; > + info = &vq->reconnect_info; > + addr = (void *)get_zeroed_page(GFP_KERNEL); > + if (!addr) > + return -1; > + > + info->vaddr = (unsigned long)(addr); > + info->size = PAGE_SIZE; > + info->index = i + 1;
Btw, I don't see an obvious difference in the page for virtqueue and device. Could we unify the logic there? So did the free.
> + } > + > + return 0; > +} > + > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev) > +{ > + struct vdpa_reconnect_info *info; > + struct vduse_virtqueue *vq; > + > + info = &dev->reconnect_status; > + free_page(info->vaddr); > + info->size = 0; > + info->vaddr = 0; > + for (int i = 0; i < dev->vq_num + 1; i++) { > + vq = &dev->vqs[i]; > + info = &vq->reconnect_info; > + free_page(info->vaddr); > + info->size = 0; > + info->vaddr = 0; > + } > + > + return 0; > +} > + > static long vduse_dev_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > @@ -1390,6 +1464,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); > > @@ -1542,6 +1618,8 @@ static int vduse_create_dev(struct vduse_dev_config *config, > ret = PTR_ERR(dev->dev); > goto err_dev; > } > + > + vduse_alloc_reconnnect_info_mem(dev);
The errors were ignored here.
Thanks
> __module_get(THIS_MODULE); > > return 0; > -- > 2.34.3 >
| |