Fix broken cpuset affinity handling on heterogeneous systems

From: Peter Zijlstra <[email protected]>

There is a difference in behaviour between CPUSET={y,n} that is now wrecking havoc with {relax,force}_compatible_cpus_allowed_ptr().

Specifically, since commit 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask") relax_compatible_cpus_allowed_ptr() is calling __sched_setaffinity() unconditionally.

But the underlying problem goes back a lot further, possibly to
commit: ae1c802382f7 ("cpuset: apply cs->effective_{cpus,mems}") which switched cpuset_cpus_allowed() from cs->cpus_allowed to cs->effective_cpus.

The problem is that for CPUSET=y cpuset_cpus_allowed() will filter out all offline CPUs. For tasks that are part of a (!root) cpuset this is then later fixed up by the cpuset hotplug notifiers that re-evaluate and re-apply cs->effective_cpus, but for (normal) tasks in the root cpuset this does not happen and they will forever after be excluded from CPUs onlined later.

As such, rewrite cpuset_cpus_allowed() to return a wider mask, including the offline CPUs.

Fixes 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")

Reported-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link https://lkml.kernel.org/r/20230117160825.GA17756@willie-the-truck open here

Signed-off-by Will Deacon <[email protected]>

Quoted 21 lines · Will Deacon

From: Peter Zijlstra <[email protected]>

There is a difference in behaviour between CPUSET={y,n} that is now wrecking havoc with {relax,force}_compatible_cpus_allowed_ptr().

Specifically, since commit 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask") relax_compatible_cpus_allowed_ptr() is calling __sched_setaffinity() unconditionally.

But the underlying problem goes back a lot further, possibly to
commit: ae1c802382f7 ("cpuset: apply cs->effective_{cpus,mems}") which switched cpuset_cpus_allowed() from cs->cpus_allowed to cs->effective_cpus.

The problem is that for CPUSET=y cpuset_cpus_allowed() will filter out all offline CPUs. For tasks that are part of a (!root) cpuset this is then later fixed up by the cpuset hotplug notifiers that re-evaluate and re-apply cs->effective_cpus, but for (normal) tasks in the root cpuset this does not happen and they will forever after be excluded from CPUs onlined later.

Quoted 8 lines

As such, rewrite cpuset_cpus_allowed() to return a wider mask, including the offline CPUs.

Fixes 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")

Reported-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link https://lkml.kernel.org/r/20230117160825.GA17756@willie-the-truck open here

Signed-off-by Will Deacon <[email protected]>

Before cgroup v2, cpuset had only one cpumask - cpus_allowed. It only tracked online cpus and ignored the offline ones. It behaves more like effective_cpus in cpuset v2. With v2, we have 2 cpumasks - cpus_allowed and effective_cpus. When cpuset v1 is mounted, cpus_allowed and effective_cpus are effectively the same and track online cpus. With cpuset v2, cpus_allowed contains what the user has written into and it won't be changed until another write happen. However, what the user written may not be what the system can give it and effective_cpus is what the system decides a cpuset can use.

Cpuset v2 is able to handle hotplug correctly and update the task's cpumask accordingly. So missing previously offline cpus won't happen with v2.

Since v1 keeps the old behavior, previously offlined cpus are lost in the cpuset's cpus_allowed. However tasks in the root cpuset will still be fine with cpu hotplug as its cpus_allowed should track cpu_online_mask. IOW, only tasks in a non-root cpuset suffer this problem.

It was a known issue in v1 and I believe is one of the major reasons of the cpuset v2 redesign.

A major concern I have is the overhead of creating a poor man version of v2 cpus_allowed. This issue can be worked around even for cpuset v1 if it is mounted with the cpuset_v2_mode option to behave more like v2 in its cpumask handling. Alternatively we may be able to provide a config option to make this the default for v1 without the special mount option, if necessary.

Quoted 48 lines · Waiman Long

Quoted 21 lines · Will Deacon

From: Peter Zijlstra <[email protected]>

There is a difference in behaviour between CPUSET={y,n} that is now wrecking havoc with {relax,force}_compatible_cpus_allowed_ptr().

Specifically, since commit 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask") relax_compatible_cpus_allowed_ptr() is calling __sched_setaffinity() unconditionally.

But the underlying problem goes back a lot further, possibly to
commit: ae1c802382f7 ("cpuset: apply cs->effective_{cpus,mems}") which switched cpuset_cpus_allowed() from cs->cpus_allowed to cs->effective_cpus.

The problem is that for CPUSET=y cpuset_cpus_allowed() will filter out all offline CPUs. For tasks that are part of a (!root) cpuset this is then later fixed up by the cpuset hotplug notifiers that re-evaluate and re-apply cs->effective_cpus, but for (normal) tasks in the root cpuset this does not happen and they will forever after be excluded from CPUs onlined later.

Quoted 8 lines

As such, rewrite cpuset_cpus_allowed() to return a wider mask, including the offline CPUs.

Fixes 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")

Reported-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link https://lkml.kernel.org/r/20230117160825.GA17756@willie-the-truck open here

Signed-off-by Will Deacon <[email protected]>

Before cgroup v2, cpuset had only one cpumask - cpus_allowed. It only tracked online cpus and ignored the offline ones. It behaves more like effective_cpus in cpuset v2. With v2, we have 2 cpumasks - cpus_allowed and effective_cpus. When cpuset v1 is mounted, cpus_allowed and effective_cpus are effectively the same and track online cpus. With cpuset v2, cpus_allowed contains what the user has written into and it won't be changed until another write happen. However, what the user written may not be what the system can give it and effective_cpus is what the system decides a cpuset can use.

Cpuset v2 is able to handle hotplug correctly and update the task's cpumask accordingly. So missing previously offline cpus won't happen with v2.

Since v1 keeps the old behavior, previously offlined cpus are lost in the cpuset's cpus_allowed. However tasks in the root cpuset will still be fine with cpu hotplug as its cpus_allowed should track cpu_online_mask. IOW, only tasks in a non-root cpuset suffer this problem.

Quoted 8 lines

It was a known issue in v1 and I believe is one of the major reasons of the cpuset v2 redesign.

A major concern I have is the overhead of creating a poor man version of v2 cpus_allowed. This issue can be worked around even for cpuset v1 if it is mounted with the cpuset_v2_mode option to behave more like v2 in its cpumask handling. Alternatively we may be able to provide a config option to make this the default for v1 without the special mount option, if necessary.

You're still not getting it -- even cpuset (be it v1 or v2) *MUST* *NOT* mask offline cpus for root cgroup tasks, ever. (And the only reason it gets away with masking offline for !root is that it re-applies the mask every time it changes.)

Yes it did that for a fair while -- but it is wrong and broken and a very big behavioural difference between CONFIG_CPUSET={y,n}. This must not be.

Arguably cpuset-v2 is still wrong for masking offline cpus in it's effective_cpus mask, but I really didn't want to go rewrite cpuset.c for something that needs to go into /urgent *now*.

Hence this minimal patch that at least lets sched_setaffinity() work as intended.

Quoted 70 lines · Peter Zijlstra

Quoted 48 lines · Waiman Long

Quoted 21 lines · Will Deacon

From: Peter Zijlstra <[email protected]>

There is a difference in behaviour between CPUSET={y,n} that is now wrecking havoc with {relax,force}_compatible_cpus_allowed_ptr().

Specifically, since commit 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask") relax_compatible_cpus_allowed_ptr() is calling __sched_setaffinity() unconditionally.

But the underlying problem goes back a lot further, possibly to
commit: ae1c802382f7 ("cpuset: apply cs->effective_{cpus,mems}") which switched cpuset_cpus_allowed() from cs->cpus_allowed to cs->effective_cpus.

The problem is that for CPUSET=y cpuset_cpus_allowed() will filter out all offline CPUs. For tasks that are part of a (!root) cpuset this is then later fixed up by the cpuset hotplug notifiers that re-evaluate and re-apply cs->effective_cpus, but for (normal) tasks in the root cpuset this does not happen and they will forever after be excluded from CPUs onlined later.

Quoted 8 lines

As such, rewrite cpuset_cpus_allowed() to return a wider mask, including the offline CPUs.

Fixes 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")

Reported-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link https://lkml.kernel.org/r/20230117160825.GA17756@willie-the-truck open here

Signed-off-by Will Deacon <[email protected]>

Before cgroup v2, cpuset had only one cpumask - cpus_allowed. It only tracked online cpus and ignored the offline ones. It behaves more like effective_cpus in cpuset v2. With v2, we have 2 cpumasks - cpus_allowed and effective_cpus. When cpuset v1 is mounted, cpus_allowed and effective_cpus are effectively the same and track online cpus. With cpuset v2, cpus_allowed contains what the user has written into and it won't be changed until another write happen. However, what the user written may not be what the system can give it and effective_cpus is what the system decides a cpuset can use.

Cpuset v2 is able to handle hotplug correctly and update the task's cpumask accordingly. So missing previously offline cpus won't happen with v2.

Since v1 keeps the old behavior, previously offlined cpus are lost in the cpuset's cpus_allowed. However tasks in the root cpuset will still be fine with cpu hotplug as its cpus_allowed should track cpu_online_mask. IOW, only tasks in a non-root cpuset suffer this problem.

Quoted 8 lines

It was a known issue in v1 and I believe is one of the major reasons of the cpuset v2 redesign.

A major concern I have is the overhead of creating a poor man version of v2 cpus_allowed. This issue can be worked around even for cpuset v1 if it is mounted with the cpuset_v2_mode option to behave more like v2 in its cpumask handling. Alternatively we may be able to provide a config option to make this the default for v1 without the special mount option, if necessary.

You're still not getting it -- even cpuset (be it v1 or v2) *MUST* *NOT* mask offline cpus for root cgroup tasks, ever. (And the only reason it gets away with masking offline for !root is that it re-applies the mask every time it changes.)

Yes it did that for a fair while -- but it is wrong and broken and a very big behavioural difference between CONFIG_CPUSET={y,n}. This must not be.

Arguably cpuset-v2 is still wrong for masking offline cpus in it's effective_cpus mask, but I really didn't want to go rewrite cpuset.c for something that needs to go into /urgent *now*.

Quoted 2 lines

Hence this minimal patch that at least lets sched_setaffinity() work as intended.

I don't object to the general idea of keeping offline cpus in a task's cpu affinity. In the case of cpu offline event, we can skip removing that offline cpu from the task's cpu affinity. That will partially solve the problem here and is also simpler.

I believe a main reason why effective_cpus holds only online cpus is because of the need to detect when there is no online cpus available in a given cpuset. In this case, it will fall back to the nearest ancestors with online cpus.

This offline cpu problem with cpuset v1 is a known problem for a long time. It is not a recent regression.

Quoted 71 lines · Waiman Long

Quoted 70 lines · Peter Zijlstra

Quoted 48 lines · Waiman Long

Quoted 21 lines · Will Deacon

From: Peter Zijlstra <[email protected]>

There is a difference in behaviour between CPUSET={y,n} that is now wrecking havoc with {relax,force}_compatible_cpus_allowed_ptr().

Specifically, since commit 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask") relax_compatible_cpus_allowed_ptr() is calling __sched_setaffinity() unconditionally.

But the underlying problem goes back a lot further, possibly to
commit: ae1c802382f7 ("cpuset: apply cs->effective_{cpus,mems}") which switched cpuset_cpus_allowed() from cs->cpus_allowed to cs->effective_cpus.

The problem is that for CPUSET=y cpuset_cpus_allowed() will filter out all offline CPUs. For tasks that are part of a (!root) cpuset this is then later fixed up by the cpuset hotplug notifiers that re-evaluate and re-apply cs->effective_cpus, but for (normal) tasks in the root cpuset this does not happen and they will forever after be excluded from CPUs onlined later.

Quoted 8 lines

As such, rewrite cpuset_cpus_allowed() to return a wider mask, including the offline CPUs.

Fixes 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")

Reported-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link https://lkml.kernel.org/r/20230117160825.GA17756@willie-the-truck open here

Signed-off-by Will Deacon <[email protected]>

Before cgroup v2, cpuset had only one cpumask - cpus_allowed. It only tracked online cpus and ignored the offline ones. It behaves more like effective_cpus in cpuset v2. With v2, we have 2 cpumasks - cpus_allowed and effective_cpus. When cpuset v1 is mounted, cpus_allowed and effective_cpus are effectively the same and track online cpus. With cpuset v2, cpus_allowed contains what the user has written into and it won't be changed until another write happen. However, what the user written may not be what the system can give it and effective_cpus is what the system decides a cpuset can use.

Cpuset v2 is able to handle hotplug correctly and update the task's cpumask accordingly. So missing previously offline cpus won't happen with v2.

Since v1 keeps the old behavior, previously offlined cpus are lost in the cpuset's cpus_allowed. However tasks in the root cpuset will still be fine with cpu hotplug as its cpus_allowed should track cpu_online_mask. IOW, only tasks in a non-root cpuset suffer this problem.

Quoted 8 lines

It was a known issue in v1 and I believe is one of the major reasons of the cpuset v2 redesign.

A major concern I have is the overhead of creating a poor man version of v2 cpus_allowed. This issue can be worked around even for cpuset v1 if it is mounted with the cpuset_v2_mode option to behave more like v2 in its cpumask handling. Alternatively we may be able to provide a config option to make this the default for v1 without the special mount option, if necessary.

You're still not getting it -- even cpuset (be it v1 or v2) *MUST* *NOT* mask offline cpus for root cgroup tasks, ever. (And the only reason it gets away with masking offline for !root is that it re-applies the mask every time it changes.)

Yes it did that for a fair while -- but it is wrong and broken and a very big behavioural difference between CONFIG_CPUSET={y,n}. This must not be.

Arguably cpuset-v2 is still wrong for masking offline cpus in it's effective_cpus mask, but I really didn't want to go rewrite cpuset.c for something that needs to go into /urgent *now*.

Quoted 8 lines

Quoted 2 lines

Hence this minimal patch that at least lets sched_setaffinity() work as intended.

I don't object to the general idea of keeping offline cpus in a task's cpu affinity. In the case of cpu offline event, we can skip removing that offline cpu from the task's cpu affinity. That will partially solve the problem here and is also simpler.

I believe a main reason why effective_cpus holds only online cpus is because of the need to detect when there is no online cpus available in a given cpuset. In this case, it will fall back to the nearest ancestors with online cpus.

This offline cpu problem with cpuset v1 is a known problem for a long time. It is not a recent regression.

Note that using cpus_allowed directly in cgroup v2 may not be right because cpus_allowed may have no relationship to effective_cpus at all in some cases, e.g.

   root
    |
    V
    A (cpus_allowed = 1-4, effective_cpus = 1-4)
    |
    V
    B (cpus_allowed = 5-8, effective_cpus = 1-4)

In the case of cpuset B, passing back cpus 5-8 as the allowed_cpus is wrong.

I wonder how often is cpu hotplug happening in those arm64 cpu systems that only have a subset of cpus that can run 32-bit programs.

One possible solution is to use cpuset_cpus_allowed_fallback() in case none of the cpus in the current cpuset is allowed to be used to run a given task.

Note that using cpus_allowed directly in cgroup v2 may not be right because cpus_allowed may have no relationship to effective_cpus at all in some cases, e.g.

   root
    |
    V
    A (cpus_allowed = 1-4, effective_cpus = 1-4)
    |
    V
    B (cpus_allowed = 5-8, effective_cpus = 1-4)

In the case of cpuset B, passing back cpus 5-8 as the allowed_cpus is wrong.

Quoted 12 lines · Waiman Long

Note that using cpus_allowed directly in cgroup v2 may not be right because cpus_allowed may have no relationship to effective_cpus at all in some cases, e.g.

   root
    |
    V
    A (cpus_allowed = 1-4, effective_cpus = 1-4)
    |
    V
    B (cpus_allowed = 5-8, effective_cpus = 1-4)

Quoted 1 line

In the case of cpuset B, passing back cpus 5-8 as the allowed_cpus is wrong.

I think my patch as written does the right thing here. Since the intersection of (1-4) and (5-8) is empty it will move up the hierarchy and we'll end up with (1-4) from the cgroup side of things.

So the purpose of __cs_cpus_allowed() is to override the cpus_allowed of the root set and force it to cpu_possible_mask.

Then cs_cpus_allowed() computes the intersection of cs->cpus_allowed and all it's parents. This will, in the case of B above, result in the empty mask.

Then cpuset_cpus_allowed() has a loop that starts with task_cpu_possible_mask(), intersects that with cs_cpus_allowed() and if the intersection of that and cpu_online_mask is empty, moves up the hierarchy. Given cs_cpus_allowed(B) is the empty mask, we'll move to A.

Note that since we force the mask of root to cpu_possible_mask, cs_cpus_allowed(root) will be a no-op and if we guarantee (in arch code) that cpu_online_mask always has a non-empty intersection with task_cpu_possible_mask(), this loop is guaranteed to terminate with a viable mask.

Quoted 31 lines · Peter Zijlstra

Quoted 12 lines · Waiman Long

Note that using cpus_allowed directly in cgroup v2 may not be right because cpus_allowed may have no relationship to effective_cpus at all in some cases, e.g.

   root
    |
    V
    A (cpus_allowed = 1-4, effective_cpus = 1-4)
    |
    V
    B (cpus_allowed = 5-8, effective_cpus = 1-4)

Quoted 1 line

In the case of cpuset B, passing back cpus 5-8 as the allowed_cpus is wrong.

I think my patch as written does the right thing here. Since the intersection of (1-4) and (5-8) is empty it will move up the hierarchy and we'll end up with (1-4) from the cgroup side of things.

So the purpose of __cs_cpus_allowed() is to override the cpus_allowed of the root set and force it to cpu_possible_mask.

Then cs_cpus_allowed() computes the intersection of cs->cpus_allowed and all it's parents. This will, in the case of B above, result in the empty mask.

Then cpuset_cpus_allowed() has a loop that starts with task_cpu_possible_mask(), intersects that with cs_cpus_allowed() and if the intersection of that and cpu_online_mask is empty, moves up the hierarchy. Given cs_cpus_allowed(B) is the empty mask, we'll move to A.

Quoted 5 lines

Note that since we force the mask of root to cpu_possible_mask, cs_cpus_allowed(root) will be a no-op and if we guarantee (in arch code) that cpu_online_mask always has a non-empty intersection with task_cpu_possible_mask(), this loop is guaranteed to terminate with a viable mask.

I will take a closer look at that tomorrow. I will be more comfortable ack'ing that if this is specific to v1 cpuset instead of applying this in both v1 and v2 since it is only v1 that is problematic.

Quoted 37 lines · Waiman Long

Quoted 36 lines · Peter Zijlstra

Quoted 12 lines · Waiman Long

Note that using cpus_allowed directly in cgroup v2 may not be right because cpus_allowed may have no relationship to effective_cpus at all in some cases, e.g.

   root
    |
    V
    A (cpus_allowed = 1-4, effective_cpus = 1-4) |
    V
    B (cpus_allowed = 5-8, effective_cpus = 1-4)

Quoted 1 line

In the case of cpuset B, passing back cpus 5-8 as the allowed_cpus is wrong.

I think my patch as written does the right thing here. Since the intersection of (1-4) and (5-8) is empty it will move up the hierarchy and we'll end up with (1-4) from the cgroup side of things.

So the purpose of __cs_cpus_allowed() is to override the cpus_allowed of the root set and force it to cpu_possible_mask.

Then cs_cpus_allowed() computes the intersection of cs->cpus_allowed and all it's parents. This will, in the case of B above, result in the empty mask.

Then cpuset_cpus_allowed() has a loop that starts with task_cpu_possible_mask(), intersects that with cs_cpus_allowed() and if the intersection of that and cpu_online_mask is empty, moves up the hierarchy. Given cs_cpus_allowed(B) is the empty mask, we'll move to A.

Note that since we force the mask of root to cpu_possible_mask, cs_cpus_allowed(root) will be a no-op and if we guarantee (in arch code) that cpu_online_mask always has a non-empty intersection with task_cpu_possible_mask(), this loop is guaranteed to terminate with a viable mask.

Quoted 3 lines

I will take a closer look at that tomorrow. I will be more comfortable ack'ing that if this is specific to v1 cpuset instead of applying this in both v1 and v2 since it is only v1 that is problematic.

fwiw, the regression I'm seeing is with cgroup2. I haven't tried v1.

I think I know where the problem is. It is due to the fact the cpuset hotplug code doesn't update cpumasks of the tasks in the top cpuset (root) at all when there is a cpu offline or online event. It is probably because for some of the tasks in the top cpuset, especially the percpu kthread, changing their cpumasks can be catastrophic. The hotplug code does update the cpumasks of the tasks that are not in the top cpuset. This problem is irrespective of whether v1 or v2 is in use.

Quoted 7 lines · Waiman Long

I think I know where the problem is. It is due to the fact the cpuset hotplug code doesn't update cpumasks of the tasks in the top cpuset (root) at all when there is a cpu offline or online event. It is probably because for some of the tasks in the top cpuset, especially the percpu kthread, changing their cpumasks can be catastrophic. The hotplug code does update the cpumasks of the tasks that are not in the top cpuset. This problem is irrespective of whether v1 or v2 is in use.

I've been saying this exact thing for how many mails now?

A major concern I have is the overhead of creating a poor man version of v2 cpus_allowed. This issue can be worked around even for cpuset v1 if it is mounted with the cpuset_v2_mode option to behave more like v2 in its cpumask handling. Alternatively we may be able to provide a config option to make this the default for v1 without the special mount option, if necessary.

Quoted 5 lines · Waiman Long

A major concern I have is the overhead of creating a poor man version of v2 cpus_allowed. This issue can be worked around even for cpuset v1 if it is mounted with the cpuset_v2_mode option to behave more like v2 in its cpumask handling. Alternatively we may be able to provide a config option to make this the default for v1 without the special mount option, if necessary.

It is equally broken for v2, it masks against effective_cpus. Not to mention it explicitly starts with cpu_online_mask.

After taking a close look at the patch, my understanding of what it is doing is as follows:

v2: cpus_allowed will not be affected by hotplug. So the new cpuset_cpus_allowed() will return effective_cpus + offline cpus that should have been part of effective_cpus if online before masking it with allowable cpus and then go up the cpuset hierarchy if necessary.

v1: cpus_allowed is equivalent to v2 effective_cpus. It starts at the current cpuset and move up the hierarchy if necessary to find a cpuset that have at least one allowable cpu.

First of all, it does not take into account of the v2 partition feature that may cause it to produce incorrect result if partition is enabled somewhere.

Secondly, I don't see any benefit other than having some additional offline cpu available in a task's cpumask which the scheduler will ignore anyway.

v2 is able to recover a previously offlined cpu. So we don't gain any net benefit other than the going up the cpuset hierarchy part.

For v1, I agree we should go up the cpuset hierarchy to find a usable cpuset. Instead of introducing such a complexity in cpuset_cpus_allowed(), my current preference is to do the hierarchy climbing part in an enhanced cpuset_cpus_allowed_fallback() after an initial failure of cpuset_cpus_allowed(). That will be easier to understand than having such complexity and overhead in cpuset_cpus_allowed() alone.

I will work on a patchset to do that as a counter offer.

Quoted 12 lines · Waiman Long

After taking a close look at the patch, my understanding of what it is doing is as follows:

v2: cpus_allowed will not be affected by hotplug. So the new cpuset_cpus_allowed() will return effective_cpus + offline cpus that should have been part of effective_cpus if online before masking it with allowable cpus and then go up the cpuset hierarchy if necessary.

v1: cpus_allowed is equivalent to v2 effective_cpus. It starts at the current cpuset and move up the hierarchy if necessary to find a cpuset that have at least one allowable cpu.

Quoted 2 lines

First of all, it does not take into account of the v2 partition feature that may cause it to produce incorrect result if partition is enabled somewhere.

How so? For a partition the cpus_allowed mask should be the parition CPUs. The only magical bit about partitions is that any one CPU cannot belong to two partitions and load-balancing is split.

Quoted 3 lines

Secondly, I don't see any benefit other than having some additional offline cpu available in a task's cpumask which the scheduler will ignore anyway.

Those CPUs can come online again -- you're *again* dismissing the true bug :/

If you filter out the offline CPUs at sched_setaffinity() time, you forever lose those CPUs, the task will never again move to those CPUs, even if they do come online after.

It is really simple to reproduce this:

- boot machine
- offline all CPUs except one
- taskset -p ffffffff $$
- online all CPUs

and observe your shell (and all its decendants) being stuck to the one CPU. Do the same thing on a CPUSET=n build and note the difference (you retain the full mask).

Quoted 3 lines

v2 is able to recover a previously offlined cpu. So we don't gain any net benefit other than the going up the cpuset hierarchy part.

Only for !root tasks. Not even v2 will re-set the affinity of root tasks afaict.

Quoted 7 lines

For v1, I agree we should go up the cpuset hierarchy to find a usable cpuset. Instead of introducing such a complexity in cpuset_cpus_allowed(), my current preference is to do the hierarchy climbing part in an enhanced cpuset_cpus_allowed_fallback() after an initial failure of cpuset_cpus_allowed(). That will be easier to understand than having such complexity and overhead in cpuset_cpus_allowed() alone.

Quoted 1 line

I will work on a patchset to do that as a counter offer.

We will need a small and simple patch for /urgent, or I will need to revert all your patches -- your call.

I also don't tihnk you fully appreciate the ramifications of task_cpu_possible_mask(), cpuset currently gets that quite wrong.

Quoted 4 lines

Quoted 1 line

I will work on a patchset to do that as a counter offer.

We will need a small and simple patch for /urgent, or I will need to revert all your patches -- your call.

Quoted 2 lines

I also don't tihnk you fully appreciate the ramifications of task_cpu_possible_mask(), cpuset currently gets that quite wrong.

OK, I don't realize the urgency of that. If it is that urgent, I will have no objection to get it in for now. We can improve it later on. So are you planning to get it into the current 6.2 rc or 6.3?

Tejun, are you OK with that as you are the cgroup maintainer?

Quoted 6 lines · Waiman Long

Quoted 6 lines

Quoted 1 line

I will work on a patchset to do that as a counter offer.

We will need a small and simple patch for /urgent, or I will need to revert all your patches -- your call.

I also don't tihnk you fully appreciate the ramifications of task_cpu_possible_mask(), cpuset currently gets that quite wrong.

Quoted 5 lines

OK, I don't realize the urgency of that. If it is that urgent, I will have no objection to get it in for now. We can improve it later on. So are you planning to get it into the current 6.2 rc or 6.3?

Tejun, are you OK with that as you are the cgroup maintainer?

Yeah, gotta fix the regression but is there currently a solution which fixes the regression but doesn't further break other stuff?

Quoted 11 lines · Tejun Heo

Quoted 10 lines · Waiman Long

Quoted 6 lines

Quoted 1 line

I will work on a patchset to do that as a counter offer.

We will need a small and simple patch for /urgent, or I will need to revert all your patches -- your call.

I also don't tihnk you fully appreciate the ramifications of task_cpu_possible_mask(), cpuset currently gets that quite wrong.

OK, I don't realize the urgency of that. If it is that urgent, I will have no objection to get it in for now. We can improve it later on. So are you planning to get it into the current 6.2 rc or 6.3?

Quoted 3 lines

Quoted 1 line

Tejun, are you OK with that as you are the cgroup maintainer?

Yeah, gotta fix the regression but is there currently a solution which fixes the regression but doesn't further break other stuff?

I believe there is a better way to do that, but it will need more time to flex out. Since cpuset_cpus_allowed() is only used by kernel/sched/core.c, Peter will be responsible if it somehow breaks other stuff.

Quoted 20 lines · Waiman Long

Quoted 17 lines · Tejun Heo

Quoted 6 lines · Waiman Long

Quoted 6 lines

Quoted 1 line

I will work on a patchset to do that as a counter offer.

We will need a small and simple patch for /urgent, or I will need to revert all your patches -- your call.

I also don't tihnk you fully appreciate the ramifications of task_cpu_possible_mask(), cpuset currently gets that quite wrong.

Quoted 7 lines

OK, I don't realize the urgency of that. If it is that urgent, I will have
no objection to get it in for now. We can improve it later on. So are you
planning to get it into the current 6.2 rc or 6.3?

Tejun, are you OK with that as you are the cgroup maintainer?

Yeah, gotta fix the regression but is there currently a solution which fixes
the regression but doesn't further break other stuff?

Quoted 4 lines

I believe there is a better way to do that, but it will need more time to flex out. Since cpuset_cpus_allowed() is only used by kernel/sched/core.c, Peter will be responsible if it somehow breaks other stuff.

Maybe my cpuset patch that don't update task's cpumask on cpu offline event can help. However, I don't know the exact scenario where the regression happen, so it may not.

Quoted 23 lines · Waiman Long

Quoted 22 lines · Waiman Long

Quoted 13 lines · Tejun Heo

Quoted 12 lines · Waiman Long

Quoted 6 lines

Quoted 1 line

I will work on a patchset to do that as a counter offer.

We will need a small and simple patch for /urgent, or I will need to revert all your patches -- your call.

I also don't tihnk you fully appreciate the ramifications of task_cpu_possible_mask(), cpuset currently gets that quite wrong.

OK, I don't realize the urgency of that. If it is that urgent, I will have
no objection to get it in for now. We can improve it later on. So are you
planning to get it into the current 6.2 rc or 6.3?

Quoted 4 lines

Quoted 1 line

Tejun, are you OK with that as you are the cgroup maintainer?

Yeah, gotta fix the regression but is there currently a solution which fixes
the regression but doesn't further break other stuff?

I believe there is a better way to do that, but it will need more time to flex out. Since cpuset_cpus_allowed() is only used by kernel/sched/core.c, Peter will be responsible if it somehow breaks other stuff.

Quoted 3 lines

Maybe my cpuset patch that don't update task's cpumask on cpu offline event can help. However, I don't know the exact scenario where the regression happen, so it may not.

Neither patch looks like they would break anything. That said, the patches aren't trivial and we're really close to the merge window, so I'd really appreciate if you can take a look and test a bit before we send these Linus's way. We can replace it with a better solution afterwards.

Quoted 27 lines · Waiman Long

Quoted 24 lines · Waiman Long

Quoted 13 lines · Tejun Heo

Quoted 12 lines · Waiman Long

Quoted 6 lines

Quoted 1 line

I will work on a patchset to do that as a counter offer.

We will need a small and simple patch for /urgent, or I will need to revert all your patches -- your call.

I also don't tihnk you fully appreciate the ramifications of task_cpu_possible_mask(), cpuset currently gets that quite wrong.

OK, I don't realize the urgency of that. If it is that urgent, I will have
no objection to get it in for now. We can improve it later on. So are you
planning to get it into the current 6.2 rc or 6.3?

Quoted 4 lines

Quoted 1 line

Tejun, are you OK with that as you are the cgroup maintainer?

Yeah, gotta fix the regression but is there currently a solution which fixes
the regression but doesn't further break other stuff?

I believe there is a better way to do that, but it will need more time to flex out. Since cpuset_cpus_allowed() is only used by kernel/sched/core.c, Peter will be responsible if it somehow breaks other stuff.

Quoted 3 lines

Maybe my cpuset patch that don't update task's cpumask on cpu offline event can help. However, I don't know the exact scenario where the regression happen, so it may not.

Neither patch looks like they would break anything. That said, the patches aren't trivial and we're really close to the merge window, so I'd really appreciate if you can take a look and test a bit before we send these Linus's way. We can replace it with a better solution afterwards.

kernel/cgroup/cpuset.c +5 -0

+static const struct cpumask *__cs_cpus_allowed(struct cpuset *cs)
+{
+	const struct cpumask *cs_mask = cs->cpus_allowed;
+	if (!parent_cs(cs))
+		cs_mask = cpu_possible_mask;

void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask) {

kernel/cgroup/cpuset.c +15 -1

 	unsigned long flags;
+	struct cpuset *cs;
 
 	spin_lock_irqsave(&callback_lock, flags);
-	guarantee_online_cpus(tsk, pmask);
+	rcu_read_lock();
+
+	cs = task_cs(tsk);
+	do {
+		cpumask_copy(pmask, task_cpu_possible_mask(tsk));
+		cs_cpus_allowed(cs, pmask);
+
+		if (cpumask_intersects(pmask, cpu_online_mask))
+			break;
+
+		cs = parent_cs(cs);
+	} while (cs);
+
+	rcu_read_unlock();
 	spin_unlock_irqrestore(&callback_lock, flags);
 }

Signature · 2 lines

-- 
2.39.1.456.gfc5497dd1b-goog

set_cpus_allowed_ptr() will fail with -EINVAL if the requested affinity mask is not a subset of the task_cpu_possible_mask() for the task being updated. Consequently, on a heterogeneous system with cpusets spanning the different CPU types, updates to the cgroup hierarchy can silently fail to update task affinities when the effective affinity mask for the cpuset is expanded.

For example, consider an arm64 system with 4 CPUs, where CPUs 2-3 are the only cores capable of executing 32-bit tasks. Attaching a 32-bit task to a cpuset containing CPUs 0-2 will correctly affine the task to CPU 2. Extending the cpuset to CPUs 0-3, however, will fail to extend the affinity mask of the 32-bit task because update_tasks_cpumask() will pass the full 0-3 mask to set_cpus_allowed_ptr().

Extend update_tasks_cpumask() to take a temporary 'cpumask' paramater and use it to mask the 'effective_cpus' mask with the possible mask for each task being updated.

Fixes 431c69fac05b ("cpuset: Honour task_cpu_possible_mask() in guarantee_online_cpus()")

Signed-off-by Will Deacon <[email protected]>

---

Note: We wondered whether it was worth calling guarantee_online_cpus() if the cpumask_and() returns 0 in update_tasks_cpumask(), but given that this path is only called when the effective mask changes, it didn't seem appropriate. Ultimately, if you have 32-bit tasks attached to a cpuset containing only 64-bit cpus, then the affinity is going to be forced.

Quoted 20 lines · Will Deacon

set_cpus_allowed_ptr() will fail with -EINVAL if the requested affinity mask is not a subset of the task_cpu_possible_mask() for the task being updated. Consequently, on a heterogeneous system with cpusets spanning the different CPU types, updates to the cgroup hierarchy can silently fail to update task affinities when the effective affinity mask for the cpuset is expanded.

For example, consider an arm64 system with 4 CPUs, where CPUs 2-3 are the only cores capable of executing 32-bit tasks. Attaching a 32-bit task to a cpuset containing CPUs 0-2 will correctly affine the task to CPU 2. Extending the cpuset to CPUs 0-3, however, will fail to extend the affinity mask of the 32-bit task because update_tasks_cpumask() will pass the full 0-3 mask to set_cpus_allowed_ptr().

Extend update_tasks_cpumask() to take a temporary 'cpumask' paramater and use it to mask the 'effective_cpus' mask with the possible mask for each task being updated.

Fixes 431c69fac05b ("cpuset: Honour task_cpu_possible_mask() in guarantee_online_cpus()")

Signed-off-by Will Deacon <[email protected]>

Quoted 8 lines

---

Note: We wondered whether it was worth calling guarantee_online_cpus() if the cpumask_and() returns 0 in update_tasks_cpumask(), but given that this path is only called when the effective mask changes, it didn't seem appropriate. Ultimately, if you have 32-bit tasks attached to a cpuset containing only 64-bit cpus, then the affinity is going to be forced.

Now I see how the sched_setaffinity() change is impacting arm64. Instead of putting in the bandage in cpuset. I would suggest doing another cpu masking in __set_cpus_allowed_ptr() similar to what is now done for user_cpus_ptr.

Quoted 29 lines · Waiman Long

Quoted 28 lines · Will Deacon

set_cpus_allowed_ptr() will fail with -EINVAL if the requested affinity mask is not a subset of the task_cpu_possible_mask() for the task being updated. Consequently, on a heterogeneous system with cpusets spanning the different CPU types, updates to the cgroup hierarchy can silently fail to update task affinities when the effective affinity mask for the cpuset is expanded.

For example, consider an arm64 system with 4 CPUs, where CPUs 2-3 are the only cores capable of executing 32-bit tasks. Attaching a 32-bit task to a cpuset containing CPUs 0-2 will correctly affine the task to CPU 2. Extending the cpuset to CPUs 0-3, however, will fail to extend the affinity mask of the 32-bit task because update_tasks_cpumask() will pass the full 0-3 mask to set_cpus_allowed_ptr().

Extend update_tasks_cpumask() to take a temporary 'cpumask' paramater and use it to mask the 'effective_cpus' mask with the possible mask for each task being updated.

Fixes 431c69fac05b ("cpuset: Honour task_cpu_possible_mask() in guarantee_online_cpus()")

Signed-off-by Will Deacon <[email protected]>

---

Note: We wondered whether it was worth calling guarantee_online_cpus() if the cpumask_and() returns 0 in update_tasks_cpumask(), but given that this path is only called when the effective mask changes, it didn't seem appropriate. Ultimately, if you have 32-bit tasks attached to a cpuset containing only 64-bit cpus, then the affinity is going to be forced.

Quoted 3 lines

Now I see how the sched_setaffinity() change is impacting arm64. Instead of putting in the bandage in cpuset. I would suggest doing another cpu masking in __set_cpus_allowed_ptr() similar to what is now done for user_cpus_ptr.

NO! cpuset is *BROKEN* it has been for a while, it needs to get fixed.

Masking the offline CPUs is *WRONG*.

set_cpus_allowed_ptr() will fail with -EINVAL if the requested affinity mask is not a subset of the task_cpu_possible_mask() for the task being updated. Consequently, on a heterogeneous system with cpusets spanning the different CPU types, updates to the cgroup hierarchy can silently fail to update task affinities when the effective affinity mask for the cpuset is expanded.

For example, consider an arm64 system with 4 CPUs, where CPUs 2-3 are the only cores capable of executing 32-bit tasks. Attaching a 32-bit task to a cpuset containing CPUs 0-2 will correctly affine the task to CPU 2. Extending the cpuset to CPUs 0-3, however, will fail to extend the affinity mask of the 32-bit task because update_tasks_cpumask() will pass the full 0-3 mask to set_cpus_allowed_ptr().

Extend update_tasks_cpumask() to take a temporary 'cpumask' paramater and use it to mask the 'effective_cpus' mask with the possible mask for each task being updated.

Fixes 431c69fac05b ("cpuset: Honour task_cpu_possible_mask() in guarantee_online_cpus()")

Signed-off-by Will Deacon <[email protected]>

---

Note: We wondered whether it was worth calling guarantee_online_cpus() if the cpumask_and() returns 0 in update_tasks_cpumask(), but given that this path is only called when the effective mask changes, it didn't seem appropriate. Ultimately, if you have 32-bit tasks attached to a cpuset containing only 64-bit cpus, then the affinity is going to be forced.

set_cpus_allowed_ptr() will fail with -EINVAL if the requested affinity mask is not a subset of the task_cpu_possible_mask() for the task being updated. Consequently, on a heterogeneous system with cpusets spanning the different CPU types, updates to the cgroup hierarchy can silently fail to update task affinities when the effective affinity mask for the cpuset is expanded.

For example, consider an arm64 system with 4 CPUs, where CPUs 2-3 are the only cores capable of executing 32-bit tasks. Attaching a 32-bit task to a cpuset containing CPUs 0-2 will correctly affine the task to CPU 2. Extending the cpuset to CPUs 0-3, however, will fail to extend the affinity mask of the 32-bit task because update_tasks_cpumask() will pass the full 0-3 mask to set_cpus_allowed_ptr().

Extend update_tasks_cpumask() to take a temporary 'cpumask' paramater and use it to mask the 'effective_cpus' mask with the possible mask for each task being updated.

Fixes 431c69fac05b ("cpuset: Honour task_cpu_possible_mask() in guarantee_online_cpus()")

Signed-off-by Will Deacon <[email protected]>

---

Note: We wondered whether it was worth calling guarantee_online_cpus() if the cpumask_and() returns 0 in update_tasks_cpumask(), but given that this path is only called when the effective mask changes, it didn't seem appropriate. Ultimately, if you have 32-bit tasks attached to a cpuset containing only 64-bit cpus, then the affinity is going to be forced.

  kernel/cgroup/cpuset.c | 18 +++++++++++-------
  1 file changed, 11 insertions(+), 7 deletions(-)

kernel/cgroup/cpuset.c +10 -6

index 8552cc2c586a..f15fb0426707 100644
@@ -1205,12 +1205,13 @@ void rebuild_sched_domains(void)
  /**
   * update_tasks_cpumask - Update the cpumasks of tasks in the cpuset.
   * @cs: the cpuset in which each task's cpus_allowed mask needs to be changed
+ * @new_cpus: the temp variable for the new effective_cpus mask
   *
   * Iterate through each task of @cs updating its cpus_allowed to the
   * effective cpuset's.  As this function is called with cpuset_rwsem held,
   * cpuset membership stays stable.
   */
-static void update_tasks_cpumask(struct cpuset *cs)
+static void update_tasks_cpumask(struct cpuset *cs, struct cpumask *new_cpus)
  {
  	struct css_task_iter it;
  	struct task_struct *task;

@@ -1224,7 +1225,10 @@ static void update_tasks_cpumask(struct cpuset *cs)
  		if (top_cs && (task->flags & PF_KTHREAD) &&
  		    kthread_is_per_cpu(task))
  			continue;
-		set_cpus_allowed_ptr(task, cs->effective_cpus);
+
+		cpumask_and(new_cpus, cs->effective_cpus,
+			    task_cpu_possible_mask(task));
+		set_cpus_allowed_ptr(task, new_cpus);
  	}
  	css_task_iter_end(&it);
  }

@@ -1509,7 +1513,7 @@ static int update_parent_subparts_cpumask(struct cpuset *cs, int cmd,
  	spin_unlock_irq(&callback_lock);
 
  	if (adding || deleting)
-		update_tasks_cpumask(parent);
+		update_tasks_cpumask(parent, tmp->new_cpus);
 
  	/*
  	 * Set or clear CS_SCHED_LOAD_BALANCE when partcmd_update, if necessary.

@@ -1661,7 +1665,7 @@ static void update_cpumasks_hier(struct cpuset *cs, struct tmpmasks *tmp,
  		WARN_ON(!is_in_v2_mode() &&
  			!cpumask_equal(cp->cpus_allowed, cp->effective_cpus));
 
-		update_tasks_cpumask(cp);
+		update_tasks_cpumask(cp, tmp->new_cpus);
 
  		/*
  		 * On legacy hierarchy, if the effective cpumask of any non-

@@ -2309,7 +2313,7 @@ static int update_prstate(struct cpuset *cs, int new_prs)
  		}
  	}
 
-	update_tasks_cpumask(parent);
+	update_tasks_cpumask(parent, tmpmask.new_cpus);
 
  	if (parent->child_ecpus_count)
  		update_sibling_cpumasks(parent, cs, &tmpmask);

@@ -3347,7 +3351,7 @@ hotplug_update_tasks_legacy(struct cpuset *cs,
  	 * as the tasks will be migrated to an ancestor.
  	 */
  	if (cpus_updated && !cpumask_empty(cs->cpus_allowed))
-		update_tasks_cpumask(cs);
+		update_tasks_cpumask(cs, new_cpus);
  	if (mems_updated && !nodes_empty(cs->mems_allowed))
  		update_tasks_nodemask(cs);
 

set_cpus_allowed_ptr() will fail with -EINVAL if the requested affinity mask is not a subset of the task_cpu_possible_mask() for the task being updated. Consequently, on a heterogeneous system with cpusets spanning the different CPU types, updates to the cgroup hierarchy can silently fail to update task affinities when the effective affinity mask for the cpuset is expanded.

For example, consider an arm64 system with 4 CPUs, where CPUs 2-3 are the only cores capable of executing 32-bit tasks. Attaching a 32-bit task to a cpuset containing CPUs 0-2 will correctly affine the task to CPU 2. Extending the cpuset to CPUs 0-3, however, will fail to extend the affinity mask of the 32-bit task because update_tasks_cpumask() will pass the full 0-3 mask to set_cpus_allowed_ptr().

Extend update_tasks_cpumask() to take a temporary 'cpumask' paramater and use it to mask the 'effective_cpus' mask with the possible mask for each task being updated.

Fixes 431c69fac05b ("cpuset: Honour task_cpu_possible_mask() in guarantee_online_cpus()")

Signed-off-by Will Deacon <[email protected]>