Messages in this thread | | | Date | Thu, 16 Mar 2023 10:11:42 +0100 | From | Stefano Garzarella <> | Subject | Re: [PATCH v2 8/8] vdpa_sim: add support for user VA |
| |
On Tue, Mar 14, 2023 at 01:36:13PM +0800, Jason Wang wrote: >On Thu, Mar 2, 2023 at 7:35 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> The new "use_va" module parameter (default: false) is used in >> vdpa_alloc_device() to inform the vDPA framework that the device >> supports VA. >> >> vringh is initialized to use VA only when "use_va" is true and the >> user's mm has been bound. So, only when the bus supports user VA >> (e.g. vhost-vdpa). >> >> vdpasim_mm_work_fn work is used to attach the kthread to the user >> address space when the .bind_mm callback is invoked, and to detach >> it when the .unbind_mm callback is invoked. >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> >> Notes: >> v2: >> - `use_va` set to true by default [Eugenio] >> - supported the new unbind_mm callback [Jason] >> - removed the unbind_mm call in vdpasim_do_reset() [Jason] >> - avoided to release the lock while call kthread_flush_work() since we >> are now using a mutex to protect the device state >> >> drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 + >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 98 +++++++++++++++++++++++++++++++- >> 2 files changed, 97 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h >> index 4774292fba8c..3a42887d05d9 100644 >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h >> @@ -59,6 +59,7 @@ struct vdpasim { >> struct vdpasim_virtqueue *vqs; >> struct kthread_worker *worker; >> struct kthread_work work; >> + struct mm_struct *mm_bound; >> struct vdpasim_dev_attr dev_attr; >> /* mutex to synchronize virtqueue state */ >> struct mutex mutex; >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> index a28103a67ae7..eda26bc33df5 100644 >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> @@ -35,10 +35,77 @@ module_param(max_iotlb_entries, int, 0444); >> MODULE_PARM_DESC(max_iotlb_entries, >> "Maximum number of iotlb entries for each address space. 0 means unlimited. (default: 2048)"); >> >> +static bool use_va = true; >> +module_param(use_va, bool, 0444); >> +MODULE_PARM_DESC(use_va, "Enable/disable the device's ability to use VA"); >> + >> #define VDPASIM_QUEUE_ALIGN PAGE_SIZE >> #define VDPASIM_QUEUE_MAX 256 >> #define VDPASIM_VENDOR_ID 0 >> >> +struct vdpasim_mm_work { >> + struct kthread_work work; >> + struct mm_struct *mm; >> + bool bind; >> + int ret; >> +}; >> + >> +static void vdpasim_mm_work_fn(struct kthread_work *work) >> +{ >> + struct vdpasim_mm_work *mm_work = >> + container_of(work, struct vdpasim_mm_work, work); >> + >> + mm_work->ret = 0; >> + >> + if (mm_work->bind) { >> + kthread_use_mm(mm_work->mm); >> + //TODO: should we attach the cgroup of the mm owner? >> + } else { >> + kthread_unuse_mm(mm_work->mm); >> + } >> +} >> + >> +static void vdpasim_worker_queue_mm(struct vdpasim *vdpasim, >> + struct vdpasim_mm_work *mm_work) >> +{ > >Nit: we need to tweak the name as it does flush besides queuing the work.
Yep, or split in 2 functions.
> >> + struct kthread_work *work = &mm_work->work; >> + >> + kthread_init_work(work, vdpasim_mm_work_fn); >> + kthread_queue_work(vdpasim->worker, work); >> + >> + kthread_flush_work(work); >> +} >> + >> +static int vdpasim_worker_bind_mm(struct vdpasim *vdpasim, >> + struct mm_struct *new_mm) >> +{ >> + struct vdpasim_mm_work mm_work; >> + >> + mm_work.mm = new_mm; >> + mm_work.bind = true; >> + >> + vdpasim_worker_queue_mm(vdpasim, &mm_work); >> + >> + if (!mm_work.ret) >> + vdpasim->mm_bound = new_mm; >> + >> + return mm_work.ret; >> +} >> + >> +static void vdpasim_worker_unbind_mm(struct vdpasim *vdpasim) >> +{ >> + struct vdpasim_mm_work mm_work; >> + >> + if (!vdpasim->mm_bound) >> + return; >> + >> + mm_work.mm = vdpasim->mm_bound; >> + mm_work.bind = false; > >Can we simply use mm_work.mm = NULL for unbinding? > >> + >> + vdpasim_worker_queue_mm(vdpasim, &mm_work); >> + >> + vdpasim->mm_bound = NULL; > >And change the mm_bound in the worker?
Yep, I need to put `vdpasim` in struct vdpasim_mm_work.
I'll do in the next version.
Thanks, Stefano
| |