Re: cgroup2 freezer and kvm_vm_worker_thread()

[Tejun Heo <tj@kernel.org>]

Hello, Paolo.

On Tue, Oct 29, 2024 at 11:59:43PM +0100, Paolo Bonzini wrote:
...
> For the freezing part, would this be anything more than
> 
> fdiff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index d16ce8174ed6..b7b6a1c1b6a4 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -47,6 +47,7 @@
>  #include <linux/kern_levels.h>
>  #include <linux/kstrtox.h>
>  #include <linux/kthread.h>
> +#include <linux/freezer.h>
>  #include <linux/wordpart.h>
>  #include <asm/page.h>
> @@ -7429,22 +7430,27 @@ static long get_nx_huge_page_recovery_timeout(u64 start_time)
>  static int kvm_nx_huge_page_recovery_worker(struct kvm *kvm, uintptr_t data)
>  {
>  	u64 start_time;
> +	u64 end_time;
> +
> +	set_freezable();
>  	while (true) {
>  		start_time = get_jiffies_64();
> +		end_time = start_time + get_nx_huge_page_recovery_timeout(start_time);
> +		for (;;) {
>  			set_current_state(TASK_INTERRUPTIBLE);
> +			if (kthread_freezable_should_stop(NULL))
> +				break;
> +			start_time = get_jiffies_64();
> +			if ((s64)(end_time - start_time) <= 0)
> +				break;
> +			schedule_timeout(end_time - start_time);
>  		}
>  		set_current_state(TASK_RUNNING);
> +		if (kthread_freezable_should_stop(NULL))

So, this is the PM and cgroup1 freezer which traps tasks in a pretty opaque
state which is problematic when only a part of the system is frozen, so the
cgroup2 freezer is implemented in the signal delivery / trap path so that
they behave similar to e.g. SIGSTOP stops.

>  			return 0;
>  		kvm_recover_nx_huge_pages(kvm);
> 
> (untested beyond compilation).
> 
> I'm not sure if the KVM worker thread should process signals.  We want it
> to take the CPU time it uses from the guest, but otherwise it's not running
> on behalf of userspace in the way that io_wq_worker() is.

I see, so io_wq_worker()'s handle signals only partially. It sets
PF_USER_WORKER which ignores fatal signals, so the only signals which take
effect are STOP/CONT (and friends) which is handled in do_signal_stop()
which is also where the cgroup2 freezer is implemented.

Given that the kthreads are tied to user processes, I think it'd be better
to behave similarly to user tasks as possible in this regard if userspace
being able to stop/cont these kthreads are okay.

If that can't be done, we can add a mechanism to ignore these tasks when
determining when a cgroup is frozen provided that this doesn't affect the
snapshotting (ie. when taking a snapshot of frozen cgroup, activities from
the kthreads won't corrupt the snapshot).

Thanks.

-- 
tejun