Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752271AbdGaRzH (ORCPT <rfc822;w@1wt.eu>);
        Mon, 31 Jul 2017 13:55:07 -0400
Received: from mail-oi0-f50.google.com ([209.85.218.50]:35449 "EHLO
        mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752191AbdGaRzE (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 31 Jul 2017 13:55:04 -0400
MIME-Version: 1.0
In-Reply-To: <20170731164227.GA7922@li70-116.members.linode.com>
References: <CAJWu+opFTvnGnYyRo9eYZJUxiHdoMCMoadUvnX3p=6ypwWX_Jw@mail.gmail.com>
 <20170630142815.GA9743@destiny> <1498842140.15161.66.camel@gmail.com>
 <CAJWu+oo7AY+PYqdwSoW6ToRe2o40MhT1wSCrRU8nkQuH0cFO4w@mail.gmail.com>
 <1501340845.7706.168.camel@gmail.com> <CAJWu+ooaqWVTad8JNc4gnCAjkgZj4toCb7RrCKNQfm=x3tD1Gw@mail.gmail.com>
 <CAJWu+ore7pfzMw4dz=y4YMi5YGs_b5DwjA6=GfXFsqqJN10gWQ@mail.gmail.com>
 <CAJWu+ookYPvXrEF=bDW_aui30NV91hMBawy=+GZn_xEiriU0Ag@mail.gmail.com>
 <20170731122149.GA7539@li70-116.members.linode.com> <CAJWu+oorSt-vDwVGHBNtJmLJEQD8Vni79QYO0=YnvBpZUy8u9A@mail.gmail.com>
 <20170731164227.GA7922@li70-116.members.linode.com>
From: Joel Fernandes <joelaf@google.com>
Date: Mon, 31 Jul 2017 10:55:03 -0700
Message-ID: <CAJWu+ooShdrBo-5hdiKOywSQM+v2e_k32GqvcozgrXmakApzWw@mail.gmail.com>
Subject: Re: wake_wide mechanism clarification
To: Josef Bacik <josef@toxicpanda.com>
Cc: Mike Galbraith <umgwanakikbuti@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>, Juri Lelli <Juri.Lelli@arm.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Patrick Bellasi <patrick.bellasi@arm.com>,
        Brendan Jackman <brendan.jackman@arm.com>,
        Chris Redpath <Chris.Redpath@arm.com>,
        Michael Wang <wangyun@linux.vnet.ibm.com>,
        Matt Fleming <matt@codeblueprint.co.uk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4321
Lines: 91

On Mon, Jul 31, 2017 at 9:42 AM, Josef Bacik <josef@toxicpanda.com> wrote:
<snip>
>>
>> >
>> > So why do you care about wake_wide() anyway?  Are you observing some problem
>> > that you suspect is affected by the affine wakeup stuff?  Or are you just trying
>>
>> I am dealing with an affine wake up issue, yes.
>>
>> > to understand what is going on for fun?  Cause if you are just doing this for
>> > fun you are a very strange person, thanks,
>>
>> Its not just for fun :) Let me give you some background about me, I
>> work in the Android team and one of the things I want to do is to take
>> an out of tree patch that's been carried for some time and post a more
>> upstreamable solution - this is related to wake ups from the binder
>> driver which does sync wake ups (WF_SYNC). I can't find the exact out
>> of tree patch publicly since it wasn't posted to a list, but the code
>> is here [1]. What's worse is I have recently found really bad issues
>> with this patch itself where runnable times are increased. I should
>> have provided this background earlier (sorry that I didn't, my plan
>> was to trigger a separate discussion about the binder sync wake up
>> thing as a part of a patch/proposal I want to post - which I plan to
>> do so). Anyway, as a part of this effort, I want to understand
>> wake_wide() better and "respect" it since it sits in the wake up path
>> and I wanted to my proposal to work well with it, especially since I
>> want to solve this problem in an upstream-friendly way.
>>
>> The other reason to trigger the discussion, is, I have seen
>> wake_wide() enough number of times and asked enough number of folks
>> how it works that it seems sensible to ask about it here (I was also
>> suggested to ask about wake_wide on LKML because since few people
>> seemingly understand how it works) and hopefully now its a bit better
>> understood.
>>
>> I agree with you that instead of spending insane amounts of time on
>> wake_wide itself, its better to directly work on a problem and collect
>> some data - which is also what I'm doing, but I still thought its
>> worth doing some digging into wake_wide() during some free time I had,
>> thanks.
>>
>
> Ok so your usecase is to _always_ wake affine if we're doing a sync wakeup.  I
> _think_ for your case it's best to make wake_affine() make this decision, and
> you don't want wake_wide() to filter out your wakeup as not-affine?  So perhaps
> just throw a test in there to not wake wide if WF_SYNC is set.  This makes

Hmm I was actually thinking that since 'sync' is more of a hint, that
we do a wake_wide() first anyway since its already so conservative,
and for the times it does resort to wide, its probably the right
decision from a scheduling standpoint to avoid affine and avoid too
many tasks too quickly. Do you think that's a fair?

I tried a quick patch and doing wake_wide first, and then checking for
sync does seem to work well.

> logical sense to me as synchronous wakeups are probably going to want to be
> affine wakeups, and then we can rely on wake_affine() to do the load checks to
> make sure it really makes sense.  How does that sound?  Thanks,

Yep that sounds good and I will try that.

What I was thinking was do the regular wake_wide and wake_affine
checks, and then do something like this:

in select_task_rq_fair, before calling select_idle_sibling, do
something like this to check if only 1 task is running on the waker's
CPU (this is after doing the wake_wide and wake_affine checks)

+               idle_sync = (sync && (new_cpu == cpu) &&
+                            cpu_rq(cpu)->nr_running == 1);

and then in select_idle_sibling, something like:

+               /*
+                * If the previous and target CPU share cache and a sync wakeup
+                * is requested and the CPU is about to goto idle, because it
+                * has only the waker running which requested sync, the target
+                * is a better choice for cache affinity and keeping task's
+                * previous core idle and low power state.
+                */
+               if (idle_sync && cpus_share_cache(prev, target))
+                       return target;
+

I haven't tested this patch though so I'm not sure it works well yet,
but just sharing the idea. What do you think?

thanks,

-Joel