Hi Paolo,
On Tue, Oct 05, 2021 at 11:38:29AM +0200, Paolo Bonzini wrote:
> [+Wanpeng]
>
> On 05/10/21 00:26, Nitesh Narayan Lal wrote:
> > From: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> >
> > kvm_vm_worker_thread() creates a kthread VM worker and migrates it
> > to the parent cgroup using cgroup_attach_task_all() based on its
> > effective cpumask.
> >
> > In an environment that is booted with the nohz_full kernel option, cgroup's
> > effective cpumask can also include CPUs running in nohz_full mode. These
> > CPUs often run SCHED_FIFO tasks which may result in the starvation of the
> > VM worker if it has been migrated to one of these CPUs.
>
> There are other effects of cgroups (e.g. memory accounting) than just the
> cpumask; for v1 you could just skip the cpuset, but if
> cgroup_attach_task_all is ever ported to v2's cgroup_attach_task, we will
> not be able to separate the cpuset cgroup from the others.
cgroup_attach_task_all does use cgroup_attach_task on linux-2.6.git...
It would be good to have this working on both cgroup-v1 and cgroup-v2.
Is kvm-nx-hpage using significant amounts of memory?
> Why doesn't the scheduler move the task to a CPU that is not being hogged by
> vCPU SCHED_FIFO tasks?
Because cpuset placement is enforced:
CPUSET(7) Linux Programmer's Manual CPUSET(7)
Cpusets are integrated with the sched_setaffinity(2) scheduling affinity mechanism and the
mbind(2) and set_mempolicy(2) memory-placement mechanisms in the kernel. Neither of these
mechanisms let a process make use of a CPU or memory node that is not allowed by that
process's cpuset. If changes to a process's cpuset placement conflict with these other
mechanisms, then cpuset placement is enforced even if it means overriding these other mech‐
anisms. The kernel accomplishes this overriding by silently restricting the CPUs and mem‐
ory nodes requested by these other mechanisms to those allowed by the invoking process's
cpuset. This can result in these other calls returning an error, if for example, such a
call ends up requesting an empty set of CPUs or memory nodes, after that request is
restricted to the invoking process's cpuset.
> The parent cgroup should always have one for
> userspace's own housekeeping.
>
> As an aside, if we decide that KVM's worker threads count as housekeeping,
> you'd still want to bind the kthread to the housekeeping CPUs(*).
This is being done automatically by HK_FLAG_KTHREAD (see
kernel/thread.c).
>
> Paolo
>
> (*) switching from kthread_run to kthread_create+kthread_bind_mask
>
> > Since unbounded kernel threads allowed CPU mask already respects nohz_full
> > CPUs at the time of their setup (because of 9cc5b8656892: "isolcpus: Affine
> > unbound kernel threads to housekeeping cpus"), retain the initial CPU mask
> > for the kthread by stopping its migration to the parent cgroup's effective
> > CPUs.
> >
> > Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> > Signed-off-by: Nitesh Narayan Lal <nitesh@xxxxxxxxxx>
> > ---
> > virt/kvm/kvm_main.c | 20 +++++++++++++++-----
> > 1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 7851f3a1b5f7..87bc193fd020 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -56,6 +56,7 @@
> > #include <asm/processor.h>
> > #include <asm/ioctl.h>
> > #include <linux/uaccess.h>
> > +#include <linux/sched/isolation.h>
> > #include "coalesced_mmio.h"
> > #include "async_pf.h"
> > @@ -5634,11 +5635,20 @@ static int kvm_vm_worker_thread(void *context)
> > if (err)
> > goto init_complete;
> > - err = cgroup_attach_task_all(init_context->parent, current);
> > - if (err) {
> > - kvm_err("%s: cgroup_attach_task_all failed with err %d\n",
> > - __func__, err);
> > - goto init_complete;
> > + /*
> > + * For nohz_full enabled environments, don't migrate the worker thread
> > + * to parent cgroup as its effective mask may have a CPU running in
> > + * nohz_full mode. nohz_full CPUs often run SCHED_FIFO task which could
> > + * result in starvation of the worker thread if it is pinned on the same
> > + * CPU.
> > + */
Actually, we don't want the kthread in the isolated CPUs (irrespective
of nohz_full=, starvation, or anything). Its just about
"don't run a kernel thread on isolated CPUs".
> > + if (!housekeeping_enabled(HK_FLAG_KTHREAD)) {
> > + err = cgroup_attach_task_all(init_context->parent, current);
> > + if (err) {
> > + kvm_err("%s: cgroup_attach_task_all failed with err %d\n",
> > + __func__, err);
> > + goto init_complete;
> > + }
> > }
> > set_user_nice(current, task_nice(init_context->parent));
> >
>
>
