An alternative series for asymmetric AArch32 systems

If the scheduler cannot find an allowed CPU for a task, cpuset_cpus_allowed_fallback() will widen the affinity to cpu_possible_mask if cgroup v1 is in use.

In preparation for allowing architectures to provide their own fallback mask, just return early if we're not using cgroup v2 and allow select_fallback_rq() to figure out the mask by itself.

Cc Li Zefan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Reviewed-by Quentin Perret <[email protected]>

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

---
 kernel/cgroup/cpuset.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/cgroup/cpuset.c +0 -0

index 57b5b5d0a5fd..e970737c3ed2 100644

kernel/cgroup/cpuset.c +0 -0

 	rcu_read_unlock();
 
 	/*

Signature · 1 line

2.29.2.576.ga3fc446d84-goog

Quoted 23 lines · Will Deacon

If the scheduler cannot find an allowed CPU for a task, cpuset_cpus_allowed_fallback() will widen the affinity to cpu_possible_mask if cgroup v1 is in use.

In preparation for allowing architectures to provide their own fallback mask, just return early if we're not using cgroup v2 and allow select_fallback_rq() to figure out the mask by itself.

Cc Li Zefan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Reviewed-by Quentin Perret <[email protected]>

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

---
 kernel/cgroup/cpuset.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/cgroup/cpuset.c +0 -0

index 57b5b5d0a5fd..e970737c3ed2 100644
@@ -3299,9 +3299,11 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
 

kernel/cgroup/cpuset.c void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask) · line 3306

Quoted 9 lines

kernel/cgroup/cpuset.c +4 -2

 void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
 {
+	if (!is_in_v2_mode())
+		return; /* select_fallback_rq will try harder */
+
 	rcu_read_lock();
-	do_set_cpus_allowed(tsk, is_in_v2_mode() ?
-		task_cs(tsk)->cpus_allowed : cpu_possible_mask);
+	do_set_cpus_allowed(tsk, task_cs(tsk)->cpus_allowed);

Why is it safe to return that for cpuset v2?

Quoted 24 lines · Qais Yousef

Quoted 23 lines · Will Deacon

If the scheduler cannot find an allowed CPU for a task, cpuset_cpus_allowed_fallback() will widen the affinity to cpu_possible_mask if cgroup v1 is in use.

In preparation for allowing architectures to provide their own fallback mask, just return early if we're not using cgroup v2 and allow select_fallback_rq() to figure out the mask by itself.

Cc Li Zefan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Reviewed-by Quentin Perret <[email protected]>

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

---
 kernel/cgroup/cpuset.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/cgroup/cpuset.c +0 -0

index 57b5b5d0a5fd..e970737c3ed2 100644
@@ -3299,9 +3299,11 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
 

Quoted 11 lines

kernel/cgroup/cpuset.c void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask) · line 3306

Quoted 9 lines

kernel/cgroup/cpuset.c +4 -2

 void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
 {
+	if (!is_in_v2_mode())
+		return; /* select_fallback_rq will try harder */
+
 	rcu_read_lock();
-	do_set_cpus_allowed(tsk, is_in_v2_mode() ?
-		task_cs(tsk)->cpus_allowed : cpu_possible_mask);
+	do_set_cpus_allowed(tsk, task_cs(tsk)->cpus_allowed);

Why is it safe to return that for cpuset v2?

v1

Because in that case it does cpu_possible_mask, which, if you look at select_fallback_rq(), is exactly what happens when cpuset 'fails' to find a candidate.

Or at least, that's how I read the patch.

Quoted 24 lines · Qais Yousef

Quoted 23 lines · Will Deacon

If the scheduler cannot find an allowed CPU for a task, cpuset_cpus_allowed_fallback() will widen the affinity to cpu_possible_mask if cgroup v1 is in use.

In preparation for allowing architectures to provide their own fallback mask, just return early if we're not using cgroup v2 and allow select_fallback_rq() to figure out the mask by itself.

Cc Li Zefan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Reviewed-by Quentin Perret <[email protected]>

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

---
 kernel/cgroup/cpuset.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/cgroup/cpuset.c +0 -0

index 57b5b5d0a5fd..e970737c3ed2 100644
@@ -3299,9 +3299,11 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
 

Quoted 11 lines

kernel/cgroup/cpuset.c void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask) · line 3306

Quoted 9 lines

kernel/cgroup/cpuset.c +4 -2

 void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
 {
+	if (!is_in_v2_mode())
+		return; /* select_fallback_rq will try harder */
+
 	rcu_read_lock();
-	do_set_cpus_allowed(tsk, is_in_v2_mode() ?
-		task_cs(tsk)->cpus_allowed : cpu_possible_mask);
+	do_set_cpus_allowed(tsk, task_cs(tsk)->cpus_allowed);

Why is it safe to return that for cpuset v2?

v1

Because in that case it does cpu_possible_mask, which, if you look at select_fallback_rq(), is exactly what happens when cpuset 'fails' to find a candidate.

Or at least, that's how I read the patch.

Asymmetric systems may not offer the same level of userspace ISA support across all CPUs, meaning that some applications cannot be executed by some CPUs. As a concrete example, upcoming arm64 big.LITTLE designs do not feature support for 32-bit applications on both clusters.

Modify guarantee_online_cpus() to take task_cpu_possible_mask() into account when trying to find a suitable set of online CPUs for a given task. This will avoid passing an invalid mask to set_cpus_allowed_ptr() during ->attach() and will subsequently allow the cpuset hierarchy to be taken into account when forcefully overriding the affinity mask for a task which requires migration to a compatible CPU.

Cc Li Zefan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

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

---
 include/linux/cpuset.h |  3 ++-
 kernel/cgroup/cpuset.c | 33 +++++++++++++++++++--------------
 2 files changed, 21 insertions(+), 15 deletions(-)

include/linux/cpuset.h +2 -1

index 04c20de66afc..414a8e694413 100644
@@ -15,6 +15,7 @@
 #include <linux/cpumask.h>
 #include <linux/nodemask.h>
 #include <linux/mm.h>
+#include <linux/mmu_context.h>
 #include <linux/jump_label.h>
 
 #ifdef CONFIG_CPUSETS

@@ -184,7 +185,7 @@ static inline void cpuset_read_unlock(void) { }
 static inline void cpuset_cpus_allowed(struct task_struct *p,
                                       struct cpumask *mask)
 {
-       cpumask_copy(mask, cpu_possible_mask);
+       cpumask_copy(mask, task_cpu_possible_mask(p));
 }
 
 static inline void cpuset_cpus_allowed_fallback(struct task_struct *p)

kernel/cgroup/cpuset.c +4 -3

index e970737c3ed2..d30febf1f69f 100644
@@ -372,18 +372,26 @@ static inline bool is_in_v2_mode(void)
 }
 
 /*
- * Return in pmask the portion of a cpusets's cpus_allowed that
- * are online.  If none are online, walk up the cpuset hierarchy
- * until we find one that does have some online cpus.
+ * Return in pmask the portion of a task's cpusets's cpus_allowed that
+ * are online and are capable of running the task.  If none are found,
+ * walk up the cpuset hierarchy until we find one that does have some
+ * appropriate cpus.
  *
  * One way or another, we guarantee to return some non-empty subset
  * of cpu_online_mask.
  *
  * Call with callback_lock or cpuset_mutex held.

kernel/cgroup/cpuset.c +9 -10

+               cpumask_copy(pmask, cpu_online_mask);
+
+       while (!cpumask_intersects(cs->effective_cpus, pmask)) {
                cs = parent_cs(cs);
                if (unlikely(!cs)) {
                        /*

@@ -393,11 +401,10 @@ static void guarantee_online_cpus(struct cpuset *cs, struct cpumask *pmask)
                         * cpuset's effective_cpus is on its way to be
                         * identical to cpu_online_mask.
                         */
-                       cpumask_copy(pmask, cpu_online_mask);
                        return;
                }
        }
-       cpumask_and(pmask, cs->effective_cpus, cpu_online_mask);
+       cpumask_and(pmask, pmask, cs->effective_cpus);
 }
 
 /*

@@ -2176,15 +2183,13 @@ static void cpuset_attach(struct cgroup_taskset *tset)
 
        percpu_down_write(&cpuset_rwsem);
 
-       /* prepare for attach */
-       if (cs == &top_cpuset)
-               cpumask_copy(cpus_attach, cpu_possible_mask);
-       else
-               guarantee_online_cpus(cs, cpus_attach);
-
        guarantee_online_mems(cs, &cpuset_attach_nodemask_to);
 
        cgroup_taskset_for_each(task, css, tset) {
+               if (cs != &top_cpuset)
+                       guarantee_online_cpus(task, cpus_attach);
+               else
+                       cpumask_copy(cpus_attach, task_cpu_possible_mask(task));
                /*
                 * can_attach beforehand should guarantee that this doesn't
                 * fail.  TODO: have a better way to handle failure here

@@ -3280,7 +3285,7 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
 
        spin_lock_irqsave(&callback_lock, flags);
        rcu_read_lock();
-       guarantee_online_cpus(task_cs(tsk), pmask);
+       guarantee_online_cpus(tsk, pmask);
        rcu_read_unlock();
        spin_unlock_irqrestore(&callback_lock, flags);
 }
--

2.29.2.576.ga3fc446d84-goog

Asymmetric systems may not offer the same level of userspace ISA support across all CPUs, meaning that some applications cannot be executed by some CPUs. As a concrete example, upcoming arm64 big.LITTLE designs do not feature support for 32-bit applications on both clusters.

Although userspace can carefully manage the affinity masks for such tasks, one place where it is particularly problematic is execve() because the CPU on which the execve() is occurring may be incompatible with the new application image. In such a situation, it is desirable to restrict the affinity mask of the task and ensure that the new image is entered on a compatible CPU. From userspace's point of view, this looks the same as if the incompatible CPUs have been hotplugged off in the task's affinity mask.

In preparation for restricting the affinity mask for compat tasks on arm64 systems without uniform support for 32-bit applications, introduce force_compatible_cpus_allowed_ptr(), which restricts the affinity mask for a task to contain only compatible CPUs.

Reviewed-by Quentin Perret <[email protected]>

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

---
 include/linux/sched.h |   1 +
 kernel/sched/core.c   | 100 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 86 insertions(+), 15 deletions(-)

include/linux/sched.h +1 -0

index 76cd21fa5501..e42dd0fb85c5 100644
@@ -1653,6 +1653,7 @@ extern int task_can_attach(struct task_struct *p, const struct cpumask *cs_cpus_
 #ifdef CONFIG_SMP
 extern void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask);
 extern int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask);
+extern void force_compatible_cpus_allowed_ptr(struct task_struct *p);
 #else
 static inline void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
 {

kernel/sched/core.c +0 -0

index 92ac3e53f50a..1cfc94be18a9 100644
@@ -1863,25 +1863,19 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
 }

kernel/sched/core.c +43 -8

  */
-static int __set_cpus_allowed_ptr(struct task_struct *p,
-                                 const struct cpumask *new_mask, bool check)
+static int __set_cpus_allowed_ptr_locked(struct task_struct *p,
+                                        const struct cpumask *new_mask,
+                                        bool check,
+                                        struct rq *rq,
+                                        struct rq_flags *rf)
 {
        const struct cpumask *cpu_valid_mask = cpu_active_mask;
        const struct cpumask *cpu_allowed_mask = task_cpu_possible_mask(p);
        unsigned int dest_cpu;
-       struct rq_flags rf;
-       struct rq *rq;
        int ret = 0;
 
-       rq = task_rq_lock(p, &rf);
        update_rq_clock(rq);
 
        if (p->flags & PF_KTHREAD) {

@@ -1936,7 +1930,7 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
        if (task_running(rq, p) || p->state == TASK_WAKING) {
                struct migration_arg arg = { p, dest_cpu };
                /* Need help from migration thread: drop lock and wait. */
-               task_rq_unlock(rq, p, &rf);
+               task_rq_unlock(rq, p, rf);
                stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
                return 0;
        } else if (task_on_rq_queued(p)) {

@@ -1944,20 +1938,96 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
                 * OK, since we're going to drop the lock immediately
                 * afterwards anyway.
                 */
-               rq = move_queued_task(rq, &rf, p, dest_cpu);
+               rq = move_queued_task(rq, rf, p, dest_cpu);
        }
 out:
-       task_rq_unlock(rq, p, &rf);
+       task_rq_unlock(rq, p, rf);
 
        return ret;
 }
 
+/*
+ * Change a given task's CPU affinity. Migrate the thread to a
+ * proper CPU and schedule it away if the CPU it's executing on
+ * is removed from the allowed bitmask.
+ *
+ * NOTE: the caller must have a valid reference to the task, the
+ * task must not exit() & deallocate itself prematurely. The
+ * call is not atomic; no spinlocks may be held.
+ */
+static int __set_cpus_allowed_ptr(struct task_struct *p,
+                                 const struct cpumask *new_mask, bool check)
+{
+       struct rq_flags rf;
+       struct rq *rq;
+
+       rq = task_rq_lock(p, &rf);
+       return __set_cpus_allowed_ptr_locked(p, new_mask, check, rq, &rf);
+}
+
 int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 {
        return __set_cpus_allowed_ptr(p, new_mask, false);
 }
 EXPORT_SYMBOL_GPL(set_cpus_allowed_ptr);
 
+/*
+ * Change a given task's CPU affinity to the intersection of its current
+ * affinity mask and @subset_mask, writing the resulting mask to @new_mask.
+ * If the resulting mask is empty, leave the affinity unchanged and return
+ * -EINVAL.
+ */
+static int restrict_cpus_allowed_ptr(struct task_struct *p,
+                                    struct cpumask *new_mask,
+                                    const struct cpumask *subset_mask)
+{
+       struct rq_flags rf;
+       struct rq *rq;
+
+       rq = task_rq_lock(p, &rf);
+       if (!cpumask_and(new_mask, &p->cpus_mask, subset_mask)) {
+               task_rq_unlock(rq, p, &rf);

kernel/sched/core.c +31 -1

+ */
+void force_compatible_cpus_allowed_ptr(struct task_struct *p)
+{
+       cpumask_var_t new_mask;
+       const struct cpumask *override_mask = task_cpu_possible_mask(p);
+
+       if (!alloc_cpumask_var(&new_mask, GFP_KERNEL))
+               goto out_set_mask;
+
+       if (!restrict_cpus_allowed_ptr(p, new_mask, override_mask))
+               goto out_free_mask;
+
+       /*
+        * We failed to find a valid subset of the affinity mask for the
+        * task, so override it based on its cpuset hierarchy.
+        */
+       cpuset_cpus_allowed(p, new_mask);
+       override_mask = new_mask;
+
+out_set_mask:
+       if (printk_ratelimit()) {
+               printk_deferred("Overriding affinity for process %d (%s) to CPUs %*pbl\n",
+                               task_pid_nr(p), p->comm,
+                               cpumask_pr_args(override_mask));
+       }
+
+       set_cpus_allowed_ptr(p, override_mask);
+out_free_mask:
+       free_cpumask_var(new_mask);
+}
+
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
 #ifdef CONFIG_SCHED_DEBUG
--

2.29.2.576.ga3fc446d84-goog

The aim of this series is to allow 32-bit ARM applications to run on arm64 SoCs where not all of the CPUs support the 32-bit instruction set. Unfortunately, such SoCs are real and will continue to be productised over the next few years at least. I can assure you that I'm not just doing this for fun.

Changes in v5 include:

* Teach cpuset_cpus_allowed() about task_cpu_possible_mask() so that we can avoid returning incompatible CPUs for a given task. This means that sched_setaffinity() can be used with larger masks (like the online mask) from userspace and also allows us to take into account the cpuset hierarchy when forcefully overriding the affinity for a task on execve().

* Honour task_cpu_possible_mask() when attaching a task to a cpuset, so that the resulting affinity mask does not contain any incompatible CPUs (since it would be rejected by set_cpus_allowed_ptr() otherwise).

* Moved overriding of the affinity mask into the scheduler core rather than munge affinity masks directly in the architecture backend.

Quoted 15 lines · Will Deacon

The aim of this series is to allow 32-bit ARM applications to run on arm64 SoCs where not all of the CPUs support the 32-bit instruction set. Unfortunately, such SoCs are real and will continue to be productised over the next few years at least. I can assure you that I'm not just doing this for fun.

Changes in v5 include:

* Teach cpuset_cpus_allowed() about task_cpu_possible_mask() so that we can avoid returning incompatible CPUs for a given task. This means that sched_setaffinity() can be used with larger masks (like the online mask) from userspace and also allows us to take into account the cpuset hierarchy when forcefully overriding the affinity for a task on execve().

Quoted 6 lines

* Honour task_cpu_possible_mask() when attaching a task to a cpuset, so that the resulting affinity mask does not contain any incompatible CPUs (since it would be rejected by set_cpus_allowed_ptr() otherwise).

* Moved overriding of the affinity mask into the scheduler core rather than munge affinity masks directly in the architecture backend.

Hurmph... so if I can still read, this thing will auto truncate the affinity mask to something that only contains compatible CPUs, right?

Assuming our system has 8 CPUs (0xFF), half of which are 32bit capable (0x0F), then, when our native task (with affinity 0x3c) does a fork()+execve() of a 32bit thingy the resulting task has 0x0c.

If that in turn does fork()+execve() of a native task, it will retain the trucated affinity mask (0x0c), instead of returning to the wider mask (0x3c).

IOW, any (accidental or otherwise) trip through a 32bit helper, will destroy user state (the affinity mask: 0x3c).

Should we perhaps split task_struct::cpus_mask, one to keep an original copy of the user state, and one to be an effective cpumask for the task? That way, the moment a task constricts or widens it's task_cpu_possible_mask() we can re-compute the effective mask without loss of information.

Quoted 2 lines · Peter Zijlstra

IOW, any (accidental or otherwise) trip through a 32bit helper, will destroy user state (the affinity mask: 0x3c).

Yes, that's correct, and I agree that it's a rough edge. If you're happy with the idea of adding an extra mask to make this work, then I can start hacking that up

(although I doubt I'll get something out before the new year at this point).

Hi all,

Christmas has come early: it's time for version five of these patches which have previously appeared here:

v1: https://lore.kernel.org/r/[email protected]
v2: https://lore.kernel.org/r/[email protected]
v3: https://lore.kernel.org/r/[email protected]
v4: https://lore.kernel.org/r/[email protected]

and which started life as a reimplementation of some patches from Qais:

https://lore.kernel.org/r/[email protected]

There's also now a nice writeup on LWN:

https://lwn.net/Articles/838339/

and rumours of a feature film are doing the rounds.

[subscriber-only, but if you're reading this then you should really
subscribe.]

The aim of this series is to allow 32-bit ARM applications to run on arm64 SoCs where not all of the CPUs support the 32-bit instruction set. Unfortunately, such SoCs are real and will continue to be productised over the next few years at least. I can assure you that I'm not just doing this for fun.

Changes in v5 include:

* Teach cpuset_cpus_allowed() about task_cpu_possible_mask() so that we can avoid returning incompatible CPUs for a given task. This means that sched_setaffinity() can be used with larger masks (like the online mask) from userspace and also allows us to take into account the cpuset hierarchy when forcefully overriding the affinity for a task on execve().

* Honour task_cpu_possible_mask() when attaching a task to a cpuset, so that the resulting affinity mask does not contain any incompatible CPUs (since it would be rejected by set_cpus_allowed_ptr() otherwise).

* Moved overriding of the affinity mask into the scheduler core rather than munge affinity masks directly in the architecture backend.

* Extended comments and documentation.

* Some renaming and cosmetic changes.

I'm pretty happy with this now, although it still needs review and will require rebasing to play nicely with the SCA changes in -next.

Quoted 16 lines · Will Deacon

Changes in v5 include:

* Teach cpuset_cpus_allowed() about task_cpu_possible_mask() so that we can avoid returning incompatible CPUs for a given task. This means that sched_setaffinity() can be used with larger masks (like the online mask) from userspace and also allows us to take into account the cpuset hierarchy when forcefully overriding the affinity for a task on execve().

* Honour task_cpu_possible_mask() when attaching a task to a cpuset, so that the resulting affinity mask does not contain any incompatible CPUs (since it would be rejected by set_cpus_allowed_ptr() otherwise).

* Moved overriding of the affinity mask into the scheduler core rather than munge affinity masks directly in the architecture backend.

Quoted 6 lines

* Extended comments and documentation.

* Some renaming and cosmetic changes.

I'm pretty happy with this now, although it still needs review and will require rebasing to play nicely with the SCA changes in -next.

I still have concerns about the cpuset v1 handling. Specifically:

1. Attaching a 32bit task to 64bit only cpuset is allowed.

I think the right behavior here is to prevent that as the intersection will appear as offline cpus for the 32bit tasks. So it shouldn't be allowed to move there.

2. Modifying cpuset.cpus could result with empty set for 32bit tasks.

It is a variation of the above, it's just the cpuset transforms into 64bit only after we attach.

I think the right behavior here is to move the 32bit tasks to the nearest ancestor like we do when all cpuset.cpus are hotplugged out.

We could too return an error if the new set will result an empty set for the 32bit tasks. In a similar manner to how it fails if you write a cpu that is offline.

3. If a 64bit task belongs to 64bit-only-cpuset execs a 32bit binary, the 32 tasks will inherit the cgroup setting.

Like above, we should move this to the nearest ancestor.

To simplify the problem for v1, we could say that asym ISA tasks can only live in the root cpuset for v1. This will simplify the solution too since we will only need to ensure that these tasks are moved to the root group on exec and block any future move to anything else. Of course this dictates that such systems must use cpuset v2 if they care. Not a terrible restriction IMO.

Hi Qais,

Quoted 26 lines · Qais Yousef

Quoted 16 lines · Will Deacon

Changes in v5 include:

* Teach cpuset_cpus_allowed() about task_cpu_possible_mask() so that we can avoid returning incompatible CPUs for a given task. This means that sched_setaffinity() can be used with larger masks (like the online mask) from userspace and also allows us to take into account the cpuset hierarchy when forcefully overriding the affinity for a task on execve().

* Honour task_cpu_possible_mask() when attaching a task to a cpuset, so that the resulting affinity mask does not contain any incompatible CPUs (since it would be rejected by set_cpus_allowed_ptr() otherwise).

* Moved overriding of the affinity mask into the scheduler core rather than munge affinity masks directly in the architecture backend.

Quoted 6 lines

* Extended comments and documentation.

* Some renaming and cosmetic changes.

I'm pretty happy with this now, although it still needs review and will require rebasing to play nicely with the SCA changes in -next.

I still have concerns about the cpuset v1 handling. Specifically:

Quoted 5 lines

1. Attaching a 32bit task to 64bit only cpuset is allowed.

I think the right behavior here is to prevent that as the intersection will appear as offline cpus for the 32bit tasks. So it shouldn't be allowed to move there.

Suren or Quantin can correct me if I'm wrong I'm here, but I think Android relies on this working so it's not an option for us to prevent the attach.

I also don't think it really achieves much, since as you point out, the same problem exists in other cases such as execve() of a 32-bit binary, or hotplugging off all 32-bit CPUs within a mixed cpuset. Allowing the attach and immediately reparenting would probably be better, but see below.

Quoted 15 lines

2. Modifying cpuset.cpus could result with empty set for 32bit tasks.

It is a variation of the above, it's just the cpuset transforms into 64bit only after we attach.

I think the right behavior here is to move the 32bit tasks to the nearest ancestor like we do when all cpuset.cpus are hotplugged out.

We could too return an error if the new set will result an empty set for the 32bit tasks. In a similar manner to how it fails if you write a cpu that is offline.

3. If a 64bit task belongs to 64bit-only-cpuset execs a 32bit binary, the 32 tasks will inherit the cgroup setting.

Quoted 1 line

Like above, we should move this to the nearest ancestor.

I considered this when I was writing the patches, but the reality is that by allowing 32-bit tasks to attach to a 64-bit only cpuset (which is required by Android), we have no choice but to expose a new ABI to userspace. This is all gated behind a command-line option, so I think that's fine, but then why not just have the same behaviour as cgroup v2? I don't see the point in creating two new ABIs (for cgroup v1 and v2 respectively) if we don't need

to. If it was _identical_ to the hotplug case, then we would surely just follow the existing behaviour, but it's really quite different in this situation because the cpuset is not empty.

One thing we should definitely do though is add this to the documentation for the command-line option.

Quoted 35 lines · Will Deacon

Hi Qais,

Quoted 31 lines · Qais Yousef

Quoted 16 lines · Will Deacon

Changes in v5 include:

* Teach cpuset_cpus_allowed() about task_cpu_possible_mask() so that we can avoid returning incompatible CPUs for a given task. This means that sched_setaffinity() can be used with larger masks (like the online mask) from userspace and also allows us to take into account the cpuset hierarchy when forcefully overriding the affinity for a task on execve().

* Honour task_cpu_possible_mask() when attaching a task to a cpuset, so that the resulting affinity mask does not contain any incompatible CPUs (since it would be rejected by set_cpus_allowed_ptr() otherwise).

* Moved overriding of the affinity mask into the scheduler core rather than munge affinity masks directly in the architecture backend.

Quoted 6 lines

* Extended comments and documentation.

* Some renaming and cosmetic changes.

I'm pretty happy with this now, although it still needs review and will require rebasing to play nicely with the SCA changes in -next.

I still have concerns about the cpuset v1 handling. Specifically:

1. Attaching a 32bit task to 64bit only cpuset is allowed.

I think the right behavior here is to prevent that as the intersection will appear as offline cpus for the 32bit tasks. So it shouldn't be allowed to move there.

Quoted 2 lines

Suren or Quantin can correct me if I'm wrong I'm here, but I think Android relies on this working so it's not an option for us to prevent the attach.

I don't think so. It's just a matter who handles the error. ie: kernel fix it up silently and effectively make the cpuset a NOP since we don't respect the affinity of the cpuset, or user space pick the next best thing. Since this could return an error anyway, likely user space already handles this.

Quoted 4 lines

I also don't think it really achieves much, since as you point out, the same problem exists in other cases such as execve() of a 32-bit binary, or hotplugging off all 32-bit CPUs within a mixed cpuset. Allowing the attach and immediately reparenting would probably be better, but see below.

I am just wary that we're introducing a generic asymmetric ISA support, so my concerns have been related to making sure the behavior is sane generally. When this gets merged, I can bet more 'fun' hardware will appear all over the place. We're opening the flood gates I'm afraid :p

Quoted 15 lines

Quoted 15 lines

2. Modifying cpuset.cpus could result with empty set for 32bit tasks.

It is a variation of the above, it's just the cpuset transforms into 64bit only after we attach.

I think the right behavior here is to move the 32bit tasks to the nearest ancestor like we do when all cpuset.cpus are hotplugged out.

We could too return an error if the new set will result an empty set for the 32bit tasks. In a similar manner to how it fails if you write a cpu that is offline.

3. If a 64bit task belongs to 64bit-only-cpuset execs a 32bit binary, the 32 tasks will inherit the cgroup setting.

Quoted 8 lines

Quoted 1 line

Like above, we should move this to the nearest ancestor.

I considered this when I was writing the patches, but the reality is that by allowing 32-bit tasks to attach to a 64-bit only cpuset (which is required by Android), we have no choice but to expose a new ABI to userspace. This is all gated behind a command-line option, so I think that's fine, but then why not just have the same behaviour as cgroup v2? I don't see the point in creating two new ABIs (for cgroup v1 and v2 respectively) if we don't need

Ultimately it's up to Tejun and Peter I guess. I thought we need to preserve the v1 behavior for the new class of tasks. I won't object to the new ABI myself. Maybe we just need to make the commit messages and cgroup-v1 documentation reflect that explicitly.

Quoted 3 lines

to. If it was _identical_ to the hotplug case, then we would surely just follow the existing behaviour, but it's really quite different in this situation because the cpuset is not empty.

It is actually effectively empty for those tasks. But I see that one could look at it from two different angles.

Quoted 2 lines

One thing we should definitely do though is add this to the documentation for the command-line option.

+1

By the way, should the command-line option be renamed to something more generic? This has already grown beyond just enabling the support for one isolated case. No strong opinion, just a suggestion.

Thanks

--
Qais Yousef