Messages in this thread | | | From | Cindy Lu <> | Date | Tue, 7 Nov 2023 15:26:27 +0800 | Subject | Re: [PATCH v1 1/4] vduse: Add function to get/free the pages for reconnection |
| |
On Wed, Oct 18, 2023 at 10:57 AM Jason Wang <jasowang@redhat.com> wrote: > > 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? > sure, will remove this here Thanks Cindy > > +}; > > + > > 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. > will change this > > + 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. > will change this
> > + 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 > sure will add the check here
Thanks cindy
> > __module_get(THIS_MODULE); > > > > return 0; > > -- > > 2.34.3 > > >
| |