On Mon, Apr 08, 2019 at 11:39:36AM -0400 Waiman Long wrote:
> On 04/08/2019 11:14 AM, Tejun Heo wrote:
> > Hello,
> >
> > (cc'ing Waiman and copying the whole message for him)
> >
> > On Fri, Apr 05, 2019 at 11:36:59AM -0400, Joel Savitz wrote:
> >> If a process is limited by taskset (i.e. cpuset) to only be allowed to
> >> run on cpu N, and then cpu N is offlined via hotplug, the process will
> >> be assigned the current value of its cpuset cgroup's effective_cpus field
> >> in a call to do_set_cpus_allowed() in cpuset_cpus_allowed_fallback().
> >> This argument's value does not makes sense for this case, because
> >> task_cs(tsk)->effective_cpus is modified by cpuset_hotplug_workfn()
> >> to reflect the new value of cpu_active_mask after cpu N is removed from
> >> the mask. While this may make sense for the cgroup affinity mask, it
> >> does not make sense on a per-task basis, as a task that was previously
> >> limited to only be run on cpu N will be limited to every cpu _except_ for
> >> cpu N after it is offlined/onlined via hotplug.
> >>
> >> Pre-patch behavior:
> >>
> >> $ grep Cpus /proc/$$/status
> >> Cpus_allowed: ff
> >> Cpus_allowed_list: 0-7
> >>
> >> $ taskset -p 4 $$
> >> pid 19202's current affinity mask: f
> >> pid 19202's new affinity mask: 4
> >>
> >> $ grep Cpus /proc/self/status
> >> Cpus_allowed: 04
> >> Cpus_allowed_list: 2
> >>
> >> # echo off > /sys/devices/system/cpu/cpu2/online
> >> $ grep Cpus /proc/$$/status
> >> Cpus_allowed: 0b
> >> Cpus_allowed_list: 0-1,3
> >>
> >> # echo on > /sys/devices/system/cpu/cpu2/online
> >> $ grep Cpus /proc/$$/status
> >> Cpus_allowed: 0b
> >> Cpus_allowed_list: 0-1,3
> >>
> >> On a patched system, the final grep produces the following
> >> output instead:
> >>
> >> $ grep Cpus /proc/$$/status
> >> Cpus_allowed: ff
> >> Cpus_allowed_list: 0-7
> >>
> >> This patch changes the above behavior by instead simply resetting the mask
> >> to cpu_possible_mask.
> >>
> >> Signed-off-by: Joel Savitz <jsavitz@xxxxxxxxxx>
> >> ---
> >> kernel/cgroup/cpuset.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> >> index 479743db6c37..5f65a2167bdf 100644
> >> --- a/kernel/cgroup/cpuset.c
> >> +++ b/kernel/cgroup/cpuset.c
> >> @@ -3243,7 +3243,7 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
> >> void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
> >> {
> >> rcu_read_lock();
> >> - do_set_cpus_allowed(tsk, task_cs(tsk)->effective_cpus);
> >> + do_set_cpus_allowed(tsk, cpu_possible_mask);
> >> rcu_read_unlock();
> > cpuset directly mangling with per-task masks has always been weird and
> > somewhat broken. Given the current cpuset behavior, I suppose this is
> > the better behavior. Waiman, what do you think?
> >
> > Thanks.
> >
> I think it may be better to use cpus_allowed in the case of fallback to
> make sure that the task isn't allowed to run on CPUs it is not supposed
> to run on, e.g. in a VM or container under cpuset control. For tasks in
> the top cpuset, it is the same as cpu_possible_mask. Of course, we are
> assuming that cpus_allowed has some sane value. BTW, there should be
> some comments about handling this case of cpu offlining.
>
This is setting cpus_allowed, so we can't use that here. This is the final
fallback. We've already tried parent cpuset bits at this point and found
nothing. If the parent had a mask that included a CPU that was still present
we would have already used that. I believe Joel's testing included using
a cpuset hierarchy and it did the right thing.
I don't know if he has those notes still or not.
Cheers,
Phil
> -Longman
>
>
>
--
