* 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: 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, akpm, mingo, linux-kernel, 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, akpm, mingo, linux-kernel, 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
* 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, tglx, akpm, mingo, linux-kernel
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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.