lkml.org 
[lkml]   [2020]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1
    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

    \
     
     \ /
      Last update: 2020-12-02 12:35    [W:6.916 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site