| Message ID | 20170516114812.10660-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 05/16/17 04:48, Christoph Hellwig wrote: > diff --git a/include/linux/timer.h b/include/linux/timer.h > index e6789b8757d5..87afe52c8349 100644 > --- a/include/linux/timer.h > +++ b/include/linux/timer.h \ > @@ -126,6 +146,32 @@ static inline void init_timer_on_stack_key(struct timer_list *timer, > init_timer_on_stack_key((_timer), (_flags), NULL, NULL) > #endif > > +/** > + * prepare_timer - initialize a timer before first use > + * @timer: timer structure to prepare > + * @func: callback to be called when the timer expires > + * @flags %TIMER_* flags that control timer behavior missing ':' on @flags: > + * > + * This function initializes a timer_list structure so that it can > + * be used (by calling add_timer() or mod_timer()). > + */ > +static inline void prepare_timer(struct timer_list *timer, > + void (*func)(struct timer_list *timer), u32 flags) > +{
On Tue, May 16, 2017 at 1:48 PM, Christoph Hellwig <hch@lst.de> wrote: > unsigned long expires; > - void (*function)(unsigned long); > + union { > + void (*func)(struct timer_list *timer); > + void (*function)(unsigned long); > + }; ... > +#define INIT_TIMER(_func, _expires, _flags) \ > +{ \ > + .entry = { .next = TIMER_ENTRY_STATIC }, \ > + .func = (_func), \ > + .expires = (_expires), \ > + .flags = TIMER_MODERN | (_flags), \ > + __TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \ > +} If I remember correctly, this will fail with gcc-4.5 and earlier, which can't use named initializers for anonymous unions. One of these two should work, but they are both ugly: a) don't use a named initializer for the union (a bit fragile) +#define INIT_TIMER(_func, _expires, _flags) \ +{ \ + .entry = { .next = TIMER_ENTRY_STATIC }, \ + .expires = (_expires), \ + { .func = (_func) }, \ + .flags = TIMER_MODERN | (_flags), \ + __TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \ +} b) give the union a name (breaks any reference to timer_list->func in C code): + union { + void (*func)(struct timer_list *timer); + void (*function)(unsigned long); + } u; ... +#define INIT_TIMER(_func, _expires, _flags) \ +{ \ + .entry = { .next = TIMER_ENTRY_STATIC }, \ + .u.func = (_func), \ + .expires = (_expires), \ + .flags = TIMER_MODERN | (_flags), \ + __TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \ +} > +/** > + * prepare_timer - initialize a timer before first use > + * @timer: timer structure to prepare > + * @func: callback to be called when the timer expires > + * @flags %TIMER_* flags that control timer behavior > + * > + * This function initializes a timer_list structure so that it can > + * be used (by calling add_timer() or mod_timer()). > + */ > +static inline void prepare_timer(struct timer_list *timer, > + void (*func)(struct timer_list *timer), u32 flags) > +{ > + __init_timer(timer, TIMER_MODERN | flags); > + timer->func = func; > +} > + > +static inline void prepare_timer_on_stack(struct timer_list *timer, > + void (*func)(struct timer_list *timer), u32 flags) > +{ > + __init_timer_on_stack(timer, TIMER_MODERN | flags); > + timer->func = func; > +} I fear this breaks lockdep output, which turns the name of the timer into a string that gets printed later. It should work when these are macros, or a macro wrapping an inline function like __init_timer is. Arnd
> b) give the union a name (breaks any reference to timer_list->func in C code): > > + union { > + void (*func)(struct timer_list *timer); > + void (*function)(unsigned long); > + } u; I'll look into that, as it seems a lot safer, and places outside the timer code shouldn't really touch it (although I bet they do, so more fixes for this series..) > I fear this breaks lockdep output, which turns the name of > the timer into a string that gets printed later. It should work > when these are macros, or a macro wrapping an inline function > like __init_timer is. Ok, I'll fix it up. Although this macro mess isn't really readable at all.
On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote: > > b) give the union a name (breaks any reference to timer_list->func in C code): > > > > + union { > > + void (*func)(struct timer_list *timer); > > + void (*function)(unsigned long); > > + } u; > > I'll look into that, as it seems a lot safer, and places outside > the timer code shouldn't really touch it (although I bet they do, > so more fixes for this series..) Meh. All the old init_timer users set function directly, so I guess we need to use the other approach.
On Thu, May 18, 2017 at 10:41 AM, Christoph Hellwig <hch@lst.de> wrote: > On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote: >> > b) give the union a name (breaks any reference to timer_list->func in C code): >> > >> > + union { >> > + void (*func)(struct timer_list *timer); >> > + void (*function)(unsigned long); >> > + } u; >> >> I'll look into that, as it seems a lot safer, and places outside >> the timer code shouldn't really touch it (although I bet they do, >> so more fixes for this series..) > > Meh. All the old init_timer users set function directly, so > I guess we need to use the other approach. How expensive would it be to add another field to timer_list and just have both pointers? Arnd
From: Christoph Hellwig > Sent: 16 May 2017 12:48 > > The new callback gets a pointer to the timer_list itself, which can > then be used to get the containing structure using container_of > instead of casting from and to unsigned long all the time. What about sensible drivers that put some other value in the 'data' field? Perhaps it ought to have been 'void *data'. Seems retrograde to be passing the address of the timer structure (which, in principle, the callers no nothing about). So I wouldn't call it 'modern', just different. David
On Fri, May 19, 2017 at 10:48:51AM +0000, David Laight wrote: > From: Christoph Hellwig > > Sent: 16 May 2017 12:48 > > > > The new callback gets a pointer to the timer_list itself, which can > > then be used to get the containing structure using container_of > > instead of casting from and to unsigned long all the time. > > What about sensible drivers that put some other value in the 'data' > field? They will add the equivalent of the data field into the containing structure of the timer. Just like we do for all other kernel interfaces using the container_of patter, which includes just about every primitive designed in the last 15 years.
On Thu, May 18, 2017 at 10:57:31AM +0200, Arnd Bergmann wrote: > How expensive would it be to add another field to timer_list and > just have both pointers? That would add 4/8 bytes to every structure containing a timer, so I'd rather avoid it if possible. But one option might be to inflict this onto users of outdated compilers and use the union for modern ones.
On Sun, May 21, 2017 at 9:00 AM, Christoph Hellwig <hch@lst.de> wrote: > On Thu, May 18, 2017 at 10:57:31AM +0200, Arnd Bergmann wrote: >> How expensive would it be to add another field to timer_list and >> just have both pointers? > > That would add 4/8 bytes to every structure containing a timer, > so I'd rather avoid it if possible. I didn't expect too many timers to be in allocated structures at the same time on most systems, but I haven't researched this at all. We should probably update the comment about the cacheline alignment though: when most users embed the timer_list in some other structure, it's not valid at all, and forcing timer_list to be cacheline aligned would waste way more space than an extra field. > But one option might be to inflict this onto users of outdated compilers > and use the union for modern ones. Good idea, this sounds better than the alternatives at least. The remaining users of those old compilers certainly don't care that much about micro-optimizing the kernel anyway. Arnd
On Thu, 18 May 2017, Christoph Hellwig wrote: > On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote: > > > b) give the union a name (breaks any reference to timer_list->func in C code): > > > > > > + union { > > > + void (*func)(struct timer_list *timer); > > > + void (*function)(unsigned long); > > > + } u; > > > > I'll look into that, as it seems a lot safer, and places outside > > the timer code shouldn't really touch it (although I bet they do, > > so more fixes for this series..) > > Meh. All the old init_timer users set function directly, so > I guess we need to use the other approach. There is another possibility. Create a coccinelle script which wraps all timer.function = f; timer->function = f; assignements into a helper timer_set_function(timer, func) and ask Linus to run it right before the next -rc. That handles everything in tree and the few new instances in next can be addressed with patches sent to the maintainers. Thanks, tglx
On Sun, May 21, 2017 at 07:57:53PM +0200, Thomas Gleixner wrote: > On Thu, 18 May 2017, Christoph Hellwig wrote: > > On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote: > > > > b) give the union a name (breaks any reference to timer_list->func in C code): > > > > > > > > + union { > > > > + void (*func)(struct timer_list *timer); > > > > + void (*function)(unsigned long); > > > > + } u; > > > > > > I'll look into that, as it seems a lot safer, and places outside > > > the timer code shouldn't really touch it (although I bet they do, > > > so more fixes for this series..) > > > > Meh. All the old init_timer users set function directly, so > > I guess we need to use the other approach. > > There is another possibility. Create a coccinelle script which wraps all > > timer.function = f; > timer->function = f; > > assignements into a helper timer_set_function(timer, func) and ask Linus to > run it right before the next -rc. That handles everything in tree and the > few new instances in next can be addressed with patches sent to the > maintainers. FWIW, there was another possible approach - I toyed with that several years ago, but it didn't go anywhere. Namely, make timer.function take void * *and* turn the setup part into setup(timer, callback, argument), verifying that * callback(argument) will be acceptable expression for C typechecking * callback returns void * argument is a pointer type then cast callback to void (*)(void *) and argument to void *. That way we get rid of any boilerplate in callbacks and get sane typechecking...
diff --git a/include/linux/timer.h b/include/linux/timer.h index e6789b8757d5..87afe52c8349 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -16,7 +16,10 @@ struct timer_list { */ struct hlist_node entry; unsigned long expires; - void (*function)(unsigned long); + union { + void (*func)(struct timer_list *timer); + void (*function)(unsigned long); + }; unsigned long data; u32 flags; @@ -52,7 +55,8 @@ struct timer_list { * workqueue locking issues. It's not meant for executing random crap * with interrupts disabled. Abuse is monitored! */ -#define TIMER_CPUMASK 0x0003FFFF +#define TIMER_CPUMASK 0x0001FFFF +#define TIMER_MODERN 0x00020000 #define TIMER_MIGRATING 0x00040000 #define TIMER_BASEMASK (TIMER_CPUMASK | TIMER_MIGRATING) #define TIMER_DEFERRABLE 0x00080000 @@ -63,6 +67,22 @@ struct timer_list { #define TIMER_TRACE_FLAGMASK (TIMER_MIGRATING | TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE) +#define INIT_TIMER(_func, _expires, _flags) \ +{ \ + .entry = { .next = TIMER_ENTRY_STATIC }, \ + .func = (_func), \ + .expires = (_expires), \ + .flags = TIMER_MODERN | (_flags), \ + __TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \ +} + +#define DECLARE_TIMER(_name, _func, _expires, _flags) \ + struct timer_list _name = INIT_TIMER(_func, _expires, _flags) + +/* + * Don't use the macros below, use DECLARE_TIMER and INIT_TIMER with their + * improved callback signature above. + */ #define __TIMER_INITIALIZER(_function, _expires, _data, _flags) { \ .entry = { .next = TIMER_ENTRY_STATIC }, \ .function = (_function), \ @@ -126,6 +146,32 @@ static inline void init_timer_on_stack_key(struct timer_list *timer, init_timer_on_stack_key((_timer), (_flags), NULL, NULL) #endif +/** + * prepare_timer - initialize a timer before first use + * @timer: timer structure to prepare + * @func: callback to be called when the timer expires + * @flags %TIMER_* flags that control timer behavior + * + * This function initializes a timer_list structure so that it can + * be used (by calling add_timer() or mod_timer()). + */ +static inline void prepare_timer(struct timer_list *timer, + void (*func)(struct timer_list *timer), u32 flags) +{ + __init_timer(timer, TIMER_MODERN | flags); + timer->func = func; +} + +static inline void prepare_timer_on_stack(struct timer_list *timer, + void (*func)(struct timer_list *timer), u32 flags) +{ + __init_timer_on_stack(timer, TIMER_MODERN | flags); + timer->func = func; +} + +/* + * Don't use - use prepare_timer above for new code instead. + */ #define init_timer(timer) \ __init_timer((timer), 0) #define init_timer_pinned(timer) \ diff --git a/kernel/time/timer.c b/kernel/time/timer.c index c7978fcdbbea..48d8450cfa5f 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -579,7 +579,7 @@ static struct debug_obj_descr timer_debug_descr; static void *timer_debug_hint(void *addr) { - return ((struct timer_list *) addr)->function; + return ((struct timer_list *) addr)->func; } static bool timer_is_static_object(void *addr) @@ -930,7 +930,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) unsigned long clk = 0, flags; int ret = 0; - BUG_ON(!timer->function); + BUG_ON(!timer->func && !timer->function); /* * This is a common optimization triggered by the networking code - if @@ -1064,12 +1064,12 @@ EXPORT_SYMBOL(mod_timer); * add_timer - start a timer * @timer: the timer to be added * - * The kernel will do a ->function(->data) callback from the - * timer interrupt at the ->expires point in the future. The - * current time is 'jiffies'. + * The kernel will do a ->func (or ->function(->data) for legacy timers) + * callback from the timer interrupt at the ->expires point in the future. + * The current time is 'jiffies'. * - * The timer's ->expires, ->function (and if the handler uses it, ->data) - * fields must be set prior calling this function. + * The timer's ->expires, ->func / ->function (and if the handler uses it, + * ->data) fields must be set prior calling this function. * * Timers with an ->expires field in the past will be executed in the next * timer tick. @@ -1093,7 +1093,8 @@ void add_timer_on(struct timer_list *timer, int cpu) struct timer_base *new_base, *base; unsigned long flags; - BUG_ON(timer_pending(timer) || !timer->function); + BUG_ON(timer_pending(timer)); + BUG_ON(!timer->func && !timer->function); new_base = get_timer_cpu_base(timer->flags, cpu); @@ -1264,14 +1265,17 @@ static void call_timer_fn(struct timer_list *timer) lock_map_acquire(&lockdep_map); trace_timer_expire_entry(timer); - timer->function(timer->data); + if (timer->flags & TIMER_MODERN) + timer->func(timer); + else + timer->function(timer->data); trace_timer_expire_exit(timer); lock_map_release(&lockdep_map); if (count != preempt_count()) { WARN_ONCE(1, "timer: %pF preempt leak: %08x -> %08x\n", - timer->function, count, preempt_count()); + timer->func, count, preempt_count()); /* * Restore the preempt count. That gives us a decent * chance to survive and extract information. If the
The new callback gets a pointer to the timer_list itself, which can then be used to get the containing structure using container_of instead of casting from and to unsigned long all the time. The setup helpers take a flags argument instead of needing countless variants. Note: this further reduces space for the cpumask. By the time we'll need the additional cpumask space getting rid of the old-style timers will hopefully be finished. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/timer.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- kernel/time/timer.c | 24 ++++++++++++++---------- 2 files changed, 62 insertions(+), 12 deletions(-)