Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Nov 2022 09:11:11 +0100 (CET)
From:   Anna-Maria Behnsen <anna-maria@linutronix.de>
To:     Frederic Weisbecker <frederic@kernel.org>
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>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Theodore Ts'o <tytso@mit.edu>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Stephen Boyd <sboyd@kernel.org>, Tejun Heo <tj@kernel.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>
Subject: Re: [PATCH v4 05/16] add_timer_on(): Make sure callers have TIMER_PINNED
 flag
In-Reply-To: <20221104164342.GA1440400@lothringen>
Message-ID: <f72b4d5d-493d-916-5d19-2bf87e8c41e1@linutronix.de>
References: <20221104145737.71236-1-anna-maria@linutronix.de> <20221104145737.71236-6-anna-maria@linutronix.de> <20221104164342.GA1440400@lothringen>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 4 Nov 2022, Frederic Weisbecker wrote:

> On Fri, Nov 04, 2022 at 03:57:26PM +0100, Anna-Maria Behnsen wrote:
> > The implementation of the hierachical timer pull model will change the
> > timer bases per CPU. Timers, that have to expire on a specific CPU, require
> > the TIMER_PINNED flag. Otherwise they will be queued on the dedicated CPU
> > but in global timer base and those timers could also expire on other
> > CPUs. Timers with TIMER_DEFERRABLE flag end up in a separate base anyway
> > and are executed on the local CPU only.
> > 
> > Therefore add the missing TIMER_PINNED flag for those callers who use
> > add_timer_on() without the flag. No functional change.
> 
> You're fixing the current callers but what about the future ones?
> 
> add_timer_on() should always guarantee that a timer runs on the
> right destination, which is not the case after your patchset if the
> timer hasn't been set to TIMER_PINNED.
> 
> Therefore I think we should either have:
> 
> * add_timer_on() enforce TIMER_PINNED (doesn't work because if the timer is
>   later called with mod_timer(), we should expect it to run anywhere)
> 
> or
> 
> * add_timer_on() warns if !TIMER_PINNED

This is already part of the last patch of the queue where also the
crystalball logic is removed. But the patch where I added the WARN_ONCE()
might be the wrong patch, it should be better part of the next patch where
the new timer bases are introduced.

> or
> 
> * have an internal flag TIMER_LOCAL, that is turned on when
>   add_timer_on() is called or add_timer()/mod_timer() is called
>   on a TIMER_PINNED. Otherwise it is turned off.
> 
> The last solution should work with existing API and you don't need to
> chase the current and future users of add_timer_on().

With the last approach it doesn't matter how the timer is setup. Everything
is done by timer code implicitly. When a future caller uses add_timer_on()
and wants to modfiy this "implicitly pinned timer", he will call
mod_timer() and the timer is no longer pinned (if it do not end up in the
same bucket it was before). For a user this does not seems to be very
obvious, or am I wrong?

But if the caller sets up the timer correctly we do not need this extra
timer flag. With the WARN_ONCE() in place, callers need to do the timer
setup properly and it is more clear to the caller what should be done.

BTW, the hunk in this patch for the workqueue is also not a final fix in my
opinion. I'm preparing a cleanup queue (it's part of the deferrable cleanup
queue), where I want to set the timer flags properly when
initializing/defining the workers. I should have added a comment here...

Thanks,

	Anna-Maria