[PATCH] timer: document TIMER_PINNED

[PATCH] timer: document TIMER_PINNED

[Peter Xu]

The flag is very easy to understand but the interface is not,
especially the assumption that add_timer_on() must be used to
guarantee a correct behavior.

For example, currently if we setup a pinned timer but later on we call
mod_timer() upon the pinned timer, the mod_timer() will still try to
run the timer on the current processor and migrate the timer if
necessary.  In other words, the suggested way to arm a pinned timer
should be add_timer_on() always.  mod_timer() can be used in this case
only if current processor is the one that we want to pin the timer on.

Document it a bit with the definition of TIMER_PINNED so that all
future users will use it correctly.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: linux-kernel@vger.kernel.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/linux/timer.h | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index 7b066fd38248..4a3408a05f86 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -36,19 +36,26 @@ struct timer_list {
 #define __TIMER_LOCKDEP_MAP_INITIALIZER(_kn)
 #endif
 
-/*
- * A deferrable timer will work normally when the system is busy, but
- * will not cause a CPU to come out of idle just to service it; instead,
- * the timer will be serviced when the CPU eventually wakes up with a
- * subsequent non-deferrable timer.
+/**
+ * @TIMER_DEFERRABLE: A deferrable timer will work normally when the
+ * system is busy, but will not cause a CPU to come out of idle just
+ * to service it; instead, the timer will be serviced when the CPU
+ * eventually wakes up with a subsequent non-deferrable timer.
  *
- * An irqsafe timer is executed with IRQ disabled and it's safe to wait for
- * the completion of the running instance from IRQ handlers, for example,
- * by calling del_timer_sync().
+ * @TIMER_IRQSAFE: An irqsafe timer is executed with IRQ disabled and
+ * it's safe to wait for the completion of the running instance from
+ * IRQ handlers, for example, by calling del_timer_sync().
  *
  * Note: The irq disabled callback execution is a special case for
  * workqueue locking issues. It's not meant for executing random crap
  * with interrupts disabled. Abuse is monitored!
+ *
+ * @TIMER_PINNED: A pinned timer will not be affected by timer
+ * migration so it will always be run on a static cpu that was
+ * specified.  Note: neither timer_setup() nor mod_timer() will
+ * guarantee correct pinning of timers.  One should always use
+ * add_timer_on() when arm the timer to guarantee that the timer will
+ * be pinned to the target CPU correctly.
  */
 #define TIMER_CPUMASK		0x0003FFFF
 #define TIMER_MIGRATING		0x00040000
-- 
2.17.1

Re: [PATCH] timer: document TIMER_PINNED

[Thomas Gleixner]

Peter,

On Tue, 18 Jun 2019, Peter Xu wrote:
> -/*
> - * A deferrable timer will work normally when the system is busy, but
> - * will not cause a CPU to come out of idle just to service it; instead,
> - * the timer will be serviced when the CPU eventually wakes up with a
> - * subsequent non-deferrable timer.
> +/**
> + * @TIMER_DEFERRABLE: A deferrable timer will work normally when the
> + * system is busy, but will not cause a CPU to come out of idle just
> + * to service it; instead, the timer will be serviced when the CPU
> + * eventually wakes up with a subsequent non-deferrable timer.
>   *
> - * An irqsafe timer is executed with IRQ disabled and it's safe to wait for
> - * the completion of the running instance from IRQ handlers, for example,
> - * by calling del_timer_sync().
> + * @TIMER_IRQSAFE: An irqsafe timer is executed with IRQ disabled and
> + * it's safe to wait for the completion of the running instance from
> + * IRQ handlers, for example, by calling del_timer_sync().
>   *
>   * Note: The irq disabled callback execution is a special case for
>   * workqueue locking issues. It's not meant for executing random crap
>   * with interrupts disabled. Abuse is monitored!
> + *
> + * @TIMER_PINNED: A pinned timer will not be affected by timer
> + * migration so it will always be run on a static cpu that was
> + * specified.

That's a bit misleading.

The timer pinned flag prevents the MOHZ timer placement heuristics to take
effect. That means that the timer is enqueued on the CPU on which the
enqueue function is invoked. 

> + * Note: neither timer_setup() nor mod_timer() will
> + * guarantee correct pinning of timers.  One should always use
> + * add_timer_on() when arm the timer to guarantee that the timer will
> + * be pinned to the target CPU correctly.

I'd rather say:

    That means that pinned timers are not guaranteed to stay on the
    initialy selected CPU. They move to the CPU on which the enqueue
    function is invoked via mod_timer() or add_timer().

    If the timer should be placed on a particular CPU then add_timer_on()
    has to be used.

Or something like that. Too tired to think about it right now, but you get
the idea.

Thanks,

	tglx

Re: [PATCH] timer: document TIMER_PINNED

[Peter Xu]

On Thu, Jun 27, 2019 at 12:43:03AM +0200, Thomas Gleixner wrote:
> Peter,
> 
> On Tue, 18 Jun 2019, Peter Xu wrote:
> > -/*
> > - * A deferrable timer will work normally when the system is busy, but
> > - * will not cause a CPU to come out of idle just to service it; instead,
> > - * the timer will be serviced when the CPU eventually wakes up with a
> > - * subsequent non-deferrable timer.
> > +/**
> > + * @TIMER_DEFERRABLE: A deferrable timer will work normally when the
> > + * system is busy, but will not cause a CPU to come out of idle just
> > + * to service it; instead, the timer will be serviced when the CPU
> > + * eventually wakes up with a subsequent non-deferrable timer.
> >   *
> > - * An irqsafe timer is executed with IRQ disabled and it's safe to wait for
> > - * the completion of the running instance from IRQ handlers, for example,
> > - * by calling del_timer_sync().
> > + * @TIMER_IRQSAFE: An irqsafe timer is executed with IRQ disabled and
> > + * it's safe to wait for the completion of the running instance from
> > + * IRQ handlers, for example, by calling del_timer_sync().
> >   *
> >   * Note: The irq disabled callback execution is a special case for
> >   * workqueue locking issues. It's not meant for executing random crap
> >   * with interrupts disabled. Abuse is monitored!
> > + *
> > + * @TIMER_PINNED: A pinned timer will not be affected by timer
> > + * migration so it will always be run on a static cpu that was
> > + * specified.
> 
> That's a bit misleading.
> 
> The timer pinned flag prevents the MOHZ timer placement heuristics to take
> effect. That means that the timer is enqueued on the CPU on which the
> enqueue function is invoked. 

Right.  I thought these details could be hidden from the API layer so
timer users won't need to know these details (neither about NOHZ, nor
about the fact that the action of queuing the timer is the thing that
really matters, because add_timer_on() can hide all these), but it's
also ok to me if you prefer them to be exposed to the users.

> 
> > + * Note: neither timer_setup() nor mod_timer() will
> > + * guarantee correct pinning of timers.  One should always use
> > + * add_timer_on() when arm the timer to guarantee that the timer will
> > + * be pinned to the target CPU correctly.
> 
> I'd rather say:
> 
>     That means that pinned timers are not guaranteed to stay on the
>     initialy selected CPU. They move to the CPU on which the enqueue
>     function is invoked via mod_timer() or add_timer().
> 
>     If the timer should be placed on a particular CPU then add_timer_on()
>     has to be used.
> 
> Or something like that. Too tired to think about it right now, but you get
> the idea.

Yes, let me give it another shot soon...  Thanks,

-- 
Peter Xu