Messages in this thread | | | Date | Mon, 7 Nov 2022 09:11:11 +0100 (CET) | From | Anna-Maria Behnsen <> | Subject | Re: [PATCH v4 05/16] add_timer_on(): Make sure callers have TIMER_PINNED flag |
| |
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
| |