| Message ID | 20181226133351.894160986@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PMEM NUMA node and hotness accounting/migration | expand |
On Wed, Dec 26, 2018 at 09:15:00PM +0800, Fengguang Wu wrote: > VM is associated with an address space and not a specific thread. > > >From Documentation/virtual/kvm/api.txt: > Only run VM ioctls from the same process (address space) that was used > to create the VM. Hi, Fengguang, AFAIU the commit message only explains why a kvm object needs to bind to a single mm object (say, the reason why there is kvm->mm) however not the reverse (say, the reason why there is mm->kvm), while the latter is what this patch really needs? I'm thinking whether it's legal for multiple VMs to run on a single mm address space. I don't see a limitation so far but it's very possible I am just missing something there (if there is, IMHO they might be something nice to put into the commit message?). Thanks, > > CC: Nikita Leshenko <nikita.leshchenko@oracle.com> > CC: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> > --- > include/linux/mm_types.h | 11 +++++++++++ > virt/kvm/kvm_main.c | 3 +++ > 2 files changed, 14 insertions(+) > > --- linux.orig/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800 > +++ linux/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800 > @@ -27,6 +27,7 @@ typedef int vm_fault_t; > struct address_space; > struct mem_cgroup; > struct hmm; > +struct kvm; > > /* > * Each physical page in the system has a struct page associated with > @@ -496,6 +497,10 @@ struct mm_struct { > /* HMM needs to track a few things per mm */ > struct hmm *hmm; > #endif > + > +#if IS_ENABLED(CONFIG_KVM) > + struct kvm *kvm; > +#endif > } __randomize_layout; > > /* > @@ -507,6 +512,12 @@ struct mm_struct { > > extern struct mm_struct init_mm; > > +#if IS_ENABLED(CONFIG_KVM) > +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return mm->kvm; } > +#else > +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return NULL; } > +#endif > + > /* Pointer magic because the dynamic array size confuses some compilers. */ > static inline void mm_init_cpumask(struct mm_struct *mm) > { > --- linux.orig/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800 > +++ linux/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800 > @@ -727,6 +727,7 @@ static void kvm_destroy_vm(struct kvm *k > struct mm_struct *mm = kvm->mm; > > kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm); > + mm->kvm = NULL; > kvm_destroy_vm_debugfs(kvm); > kvm_arch_sync_events(kvm); > spin_lock(&kvm_lock); > @@ -3224,6 +3225,8 @@ static int kvm_dev_ioctl_create_vm(unsig > fput(file); > return -ENOMEM; > } > + > + kvm->mm->kvm = kvm; > kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm); > > fd_install(r, file); > > Regards,
Hi Peter, On Sat, Feb 02, 2019 at 02:57:41PM +0800, Peter Xu wrote: >On Wed, Dec 26, 2018 at 09:15:00PM +0800, Fengguang Wu wrote: >> VM is associated with an address space and not a specific thread. >> >> >From Documentation/virtual/kvm/api.txt: >> Only run VM ioctls from the same process (address space) that was used >> to create the VM. > >Hi, Fengguang, > >AFAIU the commit message only explains why a kvm object needs to bind >to a single mm object (say, the reason why there is kvm->mm) however >not the reverse (say, the reason why there is mm->kvm), while the >latter is what this patch really needs? Yeah good point. The addition of mm->kvm makes code in this patchset simple. However if that field is considered not general useful for other possible users, and the added space overheads is a concern, we can instead do with a flag (saying the mm is referenced by some KVM), and add extra lookup code to find out the exact kvm instance. >I'm thinking whether it's legal for multiple VMs to run on a single mm >address space. I don't see a limitation so far but it's very possible >I am just missing something there (if there is, IMHO they might be >something nice to put into the commit message?). Thanks, So far one QEMU only starts one KVM. I cannot think of any strong benefit to start multiple KVMs in one single QEMU, so it may well remain so in future. Anyway it's internal data structure instead of API, which can adapt to possible future changes. Thanks, Fengguang >> CC: Nikita Leshenko <nikita.leshchenko@oracle.com> >> CC: Christian Borntraeger <borntraeger@de.ibm.com> >> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> >> --- >> include/linux/mm_types.h | 11 +++++++++++ >> virt/kvm/kvm_main.c | 3 +++ >> 2 files changed, 14 insertions(+) >> >> --- linux.orig/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800 >> +++ linux/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800 >> @@ -27,6 +27,7 @@ typedef int vm_fault_t; >> struct address_space; >> struct mem_cgroup; >> struct hmm; >> +struct kvm; >> >> /* >> * Each physical page in the system has a struct page associated with >> @@ -496,6 +497,10 @@ struct mm_struct { >> /* HMM needs to track a few things per mm */ >> struct hmm *hmm; >> #endif >> + >> +#if IS_ENABLED(CONFIG_KVM) >> + struct kvm *kvm; >> +#endif >> } __randomize_layout; >> >> /* >> @@ -507,6 +512,12 @@ struct mm_struct { >> >> extern struct mm_struct init_mm; >> >> +#if IS_ENABLED(CONFIG_KVM) >> +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return mm->kvm; } >> +#else >> +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return NULL; } >> +#endif >> + >> /* Pointer magic because the dynamic array size confuses some compilers. */ >> static inline void mm_init_cpumask(struct mm_struct *mm) >> { >> --- linux.orig/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800 >> +++ linux/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800 >> @@ -727,6 +727,7 @@ static void kvm_destroy_vm(struct kvm *k >> struct mm_struct *mm = kvm->mm; >> >> kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm); >> + mm->kvm = NULL; >> kvm_destroy_vm_debugfs(kvm); >> kvm_arch_sync_events(kvm); >> spin_lock(&kvm_lock); >> @@ -3224,6 +3225,8 @@ static int kvm_dev_ioctl_create_vm(unsig >> fput(file); >> return -ENOMEM; >> } >> + >> + kvm->mm->kvm = kvm; >> kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm); >> >> fd_install(r, file); >> >> > >Regards, > >-- >Peter Xu >
On 02/02/19 07:57, Peter Xu wrote: > > I'm thinking whether it's legal for multiple VMs to run on a single mm > address space. I don't see a limitation so far but it's very possible > I am just missing something there (if there is, IMHO they might be > something nice to put into the commit message?). Thanks, Yes, it certainly is legal, and even useful in fact. For example there are people running WebAssembly in a KVM sandbox. In that case you can have multiple KVM instances in a single process. It seems to me that there is already a perfect way to link an mm to its users, which is the MMU notifier. Why do you need a separate proc_ept_idle_operations? You could change ept_idle_read into an MMU notifier callback, and have core mm/ core combine the output of mm_idle_read and all the MMU notifiers? Basically, ept_idle_ctrl becomes an argument to the new MMU notifier callback, or something like that. Paolo
--- linux.orig/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800 +++ linux/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800 @@ -27,6 +27,7 @@ typedef int vm_fault_t; struct address_space; struct mem_cgroup; struct hmm; +struct kvm; /* * Each physical page in the system has a struct page associated with @@ -496,6 +497,10 @@ struct mm_struct { /* HMM needs to track a few things per mm */ struct hmm *hmm; #endif + +#if IS_ENABLED(CONFIG_KVM) + struct kvm *kvm; +#endif } __randomize_layout; /* @@ -507,6 +512,12 @@ struct mm_struct { extern struct mm_struct init_mm; +#if IS_ENABLED(CONFIG_KVM) +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return mm->kvm; } +#else +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return NULL; } +#endif + /* Pointer magic because the dynamic array size confuses some compilers. */ static inline void mm_init_cpumask(struct mm_struct *mm) { --- linux.orig/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800 +++ linux/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800 @@ -727,6 +727,7 @@ static void kvm_destroy_vm(struct kvm *k struct mm_struct *mm = kvm->mm; kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm); + mm->kvm = NULL; kvm_destroy_vm_debugfs(kvm); kvm_arch_sync_events(kvm); spin_lock(&kvm_lock); @@ -3224,6 +3225,8 @@ static int kvm_dev_ioctl_create_vm(unsig fput(file); return -ENOMEM; } + + kvm->mm->kvm = kvm; kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm); fd_install(r, file);