On 12/01/20 15:56, Quentin Perret wrote: > On Tuesday 01 Dec 2020 at 14:11:21 (+0000), Qais Yousef wrote: > > AFAIU, OEMs have to define their cpusets. So it makes sense to me for them to > > define it correctly if they want to enable asym aarch32. > > > > Systems that don't care about this feature shouldn't be affected. If they do, > > then I'm missing something. > > Right, but there are 2 cases for 32 bit tasks in Android: > > 1. 32 bit apps; these are not an issue, the Android framework knows > about them and it's fine to expect it to setup cpusets accordingly > IMO. > > 2. 64 bit apps that also happen to have a 32 bit binary payload, and > exec into it. The Android framework has no visibility over that, > all it sees is a 64 bit app. Sadly we can't detect this stupid > pattern, but we need these to remain somewhat functional. > > I was only talking about 2. the whole time, sorry if that wasn't clear. > With that said, see below for the discussion about cpuset/hotplug. Yep, I was referring to 2 too. I found out about the app that embeds the 32 bit binary, it was our major concern if we go with user space managing affinities. > > > We deal with hotplug by not allowing one of the aarch32 cpus from going > > offline. > > Sure, but that would only work if we have that 32 bit CPU present in > _all_ cpusets, no? What I'd like to avoid is to keep a (big) 32 > bit CPU in the background cpuset of 64 bit tasks. That would make that > big CPU available to _all_ 64 bit apps in the background, whether they > need 32 bit support or not, because again we cannot distinguish them. > And yeah, I expect this to be not go down well in practice. > > > So, if we're going to support this, a requirement for Android is that > some cpusets will be 64 bit only, and it's possible that we'll exec into > 32 bit from within these cpusets. It's an edge case, we don't really > want to optimize for it, but it needs to not fall apart completely. > I'm not fundamentally against doing smarter things at all, I'm saying we > (Android) just don't _need_ smarter things ATM, so we may want to keep > it simple. Fair enough. But in that case I find it neater to fix the affinities up in the arch code as a specific solution. I'm not seeing there's a difference in the end results between the two implementations if we don't address these issues :( > > My point in the previous message is, if we're accepting this for exec, > a logical next step could be to accept it for cpuset migrations too. > Failing the cgroup migration is hard since: there is no guarantee the > source cpuset has 32 bit CPUs anyway (assuming the exec'd task is kept > in the same cpuset), so why bother; userspace just doesn't know there > are 32 bit tasks in an app and would keep trying to migrate it to 64 bit > cpuset over and over again; you could end up with apps being stuck > halfway through a top-app->background transition where some tasks have > migrated but not others, ... > > It's a bit of a mess :/ It is. I think I addressed these concerns in other parts from my previous email. Judging by your reply below I think you see what I was talking about and we're more on the same page now :-) > > > <snip> > > For hotplug we have to make sure a single cpu stays alive. The fallback you're > > talking about should still work the same if the task is not attached to > > a cpuset. Just it has to take the intersection with the > > arch_task_cpu_possible_cpu() into account. > > Yep, agreed, there's probably room for improvement there. > > > For cpusets, if hotunplug results in an empty cpuset, then all tasks are moved > > to the nearest ancestor if I read the code correctly. In our case, only 32bit > > tasks have to move out to retain this behavior. Since now for the first time we > > have tasks that can't run on all cpus. > > > > Which by the way might be the right behavior for 64bit tasks execing 32bit > > binary in a 64bit only cpuset. I suggested SIGKILL'ing them but maybe moving > > them to the nearest ancestor too is more aligned with the behavior above. > > Hmm, I guess that means putting all 32-bit-execd-from-64-bit tasks in > the root group in Android. I'll try and check the implications, but that > might be just fine... Sounds like a sensible behaviour to me anyways. It'd be only the compat tasks that will have to move to root group. And only for those minority of apps that embed a 32bit binary. I think the impact is minimum. And I think the behavior makes sense generically. Thanks! -- Qais Yousef
