[PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[Vipin Sharma]

VM worker kthreads can linger in the VM process's cgroup for sometime
after KVM temrinates the VM process.

KVM terminates the worker kthreads by calling kthread_stop() which waits
on the signal generated by exit_mm() in do_exit() during kthread's exit.
However, these kthreads are removed from the cgroup using cgroup_exit()
call which happens after exit_mm() in do_exit(). A VM process can
terminate between the time window of exit_mm() to cgroup_exit(), leaving
only worker kthreads in the cgroup.

Moving worker kthreads back to the original cgroup (kthreadd_task's
cgroup) makes sure that cgroup is empty as soon as the main VM process
is terminated.

Signed-off-by: Vipin Sharma <vipinsh@google.com>
---
 virt/kvm/kvm_main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b0f7e6eb00ff..edd304a18f16 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -5785,7 +5785,7 @@ static int kvm_vm_worker_thread(void *context)
 	init_context = NULL;
 
 	if (err)
-		return err;
+		goto out;
 
 	/* Wait to be woken up by the spawner before proceeding. */
 	kthread_parkme();
@@ -5793,6 +5793,15 @@ static int kvm_vm_worker_thread(void *context)
 	if (!kthread_should_stop())
 		err = thread_fn(kvm, data);
 
+out:
+	/*
+	 * We need to move the kthread back to its original cgroups, so that it
+	 * doesn't linger in the cgroups of the user process after that has
+	 * already terminated. exit_mm() in do_exit() signals kthread_stop() to
+	 * return, whereas, removal of the task from the cgroups happens in
+	 * cgroup_exit() which happens after exit_mm().
+	 */
+	WARN_ON(cgroup_attach_task_all(kthreadd_task, current));
 	return err;
 }
 

base-commit: d8f6ef45a623d650f9b97e11553adb4978f6aa70
-- 
2.34.1.173.g76aa8bc2d0-goog

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[kernel test robot]

Hi Vipin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on d8f6ef45a623d650f9b97e11553adb4978f6aa70]

url:    https://github.com/0day-ci/linux/commits/Vipin-Sharma/KVM-Move-VM-s-worker-kthreads-back-to-the-original-cgroups-before-exiting/20211214-130827
base:   d8f6ef45a623d650f9b97e11553adb4978f6aa70
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20211214/202112142328.a9ebDmd7-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/fd29d23507ef3f06b61d9de1b7ecd1a0d70136f3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vipin-Sharma/KVM-Move-VM-s-worker-kthreads-back-to-the-original-cgroups-before-exiting/20211214-130827
        git checkout fd29d23507ef3f06b61d9de1b7ecd1a0d70136f3
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "kthreadd_task" [arch/riscv/kvm/kvm.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[Lai Jiangshan]

On Tue, Dec 14, 2021 at 4:13 PM Vipin Sharma <vipinsh@google.com> wrote:
>
> VM worker kthreads can linger in the VM process's cgroup for sometime
> after KVM temrinates the VM process.
>
> KVM terminates the worker kthreads by calling kthread_stop() which waits
> on the signal generated by exit_mm() in do_exit() during kthread's exit.
> However, these kthreads are removed from the cgroup using cgroup_exit()
> call which happens after exit_mm() in do_exit(). A VM process can
> terminate between the time window of exit_mm() to cgroup_exit(), leaving
> only worker kthreads in the cgroup.
>
> Moving worker kthreads back to the original cgroup (kthreadd_task's
> cgroup) makes sure that cgroup is empty as soon as the main VM process
> is terminated.
>
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> ---
>  virt/kvm/kvm_main.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Hello

Off-topic, can this kvm worker_thread and the thread to do async pagefault
be possibly changed to use something like io_uring's IOWQ (fs/io-wq.c)
created by create_io_thread()?

So that every resource the threads used are credited to the process
of the vm.

Thanks
Lai

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[Vipin Sharma]

On Tue, Dec 14, 2021 at 8:14 AM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> Hello
>
> Off-topic, can this kvm worker_thread and the thread to do async pagefault
> be possibly changed to use something like io_uring's IOWQ (fs/io-wq.c)
> created by create_io_thread()?
>
> So that every resource the threads used are credited to the process
> of the vm.
>

Sorry, I am not very familiar with it. Maybe someone from the
community knows better about this.

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[Sean Christopherson]

On Tue, Dec 14, 2021, Vipin Sharma wrote:
> VM worker kthreads can linger in the VM process's cgroup for sometime
> after KVM temrinates the VM process.
> 
> KVM terminates the worker kthreads by calling kthread_stop() which waits
> on the signal generated by exit_mm() in do_exit() during kthread's exit.
> However, these kthreads are removed from the cgroup using cgroup_exit()
> call which happens after exit_mm() in do_exit(). A VM process can
> terminate between the time window of exit_mm() to cgroup_exit(), leaving
> only worker kthreads in the cgroup.
> 
> Moving worker kthreads back to the original cgroup (kthreadd_task's
> cgroup) makes sure that cgroup is empty as soon as the main VM process
> is terminated.
> 
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> ---
>  virt/kvm/kvm_main.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b0f7e6eb00ff..edd304a18f16 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -5785,7 +5785,7 @@ static int kvm_vm_worker_thread(void *context)
>  	init_context = NULL;
>  
>  	if (err)
> -		return err;
> +		goto out;
>  
>  	/* Wait to be woken up by the spawner before proceeding. */
>  	kthread_parkme();
> @@ -5793,6 +5793,15 @@ static int kvm_vm_worker_thread(void *context)
>  	if (!kthread_should_stop())
>  		err = thread_fn(kvm, data);
>  
> +out:
> +	/*
> +	 * We need to move the kthread back to its original cgroups, so that it
> +	 * doesn't linger in the cgroups of the user process after that has
> +	 * already terminated. exit_mm() in do_exit() signals kthread_stop() to
> +	 * return, whereas, removal of the task from the cgroups happens in
> +	 * cgroup_exit() which happens after exit_mm().
> +	 */
> +	WARN_ON(cgroup_attach_task_all(kthreadd_task, current));

As the build bot noted, kthreadd_task isn't exported, and I doubt you'll convince
folks to let you export it.

Why is it problematic for the kthread to linger in the cgroup?  Conceptually, it's
not really wrong.

>  	return err;
>  }
>  
> 
> base-commit: d8f6ef45a623d650f9b97e11553adb4978f6aa70
> -- 
> 2.34.1.173.g76aa8bc2d0-goog
> 

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[Vipin Sharma]

On Tue, Dec 14, 2021 at 9:16 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Dec 14, 2021, Vipin Sharma wrote:
> > +     WARN_ON(cgroup_attach_task_all(kthreadd_task, current));
>
> As the build bot noted, kthreadd_task isn't exported, and I doubt you'll convince
> folks to let you export it.
>
> Why is it problematic for the kthread to linger in the cgroup?  Conceptually, it's
> not really wrong.

Issue comes when a process tries to clear up the resources when a VM
shutdown/dies. The process sometimes get an EBUSY error when it tries
to delete the cgroup directories which were created for that VM. It is
also difficult to know how many times to retry or how much time to
wait before the cgroup is empty. This issue is not always happening.

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[kernel test robot]

Hi Vipin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on d8f6ef45a623d650f9b97e11553adb4978f6aa70]

url:    https://github.com/0day-ci/linux/commits/Vipin-Sharma/KVM-Move-VM-s-worker-kthreads-back-to-the-original-cgroups-before-exiting/20211214-130827
base:   d8f6ef45a623d650f9b97e11553adb4978f6aa70
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20211215/202112150131.MaZ9xOJx-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/fd29d23507ef3f06b61d9de1b7ecd1a0d70136f3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vipin-Sharma/KVM-Move-VM-s-worker-kthreads-back-to-the-original-cgroups-before-exiting/20211214-130827
        git checkout fd29d23507ef3f06b61d9de1b7ecd1a0d70136f3
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "kthreadd_task" [arch/x86/kvm/kvm.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[Paolo Bonzini]

On 12/14/21 06:07, Vipin Sharma wrote:
> 
> KVM terminates the worker kthreads by calling kthread_stop() which waits
> on the signal generated by exit_mm() in do_exit() during kthread's exit.

Instead of "signal", please spell it as "the 'exited' completion, 
triggered by exit_mm(), via mm_release(), during the kthread's exit". 
That makes things a bit clearer.

So the issue is that the kthread_stop happens around the time 
exit_task_work() destroys the VM, but the process can go on and signal 
its demise to the parent process before the kthread has been completely 
dropped.  Not even close() can fix it, though it may reduce the window 
completely, so I agree that this is a bug and vhost has the same bug too.

Due to the issue with kthreadd_task not being exported, perhaps you can 
change cgroup_attach_task_all to use kthreadd_task if the "from" 
argument is NULL?

Paolo

Re: [PATCH] KVM: Move VM's worker kthreads back to the original cgroups before exiting.

[Vipin Sharma]

On Wed, Dec 15, 2021 at 10:27 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> So the issue is that the kthread_stop happens around the time
> exit_task_work() destroys the VM, but the process can go on and signal
> its demise to the parent process before the kthread has been completely
> dropped.  Not even close() can fix it, though it may reduce the window
> completely, so I agree that this is a bug and vhost has the same bug too.
>
> Due to the issue with kthreadd_task not being exported, perhaps you can
> change cgroup_attach_task_all to use kthreadd_task if the "from"
> argument is NULL?

Thanks for the suggestion. This also works, I will send out a patch.