From: Will Deacon <will@kernel.org> To: Qais Yousef <qais.yousef@arm.com>, surenb@google.com Cc: linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, Catalin Marinas <catalin.marinas@arm.com>, Marc Zyngier <maz@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Peter Zijlstra <peterz@infradead.org>, Morten Rasmussen <morten.rasmussen@arm.com>, Quentin Perret <qperret@google.com>, Tejun Heo <tj@kernel.org>, Li Zefan <lizefan@huawei.com>, Johannes Weiner <hannes@cmpxchg.org>, Ingo Molnar <mingo@redhat.com>, Juri Lelli <juri.lelli@redhat.com>, Vincent Guittot <vincent.guittot@linaro.org>, kernel-team@android.com Subject: Re: [PATCH v5 00/15] An alternative series for asymmetric AArch32 systems Date: Wed, 16 Dec 2020 14:14:51 +0000 [thread overview] Message-ID: <20201216141450.GA16421@willie-the-truck> (raw) In-Reply-To: <20201216111646.omrxyhbobejzqprh@e107158-lin.cambridge.arm.com> Hi Qais, On Wed, Dec 16, 2020 at 11:16:46AM +0000, Qais Yousef wrote: > On 12/08/20 13:28, Will Deacon wrote: > > 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. > > 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. 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. > 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. 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. > 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. Sadly, I think Android is still on cgroup v1 for cpuset, but Suren will know better the status of cgroup v2 for cpusets. If it's just around the corner, then maybe we could simplify things here. Suren? Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will@kernel.org> To: Qais Yousef <qais.yousef@arm.com>, surenb@google.com Cc: linux-arch@vger.kernel.org, Marc Zyngier <maz@kernel.org>, kernel-team@android.com, Vincent Guittot <vincent.guittot@linaro.org>, Juri Lelli <juri.lelli@redhat.com>, Quentin Perret <qperret@google.com>, Peter Zijlstra <peterz@infradead.org>, Catalin Marinas <catalin.marinas@arm.com>, Johannes Weiner <hannes@cmpxchg.org>, linux-kernel@vger.kernel.org, Li Zefan <lizefan@huawei.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Tejun Heo <tj@kernel.org>, Ingo Molnar <mingo@redhat.com>, Morten Rasmussen <morten.rasmussen@arm.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v5 00/15] An alternative series for asymmetric AArch32 systems Date: Wed, 16 Dec 2020 14:14:51 +0000 [thread overview] Message-ID: <20201216141450.GA16421@willie-the-truck> (raw) In-Reply-To: <20201216111646.omrxyhbobejzqprh@e107158-lin.cambridge.arm.com> Hi Qais, On Wed, Dec 16, 2020 at 11:16:46AM +0000, Qais Yousef wrote: > On 12/08/20 13:28, Will Deacon wrote: > > 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. > > 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. 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. > 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. 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. > 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. Sadly, I think Android is still on cgroup v1 for cpuset, but Suren will know better the status of cgroup v2 for cpusets. If it's just around the corner, then maybe we could simplify things here. Suren? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-12-16 14:15 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-08 13:28 [PATCH v5 00/15] An alternative series for asymmetric AArch32 systems Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 01/15] arm64: cpuinfo: Split AArch32 registers out into a separate struct Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 02/15] arm64: Allow mismatched 32-bit EL0 support Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 03/15] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched " Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 04/15] arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 05/15] arm64: Advertise CPUs capable of running 32-bit applications in sysfs Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 06/15] sched: Introduce task_cpu_possible_mask() to limit fallback rq selection Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 07/15] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1 Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-17 12:15 ` Qais Yousef 2020-12-17 12:15 ` Qais Yousef 2020-12-17 13:44 ` Peter Zijlstra 2020-12-17 13:44 ` Peter Zijlstra 2020-12-17 14:59 ` Will Deacon 2020-12-17 14:59 ` Will Deacon 2020-12-17 15:00 ` Qais Yousef 2020-12-17 15:00 ` Qais Yousef 2020-12-08 13:28 ` [PATCH v5 08/15] cpuset: Honour task_cpu_possible_mask() in guarantee_online_cpus() Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-28 3:54 ` Suren Baghdasaryan 2020-12-28 3:54 ` Suren Baghdasaryan 2020-12-08 13:28 ` [PATCH v5 09/15] sched: Reject CPU affinity changes based on task_cpu_possible_mask() Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 10/15] sched: Introduce force_compatible_cpus_allowed_ptr() to limit CPU affinity Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-28 4:29 ` Suren Baghdasaryan 2020-12-28 4:29 ` Suren Baghdasaryan 2020-12-08 13:28 ` [PATCH v5 11/15] arm64: Implement task_cpu_possible_mask() Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 12/15] arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit EL0 Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 13/15] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 14/15] arm64: Hook up cmdline parameter to allow mismatched 32-bit EL0 Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-08 13:28 ` [PATCH v5 15/15] arm64: Remove logic to kill 32-bit tasks on 64-bit-only cores Will Deacon 2020-12-08 13:28 ` Will Deacon 2020-12-15 17:36 ` [PATCH v5 00/15] An alternative series for asymmetric AArch32 systems Peter Zijlstra 2020-12-15 17:36 ` Peter Zijlstra 2020-12-15 18:50 ` Will Deacon 2020-12-15 18:50 ` Will Deacon 2020-12-17 10:55 ` Peter Zijlstra 2020-12-17 10:55 ` Peter Zijlstra 2020-12-16 11:16 ` Qais Yousef 2020-12-16 11:16 ` Qais Yousef 2020-12-16 14:14 ` Will Deacon [this message] 2020-12-16 14:14 ` Will Deacon 2020-12-16 16:48 ` Qais Yousef 2020-12-16 16:48 ` Qais Yousef 2020-12-16 18:21 ` Suren Baghdasaryan 2020-12-16 18:21 ` Suren Baghdasaryan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201216141450.GA16421@willie-the-truck \ --to=will@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=gregkh@linuxfoundation.org \ --cc=hannes@cmpxchg.org \ --cc=juri.lelli@redhat.com \ --cc=kernel-team@android.com \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lizefan@huawei.com \ --cc=maz@kernel.org \ --cc=mingo@redhat.com \ --cc=morten.rasmussen@arm.com \ --cc=peterz@infradead.org \ --cc=qais.yousef@arm.com \ --cc=qperret@google.com \ --cc=surenb@google.com \ --cc=tj@kernel.org \ --cc=vincent.guittot@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.