* timer: permit statically-declared work with deferrable timers @ 2010-09-30 19:26 Phil Carmody 2010-09-30 19:26 ` [PATCH 1/1] " Phil Carmody 2010-09-30 20:08 ` Thomas Gleixner 0 siblings, 2 replies; 6+ messages in thread From: Phil Carmody @ 2010-09-30 19:26 UTC (permalink / raw) To: arjan, tglx; +Cc: akpm, mingo, linux-kernel, Artem.Bityutskiy Arjen, Thomas, This patch is an enabler which hopefully can lead to simplification of code elsewhere. For example, it would turn Artem's patch I refer to in the commit message (8eab945c5616fc984e97b922d6a2559be93f39a1) into just: -static DECLARE_DELAYED_WORK(cache_cleaner, do_cache_clean); +static DECLARE_DEFERRED_WORK(cache_cleaner, do_cache_clean); rather than the creation of a __init helper that required touching 3 files. I'm prepared to follow up with such simplifying patches if it's considered worthwhile. Cheers, Phil ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] timer: permit statically-declared work with deferrable timers 2010-09-30 19:26 timer: permit statically-declared work with deferrable timers Phil Carmody @ 2010-09-30 19:26 ` Phil Carmody 2010-10-01 6:19 ` Artem Bityutskiy 2010-09-30 20:08 ` Thomas Gleixner 1 sibling, 1 reply; 6+ messages in thread From: Phil Carmody @ 2010-09-30 19:26 UTC (permalink / raw) To: arjan, tglx; +Cc: akpm, mingo, linux-kernel, Artem.Bityutskiy Currently, you have to just define a delayed_work uninitialised, and then initialise it before first use. That's a tad clumsy. At risk of playing mind-games with the compiler, fooling it into doing pointer arithmetic with compile-time-constants, this lets clients properly initialise delayed work with deferrable timers statically. This patch was inspired by the issues which lead Artem Bityutskiy to commit 8eab945c5616fc984e97b922d6a2559be93f39a1. Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com> --- include/linux/timer.h | 25 +++++++++++++++++++++++++ include/linux/workqueue.h | 8 ++++++++ kernel/timer.c | 15 +-------------- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/include/linux/timer.h b/include/linux/timer.h index 38cf093..2fb272c 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -48,6 +48,18 @@ extern struct tvec_base boot_tvec_bases; #define __TIMER_LOCKDEP_MAP_INITIALIZER(_kn) #endif +/* + * Note that all tvec_bases are 2 byte aligned and lower bit of + * base in timer_list is guaranteed to be zero. Use the LSB to + * indicate whether the timer is 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. + */ +#define TBASE_DEFERRABLE_FLAG (0x1) + #define TIMER_INITIALIZER(_function, _expires, _data) { \ .entry = { .prev = TIMER_ENTRY_STATIC }, \ .function = (_function), \ @@ -58,6 +70,19 @@ extern struct tvec_base boot_tvec_bases; __FILE__ ":" __stringify(__LINE__)) \ } +#define TBASE_MAKE_DEFERRED(ptr) ((struct tvec_base *) \ + ((unsigned char *)(ptr) + TBASE_DEFERRABLE_FLAG)) + +#define TIMER_DEFERRED_INITIALIZER(_function, _expires, _data) {\ + .entry = { .prev = TIMER_ENTRY_STATIC }, \ + .function = (_function), \ + .expires = (_expires), \ + .data = (_data), \ + .base = TBASE_MAKE_DEFERRED(&boot_tvec_bases), \ + __TIMER_LOCKDEP_MAP_INITIALIZER( \ + __FILE__ ":" __stringify(__LINE__)) \ + } + #define DEFINE_TIMER(_name, _function, _expires, _data) \ struct timer_list _name = \ TIMER_INITIALIZER(_function, _expires, _data) diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h index 25e02c9..076d5a7 100644 --- a/include/linux/workqueue.h +++ b/include/linux/workqueue.h @@ -127,12 +127,20 @@ struct execute_work { .timer = TIMER_INITIALIZER(NULL, 0, 0), \ } +#define __DEFERRED_WORK_INITIALIZER(n, f) { \ + .work = __WORK_INITIALIZER((n).work, (f)), \ + .timer = TIMER_DEFERRED_INITIALIZER(NULL, 0, 0), \ + } + #define DECLARE_WORK(n, f) \ struct work_struct n = __WORK_INITIALIZER(n, f) #define DECLARE_DELAYED_WORK(n, f) \ struct delayed_work n = __DELAYED_WORK_INITIALIZER(n, f) +#define DECLARE_DEFERRED_WORK(n, f) \ + struct delayed_work n = __DEFERRED_WORK_INITIALIZER(n, f) + /* * initialize a work item's function pointer */ diff --git a/kernel/timer.c b/kernel/timer.c index 97bf05b..72853b2 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -88,18 +88,6 @@ struct tvec_base boot_tvec_bases; EXPORT_SYMBOL(boot_tvec_bases); static DEFINE_PER_CPU(struct tvec_base *, tvec_bases) = &boot_tvec_bases; -/* - * Note that all tvec_bases are 2 byte aligned and lower bit of - * base in timer_list is guaranteed to be zero. Use the LSB to - * indicate whether the timer is 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. - */ -#define TBASE_DEFERRABLE_FLAG (0x1) - /* Functions below help us manage 'deferrable' flag */ static inline unsigned int tbase_get_deferrable(struct tvec_base *base) { @@ -113,8 +101,7 @@ static inline struct tvec_base *tbase_get_base(struct tvec_base *base) static inline void timer_set_deferrable(struct timer_list *timer) { - timer->base = ((struct tvec_base *)((unsigned long)(timer->base) | - TBASE_DEFERRABLE_FLAG)); + timer->base = TBASE_MAKE_DEFERRED(timer->base); } static inline void -- 1.7.2.rc1.37.gf8c40 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] timer: permit statically-declared work with deferrable timers 2010-09-30 19:26 ` [PATCH 1/1] " Phil Carmody @ 2010-10-01 6:19 ` Artem Bityutskiy 0 siblings, 0 replies; 6+ messages in thread From: Artem Bityutskiy @ 2010-10-01 6:19 UTC (permalink / raw) To: Phil Carmody Cc: arjan@linux.intel.com, tglx@linutronix.de, akpm@linux-foundation.org, mingo@elte.hu, linux-kernel@vger.kernel.org On Thu, 2010-09-30 at 21:26 +0200, ext Phil Carmody wrote: > Currently, you have to just define a delayed_work uninitialised, and then > initialise it before first use. That's a tad clumsy. At risk of playing > mind-games with the compiler, fooling it into doing pointer arithmetic > with compile-time-constants, this lets clients properly initialise delayed > work with deferrable timers statically. > > This patch was inspired by the issues which lead Artem Bityutskiy > to commit 8eab945c5616fc984e97b922d6a2559be93f39a1. > > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com> Acked-by: Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com> -- Best Regards, Artem Bityutskiy (Артём Битюцкий) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: timer: permit statically-declared work with deferrable timers 2010-09-30 19:26 timer: permit statically-declared work with deferrable timers Phil Carmody 2010-09-30 19:26 ` [PATCH 1/1] " Phil Carmody @ 2010-09-30 20:08 ` Thomas Gleixner 2010-09-30 20:50 ` Phil Carmody 1 sibling, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2010-09-30 20:08 UTC (permalink / raw) To: Phil Carmody; +Cc: arjan, akpm, mingo, linux-kernel, Artem.Bityutskiy On Thu, 30 Sep 2010, Phil Carmody wrote: > > Arjen, Thomas, > > This patch is an enabler which hopefully can lead to simplification of code > elsewhere. For example, it would turn Artem's patch I refer to in the commit > message (8eab945c5616fc984e97b922d6a2559be93f39a1) into just: > > -static DECLARE_DELAYED_WORK(cache_cleaner, do_cache_clean); > +static DECLARE_DEFERRED_WORK(cache_cleaner, do_cache_clean); > > rather than the creation of a __init helper that required touching 3 files. > > I'm prepared to follow up with such simplifying patches if it's considered > worthwhile. If it simplies stuff, no objections from my side. The patch looks reasonable enough. How many (ab)users will it clean up ? Thanks, tglx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: timer: permit statically-declared work with deferrable timers 2010-09-30 20:08 ` Thomas Gleixner @ 2010-09-30 20:50 ` Phil Carmody 2010-09-30 20:55 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Phil Carmody @ 2010-09-30 20:50 UTC (permalink / raw) To: ext Thomas Gleixner Cc: arjan@linux.intel.com, akpm@linux-foundation.org, mingo@elte.hu, linux-kernel@vger.kernel.org, Bityutskiy Artem (Nokia-MS/Helsinki) On 30/09/10 22:08 +0200, ext Thomas Gleixner wrote: > On Thu, 30 Sep 2010, Phil Carmody wrote: > > > > > Arjen, Thomas, > > > > This patch is an enabler which hopefully can lead to simplification of code > > elsewhere. For example, it would turn Artem's patch I refer to in the commit > > message (8eab945c5616fc984e97b922d6a2559be93f39a1) into just: > > > > -static DECLARE_DELAYED_WORK(cache_cleaner, do_cache_clean); > > +static DECLARE_DEFERRED_WORK(cache_cleaner, do_cache_clean); > > > > rather than the creation of a __init helper that required touching 3 files. > > > > I'm prepared to follow up with such simplifying patches if it's considered > > worthwhile. > > If it simplies stuff, no objections from my side. The patch looks > reasonable enough. > > How many (ab)users will it clean up ? Not a large number; absolute tops - a dozen. I've not investigated any apart from the couple that Artem addressed initially. And it's OK, the clients weren't the _ab_users. If anything we're the abusers for using sleight-of-hand to hide flags inside pointer values. (Which the C standard and GCC tried hard to stop us doing. Not hard enough, though ;-) .) My main intention is to make future patches like Artem's trivial. Change the type - change one line. Life's too short to headscratch and fight compilers. Cheers, Phil ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: timer: permit statically-declared work with deferrable timers 2010-09-30 20:50 ` Phil Carmody @ 2010-09-30 20:55 ` Thomas Gleixner 0 siblings, 0 replies; 6+ messages in thread From: Thomas Gleixner @ 2010-09-30 20:55 UTC (permalink / raw) To: Phil Carmody Cc: arjan@linux.intel.com, akpm@linux-foundation.org, mingo@elte.hu, linux-kernel@vger.kernel.org, Bityutskiy Artem (Nokia-MS/Helsinki) On Thu, 30 Sep 2010, Phil Carmody wrote: > On 30/09/10 22:08 +0200, ext Thomas Gleixner wrote: > > On Thu, 30 Sep 2010, Phil Carmody wrote: > > > > > > > > Arjen, Thomas, > > > > > > This patch is an enabler which hopefully can lead to simplification of code > > > elsewhere. For example, it would turn Artem's patch I refer to in the commit > > > message (8eab945c5616fc984e97b922d6a2559be93f39a1) into just: > > > > > > -static DECLARE_DELAYED_WORK(cache_cleaner, do_cache_clean); > > > +static DECLARE_DEFERRED_WORK(cache_cleaner, do_cache_clean); > > > > > > rather than the creation of a __init helper that required touching 3 files. > > > > > > I'm prepared to follow up with such simplifying patches if it's considered > > > worthwhile. > > > > If it simplies stuff, no objections from my side. The patch looks > > reasonable enough. > > > > How many (ab)users will it clean up ? > > Not a large number; absolute tops - a dozen. I've not investigated any apart from > the couple that Artem addressed initially. > > And it's OK, the clients weren't the _ab_users. If anything we're the abusers for > using sleight-of-hand to hide flags inside pointer values. (Which the C standard > and GCC tried hard to stop us doing. Not hard enough, though ;-) .) > > My main intention is to make future patches like Artem's trivial. Change the > type - change one line. Life's too short to headscratch and fight compilers. Fair enough. tglx ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-01 6:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-30 19:26 timer: permit statically-declared work with deferrable timers Phil Carmody 2010-09-30 19:26 ` [PATCH 1/1] " Phil Carmody 2010-10-01 6:19 ` Artem Bityutskiy 2010-09-30 20:08 ` Thomas Gleixner 2010-09-30 20:50 ` Phil Carmody 2010-09-30 20:55 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox