Re: [PATCH v4 14/16] timer: Implement the hierarchical pull model

From: Frederic Weisbecker <frederic@kernel.org>
To: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: linux-kernel@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	John Stultz <jstultz@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Eric Dumazet <edumazet@google.com>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	Arjan van de Ven <arjan@infradead.org>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Rik van Riel <riel@surriel.com>
Subject: Re: [PATCH v4 14/16] timer: Implement the hierarchical pull model
Date: Wed, 9 Nov 2022 18:12:25 +0100	[thread overview]
Message-ID: <20221109171225.GB300561@lothringen> (raw)
In-Reply-To: <c0afb1-2a23-f595-3e30-bb95c39e8e65@linutronix.de>

On Tue, Nov 08, 2022 at 05:16:11PM +0100, Anna-Maria Behnsen wrote:
> On Mon, 7 Nov 2022, Frederic Weisbecker wrote:
> > > +		}
> > > +	}
> > > +
> > >  	/* We need to mark both bases in sync */
> > >  	base_local->is_idle = base_global->is_idle = is_idle;
> > 
> > Do we still need to maintain base_global->is_idle ?
> 
> is_idle information is required in trigger_dyntick_cpu(). I made a mistake
> with the hunk in trigger_dyntick_cpu() introduced in this patch. Because
> after this patch, global timers are still enqueued on any CPU because
> crystallball still exists. trigger_dyntick_cpu() is also required for non
> pinned timers. I need to move the hunk of trigger_dyntick_cpu() into the
> last patch of the queue where crystallball is removed during enqueue and
> there update also this line. Then I will drop the update of
> base_global->is_idle in timer_clear_idle() as well.
> 
> Sorry. This went wrong during splitting and folding the queue back and
> forwards...

Sure, no problem, just asked because I wanted to be sure I wasn't missing
something. I suggest waiting for broader testing after the current batch
lands upstream before removing the crystalball :-)

> 
> > (I'm going to do daily reviews on this patch because it's quite dense :)
> 
> Thanks! I try to answer your questions fast. Let me know when you are done
> or when you need an updated version for further review :)

You have at least one week ahead of you, the time for me to recollect my brain
throughout that patch. Indeed no need to repost now, I'll have some more questions
for sure.

I like the design, so I'm merely just chasing correctness issues and things that
might be made clearer.

I'm more afraid of what testing will tell wrt. performance and powersaving but,
fortunately, the world will scale much better than me to do this :)

Thanks!

> 
> Thanks,
> 
> 	Anna-Maria
> 