Message-ID: <20221108160457.GA28920@hu-pkondeti-hyd.qualcomm.com>
Date:   Tue, 8 Nov 2022 21:34:57 +0530
From:   Pavan Kondeti <quic_pkondeti@...cinc.com>
To:     Anna-Maria Behnsen <anna-maria@...utronix.de>
CC:     Pavan Kondeti <quic_pkondeti@...cinc.com>,
        <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        John Stultz <jstultz@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Eric Dumazet <edumazet@...gle.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Arjan van de Ven <arjan@...radead.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        "Frederic Weisbecker" <fweisbec@...il.com>,
        Rik van Riel <riel@...riel.com>
Subject: Re: [PATCH v4 00/16] timer: Move from a push remote at enqueue to a
 pull at expiry model

Hi Anna-Maria,

On Tue, Nov 08, 2022 at 04:06:15PM +0100, Anna-Maria Behnsen wrote:
> On Tue, 8 Nov 2022, Pavan Kondeti wrote:
> 
> > Hi Anna-Maria,
> > 
> > On Fri, Nov 04, 2022 at 03:57:21PM +0100, Anna-Maria Behnsen wrote:
> > > Next Steps
> > > ~~~~~~~~~~
> > > 
> > > Simple deferrable timers are no longer required as they can be converted to
> > > global timers. If a CPU goes idle, a formerly deferrable timer will not
> > > prevent the CPU to sleep as long as possible. Only the last migrator CPU
> > > has to take care of them. Deferrable timers with timer pinned flags needs
> > > to be expired on the specified CPU but must not prevent CPU from going
> > > idle. They require their own timer base which is never taken into account
> > > when calculating the next expiry time. This conversation and required
> > > cleanup will be done in a follow up series.
> > > 
> > 
> > Taking non-pinned deferrable timers case, they are queued on their own base
> > and its expiry is not taken into account while programming the next timer
> > event during idle.
> 
> If CPU is not the last CPU going idle, then yes.

What is special with last CPU that is going idle? Sorry, it is not clear where
the deferrable timer expiry is taken into account while programming the next
wakeup event?

forward_and_idle_timer_bases()->tmigr_cpu_deactivate() is only taking global
timer expiry (deferrable timers are NOT queued on global base) and comparing
against the local base expiry. This makes me think that we are not taking
deferrable timers expiry into account, which is correct IMO.

>  
> > Can you elaborate on "Simple deferrable timers are no longer required as they
> > can be converted to global timers" statement?
> 
> Global timers do not prevent CPU from going idle. Same thing that
> deferrable timers does right now. Global timers are queued into the
> hierarchy and migrator takes care of expiry when CPU goes idle. The main
> change of behavoir with global timers compared to deferrable timers is,
> that they will expire in time and not necessarily on the CPU they were
> enqueued. Deferrable timers were expired, only when the CPU was awake and
> always on the CPU they have been enqueued.

Thanks. This is very clear. A deferrable timer (upstream or your patches)
only expire on busy and local CPU. A CPU will not come out of idle, just to
serve a deferrable timer.

> 
> > Though they can be on global base, we still need to find a way to distinguish
> > them aginst the normal global timers so that the last migrator can program
> > the next timer event without taking these deferrable timer expiry into
> > account? IOW, a deferrable timer should not bring a completely idle system out
> > of idle to serve the deferrable timer.
> 
> This behavior will change a little. If the system is completely idle, the
> last migrator CPU has to handle the first global timer even if it is a
> formerly deferrable and non pinned timer.
> 
> > When the deferrable timers will be queued on global base, once a CPU comes out
> > of idle and serve the timers on global base, the deferrable timers also would
> > be served. This is a welcoming change. We would see a truly deferrable global
> > timer something we would be interested in. [1] has some background on this.
> 
> Serving the deferrable timers once a CPU comes out of idle is already the
> case even without the timer migration hierarchy. See upstream version of
> run_local_timers().

However upstream version does not wake a CPU just to serve a deferrable timer.
But it seems if we consider a deferrable timer just as another global timer,
sure it will not prevent the local CPU going idle but there would be one CPU
(thus, the system) that pays the penalty.

> 
> > [1]
> > https://lore.kernel.org/lkml/1430188744-24737-1-git-send-email-joonwoop@codeaurora.org/
> 
> As far as I understand the problem you are linking to correctly, you want
> to have a real "unbound" solution for deferrable or delayed work. This is
> what you get with the timer migration hierarchy and when enqueuing
> deferrable timers into global timer base. Timers are executed on the
> migrator CPU because this CPU is not idle - doesn't matter where they have
> been queued before.
> 
> It might be possible, that a fomerly deferrable timer enforces the last CPU
> going idle to come back out of idle. But the question is, how often does
> this occur in contrast to a wakeup cause by a non deferrable timer?  If you
> have a look at the timers in kernel you have 64 deferrable timers (this
> number also contain the deferrable and pinned timers). There are 7 timers
> with only TIMER_PINNED flag and some additional using the add_timer_on() to
> be enqueued on a dedicated CPU. But in total we have more than 1000
> timers. Sure - in the end, numbers hardly depends on the selected kernel
> config...

I will give an example here. Lets say we have 4 CPUs in a system. There is a
devfreq governor driver that configures a delayed work for every 20 msec.

#1 When the system is busy, this *deferrable* timer expires at the 20 msec
boundary. However, when the system is idle (i.e no use case is running but
system does not enter global suspend because of other reasons like display
ON etc), we don't expect this deferrable timer to expire at every 20 msec. 

With your proposal, we endup seeing the system (last CPU that enters idle)
coming out of idle for every 20 msec which is not desirable.

#2 Today, deferrable is local to CPU. Irrespective of the activity on the
other CPUs, this deferrable timer does not expire as long as the local CPU
is idle for whatever reason. That is definitly not the devfreq governor
expectation. The intention is to save power when system is idle but serving
the purpose when it is relatively busy.

> 
> Side note: One big problem of deferrable timers disappear with this
> approach. All deferrable timers _WILL_ expire. Even if CPU where they have
> been enqueued does not come back out of idle. Only deferrable and pinned
> timers will still have this problem.
> 
Yes, this is a welcoming change. Solves the #2 problem as mentioned above.

Thanks,
Pavan

Powered by blists - more mailing lists