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>, 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
Date: Mon, 7 Nov 2022 11:11:15 +0100 [thread overview]
Message-ID: <20221107101115.GA3279@lothringen> (raw)
In-Reply-To: <f72b4d5d-493d-916-5d19-2bf87e8c41e1@linutronix.de>
On Mon, Nov 07, 2022 at 09:11:11AM +0100, Anna-Maria Behnsen wrote:
> 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.
Ok.
>
> > 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?
That's right indeed.
>
> 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.
Yeah that sounds better.
> 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...
Ok, if we have some pinned initializers such as DECLARE_DELAYED_WORK_PINNED()
and DECLARE_DEFERRABKE_WORK_PINNED() then I think that cleans the situation.
Sounds good, thanks!
>
> Thanks,
>
> Anna-Maria
>
next prev parent reply other threads:[~2022-11-07 10:11 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-04 14:57 [PATCH v4 00/16] timer: Move from a push remote at enqueue to a pull at expiry model Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 01/16] tick-sched: Warn when next tick seems to be in the past Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 02/16] timer: Move store of next event into __next_timer_interrupt() Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 03/16] timer: Split next timer interrupt logic Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 04/16] timer: Rework idle logic Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 05/16] add_timer_on(): Make sure callers have TIMER_PINNED flag Anna-Maria Behnsen
2022-11-04 16:43 ` Frederic Weisbecker
2022-11-07 8:11 ` Anna-Maria Behnsen
2022-11-07 10:11 ` Frederic Weisbecker [this message]
2022-11-04 14:57 ` [PATCH v4 06/16] timer: Keep the pinned timers separate from the others Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 07/16] timer: Retrieve next expiry of pinned/non-pinned timers seperately Anna-Maria Behnsen
2022-11-07 11:58 ` Frederic Weisbecker
2022-11-04 14:57 ` [PATCH v4 08/16] timer: Rename get_next_timer_interrupt() Anna-Maria Behnsen
2022-11-07 12:13 ` Frederic Weisbecker
2022-11-04 14:57 ` [PATCH v4 09/16] timer: Split out "get next timer interrupt" functionality Anna-Maria Behnsen
2022-11-07 12:42 ` Frederic Weisbecker
2022-11-08 15:30 ` Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 10/16] timer: Add get next timer interrupt functionality for remote CPUs Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 11/16] timer: Restructure internal locking Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 12/16] timer: Check if timers base is handled already Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 13/16] tick/sched: Split out jiffies update helper function Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 14/16] timer: Implement the hierarchical pull model Anna-Maria Behnsen
2022-11-07 22:07 ` Frederic Weisbecker
2022-11-08 16:16 ` Anna-Maria Behnsen
2022-11-09 17:12 ` Frederic Weisbecker
2022-11-08 10:47 ` Frederic Weisbecker
2022-11-08 17:02 ` Anna-Maria Behnsen
2022-11-09 17:13 ` Frederic Weisbecker
2022-11-08 11:48 ` Frederic Weisbecker
2022-11-09 16:39 ` Frederic Weisbecker
2022-11-10 6:34 ` Anna-Maria Behnsen
2022-11-14 13:09 ` Frederic Weisbecker
2022-11-15 11:31 ` Frederic Weisbecker
2022-11-24 7:47 ` Anna-Maria Behnsen
2022-11-28 16:20 ` Frederic Weisbecker
2022-11-29 10:30 ` Frederic Weisbecker
2022-11-16 13:40 ` Frederic Weisbecker
2022-11-04 14:57 ` [PATCH v4 15/16] timer_migration: Add tracepoints Anna-Maria Behnsen
2022-11-04 14:57 ` [PATCH v4 16/16] timer: Always queue timers on the local CPU Anna-Maria Behnsen
2022-11-08 4:37 ` [PATCH v4 00/16] timer: Move from a push remote at enqueue to a pull at expiry model Pavan Kondeti
2022-11-08 15:06 ` Anna-Maria Behnsen
2022-11-08 16:04 ` Pavan Kondeti
2022-11-08 17:39 ` Anna-Maria Behnsen
2022-11-08 18:48 ` Pavan Kondeti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221107101115.GA3279@lothringen \
--to=frederic@kernel.org \
--cc=Jason@zx2c4.com \
--cc=anna-maria@linutronix.de \
--cc=arjan@infradead.org \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=edumazet@google.com \
--cc=fweisbec@gmail.com \
--cc=hpa@zytor.com \
--cc=jiangshanlai@gmail.com \
--cc=jstultz@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rafael.j.wysocki@intel.com \
--cc=riel@surriel.com \
--cc=sboyd@kernel.org \
--cc=tglx@linutronix.de \
--cc=tj@kernel.org \
--cc=tytso@mit.edu \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).