Hello.
This is a combo patch set for support defferability in timerfd.
Due implementation of timerfd is based on hrtimers and only on hrtimers,
it was necessary to add such deferrability into hrtimers too.
It brings benefits for user-land application which don't want to break idle state and for
in-kernel users who want to use hrtimer advantages.
Alexey Perevalov (2):
tracing/trivial: Add CLOCK_BOOTIME and CLOCK_TAI for human readable
clockid trace
tracing/trivial: Add CLOCK_*_DEFERRABLE for tracing clockids
Anton Vorontsov (3):
kernel/time: Add new helpers to convert ktime to/from jiffies
timerfd: Factor out timer-type unspecific timerfd_expire()
timerfd: Add support for deferrable timers
Thomas Gleixner (1):
hrtimer: Add support for deferrable timer into the hrtimer
fs/timerfd.c | 47 ++++++++++++++++----------------
include/linux/hrtimer.h | 3 ++
include/linux/jiffies.h | 4 ++-
include/linux/ktime.h | 3 +-
include/trace/events/timer.h | 11 +++++++-
include/uapi/linux/time.h | 3 ++
kernel/hrtimer.c | 62 +++++++++++++++++++++++++++++++++---------
kernel/time.c | 23 ++++++++++++++++
8 files changed, 117 insertions(+), 39 deletions(-)
--
1.7.9.5
These clockids is also used in current hrtimer implementation.
Signed-off-by: Alexey Perevalov <[email protected]>
---
include/trace/events/timer.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index 185b2c6..547b79f 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -11,7 +11,10 @@
#define clockid_to_string(clockid) \
__print_symbolic(clockid, \
{ CLOCK_MONOTONIC, "CLOCK_MONOTONIC" }, \
- { CLOCK_REALTIME, "CLOCK_REALTIME" })
+ { CLOCK_REALTIME, "CLOCK_REALTIME" }, \
+ { CLOCK_BOOTTIME, "CLOCK_BOOTTIME" }, \
+ { CLOCK_TAI, "CLOCK_TAI" } \
+ )
#define hrmode_to_string(hrmode) \
__print_symbolic(hrmode, \
--
1.7.9.5
From: Thomas Gleixner <[email protected]>
This patch introduces new public CLOCKID constants for
user space API, such as timerfd. It extends hrtimer API and makes
possible to have unified interfaces where deferreble functionality is
used. In-kernel users such as device drivers could find benefits too.
High resolution timer now could work with CLOCK_REALTIME_DEFERRABLE,
CLOCK_MONOTONIC_DEFERRABLE, CLOCK_BOOTTIME_DEFERRABLE.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Alexey Perevalov <[email protected]>
---
include/linux/hrtimer.h | 3 +++
include/uapi/linux/time.h | 3 +++
kernel/hrtimer.c | 62 +++++++++++++++++++++++++++++++++++----------
3 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index d19a5c2..fe1159c 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -158,6 +158,9 @@ enum hrtimer_base_type {
HRTIMER_BASE_REALTIME,
HRTIMER_BASE_BOOTTIME,
HRTIMER_BASE_TAI,
+ HRTIMER_BASE_MONOTONIC_DEFERRABLE,
+ HRTIMER_BASE_REALTIME_DEFERRABLE,
+ HRTIMER_BASE_BOOTTIME_DEFERRABLE,
HRTIMER_MAX_CLOCK_BASES,
};
diff --git a/include/uapi/linux/time.h b/include/uapi/linux/time.h
index e75e1b6..bb8dc60 100644
--- a/include/uapi/linux/time.h
+++ b/include/uapi/linux/time.h
@@ -56,6 +56,9 @@ struct itimerval {
#define CLOCK_BOOTTIME_ALARM 9
#define CLOCK_SGI_CYCLE 10 /* Hardware specific */
#define CLOCK_TAI 11
+#define CLOCK_REALTIME_DEFERRABLE 12
+#define CLOCK_MONOTONIC_DEFERRABLE 13
+#define CLOCK_BOOTTIME_DEFERRABLE 14
#define MAX_CLOCKS 16
#define CLOCKS_MASK (CLOCK_REALTIME | CLOCK_MONOTONIC)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 0909436..d1478fc 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -92,14 +92,35 @@ DEFINE_PER_CPU(struct hrtimer_cpu_base, hrtimer_bases) =
.get_time = &ktime_get_clocktai,
.resolution = KTIME_LOW_RES,
},
+ {
+ .index = HRTIMER_BASE_MONOTONIC_DEFERRABLE,
+ .clockid = CLOCK_MONOTONIC_DEFERRABLE,
+ .get_time = &ktime_get,
+ .resolution = KTIME_LOW_RES,
+ },
+ {
+ .index = HRTIMER_BASE_REALTIME_DEFERRABLE,
+ .clockid = CLOCK_REALTIME_DEFERRABLE,
+ .get_time = &ktime_get_real,
+ .resolution = KTIME_LOW_RES,
+ },
+ {
+ .index = HRTIMER_BASE_BOOTTIME_DEFERRABLE,
+ .clockid = CLOCK_BOOTTIME_DEFERRABLE,
+ .get_time = &ktime_get_boottime,
+ .resolution = KTIME_LOW_RES,
+ },
}
};
static const int hrtimer_clock_to_base_table[MAX_CLOCKS] = {
- [CLOCK_REALTIME] = HRTIMER_BASE_REALTIME,
- [CLOCK_MONOTONIC] = HRTIMER_BASE_MONOTONIC,
- [CLOCK_BOOTTIME] = HRTIMER_BASE_BOOTTIME,
- [CLOCK_TAI] = HRTIMER_BASE_TAI,
+ [CLOCK_REALTIME] = HRTIMER_BASE_REALTIME,
+ [CLOCK_MONOTONIC] = HRTIMER_BASE_MONOTONIC,
+ [CLOCK_BOOTTIME] = HRTIMER_BASE_BOOTTIME,
+ [CLOCK_TAI] = HRTIMER_BASE_TAI,
+ [CLOCK_REALTIME_DEFERRABLE] = HRTIMER_BASE_REALTIME_DEFERRABLE,
+ [CLOCK_MONOTONIC_DEFERRABLE] = HRTIMER_BASE_MONOTONIC_DEFERRABLE,
+ [CLOCK_BOOTTIME_DEFERRABLE] = HRTIMER_BASE_BOOTTIME_DEFERRABLE,
};
static inline int hrtimer_clockid_to_base(clockid_t clock_id)
@@ -194,7 +215,8 @@ hrtimer_check_target(struct hrtimer *timer, struct hrtimer_clock_base *new_base)
#ifdef CONFIG_HIGH_RES_TIMERS
ktime_t expires;
- if (!new_base->cpu_base->hres_active)
+ if (!new_base->cpu_base->hres_active ||
+ new_base->index >= HRTIMER_BASE_MONOTONIC_DEFERRABLE)
return 0;
expires = ktime_sub(hrtimer_get_expires(timer), new_base->offset);
@@ -556,7 +578,7 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
expires_next.tv64 = KTIME_MAX;
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
+ for (i = 0; i < HRTIMER_BASE_MONOTONIC_DEFERRABLE; i++, base++) {
struct hrtimer *timer;
struct timerqueue_node *next;
@@ -615,6 +637,13 @@ static int hrtimer_reprogram(struct hrtimer *timer,
return 0;
/*
+ * Deferrable timers are not touching the underlying
+ * hardware.
+ */
+ if (base->index >= HRTIMER_BASE_MONOTONIC_DEFERRABLE)
+ return 0;
+
+ /*
* CLOCK_REALTIME timer might be requested with an absolute
* expiry time which is less than base->offset. Nothing wrong
* about that, just avoid to call into the tick code, which
@@ -924,7 +953,10 @@ static void __remove_hrtimer(struct hrtimer *timer,
expires = ktime_sub(hrtimer_get_expires(timer),
base->offset);
- if (base->cpu_base->expires_next.tv64 == expires.tv64)
+
+ /* We only care about non deferrable timers here */
+ if (base->index <= HRTIMER_BASE_MONOTONIC_DEFERRABLE &&
+ base->cpu_base->expires_next.tv64 == expires.tv64)
hrtimer_force_reprogram(base->cpu_base, 1);
}
#endif
@@ -1152,7 +1184,7 @@ ktime_t hrtimer_get_next_event(void)
raw_spin_lock_irqsave(&cpu_base->lock, flags);
if (!hrtimer_hres_active()) {
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
+ for (i = 0; i < HRTIMER_BASE_MONOTONIC_DEFERRABLE; i++, base++) {
struct hrtimer *timer;
struct timerqueue_node *next;
@@ -1284,7 +1316,8 @@ void hrtimer_interrupt(struct clock_event_device *dev)
{
struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
ktime_t expires_next, now, entry_time, delta;
- int i, retries = 0;
+ unsigned long bases;
+ int retries = 0;
BUG_ON(!cpu_base->hres_active);
cpu_base->nr_events++;
@@ -1302,14 +1335,16 @@ retry:
* this CPU.
*/
cpu_base->expires_next.tv64 = KTIME_MAX;
+ bases = cpu_base->active_bases;
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
+ while (bases) {
struct hrtimer_clock_base *base;
struct timerqueue_node *node;
ktime_t basenow;
+ int i;
- if (!(cpu_base->active_bases & (1 << i)))
- continue;
+ i = __ffs(bases);
+ bases &= ~(1 << i);
base = cpu_base->clock_base + i;
basenow = ktime_add(now, base->offset);
@@ -1339,7 +1374,8 @@ retry:
base->offset);
if (expires.tv64 < 0)
expires.tv64 = KTIME_MAX;
- if (expires.tv64 < expires_next.tv64)
+ if (expires.tv64 < expires_next.tv64 &&
+ base->index <= HRTIMER_BASE_MONOTONIC_DEFERRABLE)
expires_next = expires;
break;
}
--
1.7.9.5
Signed-off-by: Alexey Perevalov <[email protected]>
---
include/trace/events/timer.h | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index 547b79f..ea3119a 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -13,7 +13,13 @@
{ CLOCK_MONOTONIC, "CLOCK_MONOTONIC" }, \
{ CLOCK_REALTIME, "CLOCK_REALTIME" }, \
{ CLOCK_BOOTTIME, "CLOCK_BOOTTIME" }, \
- { CLOCK_TAI, "CLOCK_TAI" } \
+ { CLOCK_TAI, "CLOCK_TAI" }, \
+ { CLOCK_MONOTONIC_DEFERRABLE, \
+ "CLOCK_MONOTONIC_DEFERRABLE" }, \
+ { CLOCK_REALTIME_DEFERRABLE, \
+ "CLOCK_REALTIME_DEFERRABLE" }, \
+ { CLOCK_BOOTTIME_DEFERRABLE, \
+ "CLOCK_BOOTTIME_DEFERRABLE" } \
)
#define hrmode_to_string(hrmode) \
--
1.7.9.5
From: Anton Vorontsov <[email protected]>
There is nothing hrtimer-specific inside the timerfd_tmrproc(), except
the function prototype. We're about to add other timer types, so factor
out generic timerfd_expire() helper from timerfd_tmrproc().
Signed-off-by: Anton Vorontsov <[email protected]>
Signed-off-by: Alexey Perevalov <[email protected]>
---
fs/timerfd.c | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/fs/timerfd.c b/fs/timerfd.c
index 9293121..3561ce7 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -229,6 +229,23 @@ static unsigned int timerfd_poll(struct file *file, poll_table *wait)
return events;
}
+static u64 timerfd_rearm(struct timerfd_ctx *ctx)
+{
+ u64 orun;
+
+ if (isalarm(ctx)) {
+ orun += alarm_forward_now(
+ &ctx->t.alarm, ctx->tintv) - 1;
+ alarm_restart(&ctx->t.alarm);
+ } else {
+ orun += hrtimer_forward_now(&ctx->t.tmr,
+ ctx->tintv) - 1;
+ hrtimer_restart(&ctx->t.tmr);
+ }
+
+ return orun;
+}
+
static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
loff_t *ppos)
{
@@ -265,15 +282,7 @@ static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
* callback to avoid DoS attacks specifying a very
* short timer period.
*/
- if (isalarm(ctx)) {
- ticks += alarm_forward_now(
- &ctx->t.alarm, ctx->tintv) - 1;
- alarm_restart(&ctx->t.alarm);
- } else {
- ticks += hrtimer_forward_now(&ctx->t.tmr,
- ctx->tintv) - 1;
- hrtimer_restart(&ctx->t.tmr);
- }
+ ticks += timerfd_rearm(ctx);
}
ctx->expired = 0;
ctx->ticks = 0;
@@ -421,18 +430,7 @@ static int do_timerfd_gettime(int ufd, struct itimerspec *t)
spin_lock_irq(&ctx->wqh.lock);
if (ctx->expired && ctx->tintv.tv64) {
ctx->expired = 0;
-
- if (isalarm(ctx)) {
- ctx->ticks +=
- alarm_forward_now(
- &ctx->t.alarm, ctx->tintv) - 1;
- alarm_restart(&ctx->t.alarm);
- } else {
- ctx->ticks +=
- hrtimer_forward_now(&ctx->t.tmr, ctx->tintv)
- - 1;
- hrtimer_restart(&ctx->t.tmr);
- }
+ ctx->ticks += timerfd_rearm(ctx);
}
t->it_value = ktime_to_timespec(timerfd_get_remaining(ctx));
t->it_interval = ktime_to_timespec(ctx->tintv);
--
1.7.9.5
From: Anton Vorontsov <[email protected]>
This patch implements a userland-side API for generic deferrable timers,
per linux/timer.h:
* 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.
These timers are crucial for power saving, i.e. periodic tasks that want
to work in background when the system is under use, but don't want to
cause wakeups themselves.
The deferred timers are somewhat orthogonal to high-res external timers,
since the deferred timer is tied to the system load, not just to some
external decrementer source.
Currently, the implementation is based on high resolution timers.
Signed-off-by: Anton Vorontsov <[email protected]>
Signed-off-by: Alexey Perevalov <[email protected]>
---
fs/timerfd.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/fs/timerfd.c b/fs/timerfd.c
index 3561ce7..c132fd2 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -231,7 +231,7 @@ static unsigned int timerfd_poll(struct file *file, poll_table *wait)
static u64 timerfd_rearm(struct timerfd_ctx *ctx)
{
- u64 orun;
+ u64 orun = 0;
if (isalarm(ctx)) {
orun += alarm_forward_now(
@@ -326,7 +326,10 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
(clockid != CLOCK_MONOTONIC &&
clockid != CLOCK_REALTIME &&
clockid != CLOCK_REALTIME_ALARM &&
- clockid != CLOCK_BOOTTIME_ALARM))
+ clockid != CLOCK_BOOTTIME_ALARM &&
+ clockid != CLOCK_REALTIME_DEFERRABLE &&
+ clockid != CLOCK_MONOTONIC_DEFERRABLE &&
+ clockid != CLOCK_BOOTTIME_DEFERRABLE))
return -EINVAL;
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
@@ -354,7 +357,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
return ufd;
}
-static int do_timerfd_settime(int ufd, int flags,
+static int do_timerfd_settime(int ufd, int flags,
const struct itimerspec *new,
struct itimerspec *old)
{
--
1.7.9.5
From: Anton Vorontsov <[email protected]>
Two new functions: jiffies_to_ktime() and ktime_to_jiffies(), we'll use
them for timerfd deferred timers handling.
We fully reuse the logic from timespec implementations, so the functions
are pretty straightforward.
The only tricky part is in headers: we have to include jiffies.h after
we defined ktime_t, this is because ktime.h needs some declarations from
jiffies.h (e.g. TICK_NSEC).
Signed-off-by: Anton Vorontsov <[email protected]>
Signed-off-by: Alexey Perevalov <[email protected]>
---
include/linux/jiffies.h | 4 +++-
include/linux/ktime.h | 3 ++-
kernel/time.c | 23 +++++++++++++++++++++++
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 1f44466..cbb15e0 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -6,6 +6,7 @@
#include <linux/types.h>
#include <linux/time.h>
#include <linux/timex.h>
+#include <linux/ktime.h>
#include <asm/param.h> /* for HZ */
/*
@@ -308,7 +309,8 @@ extern void jiffies_to_timespec(const unsigned long jiffies,
extern unsigned long timeval_to_jiffies(const struct timeval *value);
extern void jiffies_to_timeval(const unsigned long jiffies,
struct timeval *value);
-
+extern unsigned long ktime_to_jiffies(ktime_t *value);
+extern void jiffies_to_ktime(const unsigned long jiffies, ktime_t *value);
extern clock_t jiffies_to_clock_t(unsigned long x);
static inline clock_t jiffies_delta_to_clock_t(long delta)
{
diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index 31c0cd1..e8ed619 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -22,7 +22,6 @@
#define _LINUX_KTIME_H
#include <linux/time.h>
-#include <linux/jiffies.h>
/*
* ktime_t:
@@ -58,6 +57,8 @@ union ktime {
typedef union ktime ktime_t; /* Kill this */
+#include <linux/jiffies.h>
+
/*
* ktime_t definitions when using the 64-bit scalar representation:
*/
diff --git a/kernel/time.c b/kernel/time.c
index 7c7964c..22580a0 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -29,6 +29,7 @@
#include <linux/export.h>
#include <linux/timex.h>
+#include <linux/ktime.h>
#include <linux/capability.h>
#include <linux/timekeeper_internal.h>
#include <linux/errno.h>
@@ -575,6 +576,28 @@ void jiffies_to_timeval(const unsigned long jiffies, struct timeval *value)
}
EXPORT_SYMBOL(jiffies_to_timeval);
+unsigned long ktime_to_jiffies(ktime_t *value)
+{
+ struct timespec ts = ktime_to_timespec(*value);
+
+ /*
+ * nsecs_to_jiffies(ktime_to_ns(*ktime)) is unsafe as nsecs_to_jiffies
+ * doesn't handle MAX_JIFFY_OFFSET. So we reuse the logic from the
+ * timespec to jiffies conversion function.
+ */
+ return timespec_to_jiffies(&ts);
+}
+EXPORT_SYMBOL(ktime_to_jiffies);
+
+void jiffies_to_ktime(const unsigned long jiffies, ktime_t *value)
+{
+ struct timespec ts;
+
+ jiffies_to_timespec(jiffies, &ts);
+ *value = timespec_to_ktime(ts);
+}
+EXPORT_SYMBOL(jiffies_to_ktime);
+
/*
* Convert jiffies/jiffies_64 to clock_t and back.
*/
--
1.7.9.5
On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> From: Anton Vorontsov <[email protected]>
>
> Two new functions: jiffies_to_ktime() and ktime_to_jiffies(), we'll use
> them for timerfd deferred timers handling.
I don't think so. That's probably a leftover from the mess you tried
to create earlier.
Thanks,
tglx
On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> From: Anton Vorontsov <[email protected]>
>
> There is nothing hrtimer-specific inside the timerfd_tmrproc(), except
> the function prototype. We're about to add other timer types, so factor
> out generic timerfd_expire() helper from timerfd_tmrproc().
This changelog is completely useless. How is timerfd_tmrproc, which is
not a function but a function pointer, related to the patch?
Moving duplicated code to a common function is nice, but ....
> Signed-off-by: Anton Vorontsov <[email protected]>
> Signed-off-by: Alexey Perevalov <[email protected]>
> ---
> fs/timerfd.c | 40 +++++++++++++++++++---------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/fs/timerfd.c b/fs/timerfd.c
> index 9293121..3561ce7 100644
> --- a/fs/timerfd.c
> +++ b/fs/timerfd.c
> @@ -229,6 +229,23 @@ static unsigned int timerfd_poll(struct file *file, poll_table *wait)
> return events;
> }
>
> +static u64 timerfd_rearm(struct timerfd_ctx *ctx)
> +{
> + u64 orun;
> +
> + if (isalarm(ctx)) {
> + orun += alarm_forward_now(
> + &ctx->t.alarm, ctx->tintv) - 1;
> + alarm_restart(&ctx->t.alarm);
> + } else {
> + orun += hrtimer_forward_now(&ctx->t.tmr,
> + ctx->tintv) - 1;
> + hrtimer_restart(&ctx->t.tmr);
Warnings are there to be ignored and testing of user space
interfaces after a change is overrated, right?
Aside of that you just blindly copied the original code w/o fixing up
the now unnecessary line breaks.
The summary of this patch is:
1) Breaks existing functionality including user space ABI
2) Compiler warnings ignored
3) Untested
4) Utter lack of programming style
5) Useless changelog
Impressive for a trivial thing like this.
Thanks,
tglx
On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> These clockids is also used in current hrtimer implementation.
>
> Signed-off-by: Alexey Perevalov <[email protected]>
> ---
> include/trace/events/timer.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
> index 185b2c6..547b79f 100644
> --- a/include/trace/events/timer.h
> +++ b/include/trace/events/timer.h
> @@ -11,7 +11,10 @@
> #define clockid_to_string(clockid) \
> __print_symbolic(clockid, \
> { CLOCK_MONOTONIC, "CLOCK_MONOTONIC" }, \
> - { CLOCK_REALTIME, "CLOCK_REALTIME" })
> + { CLOCK_REALTIME, "CLOCK_REALTIME" }, \
> + { CLOCK_BOOTTIME, "CLOCK_BOOTTIME" }, \
> + { CLOCK_TAI, "CLOCK_TAI" } \
> + )
patching file include/trace/events/timer.h
Hunk #1 FAILED at 11.
1 out of 1 hunk FAILED -- rejects in file include/trace/events/timer.h
Brilliant stuff this.
On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> From: Anton Vorontsov <[email protected]>
>
> This patch implements a userland-side API for generic deferrable timers,
> per linux/timer.h:
>
> * 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.
>
> These timers are crucial for power saving, i.e. periodic tasks that want
> to work in background when the system is under use, but don't want to
> cause wakeups themselves.
>
> The deferred timers are somewhat orthogonal to high-res external timers,
> since the deferred timer is tied to the system load, not just to some
> external decrementer source.
Again this changelog makes no sense. What's orthogonal to high-res
timers and why are they external?
So 5 out of 6 patches are a trainwreck in various degrees of
wreckage. A pretty impressive achievement.
Thanks,
tglx
On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> Hello.
>
> This is a combo patch set for support defferability in timerfd.
> Due implementation of timerfd is based on hrtimers and only on hrtimers,
> it was necessary to add such deferrability into hrtimers too.
Sigh, this is not about timerfd. Adding new clock ids exposes them to
all other hrtimer based interfaces as well.
Thanks,
tglx
On 02/20/2014 03:01 PM, Thomas Gleixner wrote:
>
> On Thu, 20 Feb 2014, Alexey Perevalov wrote:
>
>> These clockids is also used in current hrtimer implementation.
>>
>> Signed-off-by: Alexey Perevalov <[email protected]>
>> ---
>> include/trace/events/timer.h | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
>> index 185b2c6..547b79f 100644
>> --- a/include/trace/events/timer.h
>> +++ b/include/trace/events/timer.h
>> @@ -11,7 +11,10 @@
>> #define clockid_to_string(clockid) \
>> __print_symbolic(clockid, \
>> { CLOCK_MONOTONIC, "CLOCK_MONOTONIC" }, \
>> - { CLOCK_REALTIME, "CLOCK_REALTIME" })
>> + { CLOCK_REALTIME, "CLOCK_REALTIME" }, \
>> + { CLOCK_BOOTTIME, "CLOCK_BOOTTIME" }, \
>> + { CLOCK_TAI, "CLOCK_TAI" } \
>> + )
> patching file include/trace/events/timer.h
> Hunk #1 FAILED at 11.
> 1 out of 1 hunk FAILED -- rejects in file include/trace/events/timer.h
>
> Brilliant stuff this.
>
>
>
>
>
Sorry, I missed one patch
--
Best regards,
Alexey Perevalov
On 02/20/2014 02:52 PM, Thomas Gleixner wrote:
> On Thu, 20 Feb 2014, Alexey Perevalov wrote:
>
>> From: Anton Vorontsov <[email protected]>
>>
>> There is nothing hrtimer-specific inside the timerfd_tmrproc(), except
>> the function prototype. We're about to add other timer types, so factor
>> out generic timerfd_expire() helper from timerfd_tmrproc().
> This changelog is completely useless. How is timerfd_tmrproc, which is
> not a function but a function pointer, related to the patch?
>
> Moving duplicated code to a common function is nice, but ....
>
>> Signed-off-by: Anton Vorontsov <[email protected]>
>> Signed-off-by: Alexey Perevalov <[email protected]>
>> ---
>> fs/timerfd.c | 40 +++++++++++++++++++---------------------
>> 1 file changed, 19 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/timerfd.c b/fs/timerfd.c
>> index 9293121..3561ce7 100644
>> --- a/fs/timerfd.c
>> +++ b/fs/timerfd.c
>> @@ -229,6 +229,23 @@ static unsigned int timerfd_poll(struct file *file, poll_table *wait)
>> return events;
>> }
>>
>> +static u64 timerfd_rearm(struct timerfd_ctx *ctx)
>> +{
>> + u64 orun;
>> +
>> + if (isalarm(ctx)) {
>> + orun += alarm_forward_now(
>> + &ctx->t.alarm, ctx->tintv) - 1;
>> + alarm_restart(&ctx->t.alarm);
>> + } else {
>> + orun += hrtimer_forward_now(&ctx->t.tmr,
>> + ctx->tintv) - 1;
>> + hrtimer_restart(&ctx->t.tmr);
> Warnings are there to be ignored and testing of user space
> interfaces after a change is overrated, right?
>
> Aside of that you just blindly copied the original code w/o fixing up
> the now unnecessary line breaks.
>
> The summary of this patch is:
>
> 1) Breaks existing functionality including user space ABI
> 2) Compiler warnings ignored
> 3) Untested
> 4) Utter lack of programming style
> 5) Useless changelog
>
> Impressive for a trivial thing like this.
>
> Thanks,
>
> tglx
>
Compiler warning - if you mean uninitialized orun, I fixed it
ill-timed in the next patch,
yes it should be in original patch.
--
Best regards,
Alexey Perevalov
On 02/20/2014 03:12 PM, Thomas Gleixner wrote:
> On Thu, 20 Feb 2014, Alexey Perevalov wrote:
>
>> Hello.
>>
>> This is a combo patch set for support defferability in timerfd.
>> Due implementation of timerfd is based on hrtimers and only on hrtimers,
>> it was necessary to add such deferrability into hrtimers too.
> Sigh, this is not about timerfd. Adding new clock ids exposes them to
> all other hrtimer based interfaces as well.
Posix timers, maybe something else,
do you want to separate this patch set, or add to this patch set,
1. proper description
2. and if it'll touch posix-timer, posix_timers clocks should be
extended too.
>
> Thanks,
>
> tglx
>
--
Best regards,
Alexey Perevalov
On 02/20/2014 12:40 AM, Alexey Perevalov wrote:
> From: Thomas Gleixner <[email protected]>
>
> This patch introduces new public CLOCKID constants for
> user space API, such as timerfd. It extends hrtimer API and makes
> possible to have unified interfaces where deferreble functionality is
> used. In-kernel users such as device drivers could find benefits too.
>
> High resolution timer now could work with CLOCK_REALTIME_DEFERRABLE,
> CLOCK_MONOTONIC_DEFERRABLE, CLOCK_BOOTTIME_DEFERRABLE.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Alexey Perevalov <[email protected]>
> ---
> include/linux/hrtimer.h | 3 +++
> include/uapi/linux/time.h | 3 +++
> kernel/hrtimer.c | 62 +++++++++++++++++++++++++++++++++++----------
> 3 files changed, 55 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
> index d19a5c2..fe1159c 100644
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -158,6 +158,9 @@ enum hrtimer_base_type {
> HRTIMER_BASE_REALTIME,
> HRTIMER_BASE_BOOTTIME,
> HRTIMER_BASE_TAI,
> + HRTIMER_BASE_MONOTONIC_DEFERRABLE,
> + HRTIMER_BASE_REALTIME_DEFERRABLE,
> + HRTIMER_BASE_BOOTTIME_DEFERRABLE,
> HRTIMER_MAX_CLOCK_BASES,
> };
>
> diff --git a/include/uapi/linux/time.h b/include/uapi/linux/time.h
> index e75e1b6..bb8dc60 100644
> --- a/include/uapi/linux/time.h
> +++ b/include/uapi/linux/time.h
> @@ -56,6 +56,9 @@ struct itimerval {
> #define CLOCK_BOOTTIME_ALARM 9
> #define CLOCK_SGI_CYCLE 10 /* Hardware specific */
> #define CLOCK_TAI 11
> +#define CLOCK_REALTIME_DEFERRABLE 12
> +#define CLOCK_MONOTONIC_DEFERRABLE 13
> +#define CLOCK_BOOTTIME_DEFERRABLE 14
Adding the deferrable HRTIMER bases above is right, but I don't think we
agreed on adding the _DEFERRABLE clockids.
I'd instead prefer you use add a new TIMER_DEFERABLE flags argument, and
use the combination of the clockid + flag to decide which HRTIMER base
is used.
thanks
-john
On Thu, 20 Feb 2014, John Stultz wrote:
> On 02/20/2014 12:40 AM, Alexey Perevalov wrote:
> > diff --git a/include/uapi/linux/time.h b/include/uapi/linux/time.h
> > index e75e1b6..bb8dc60 100644
> > --- a/include/uapi/linux/time.h
> > +++ b/include/uapi/linux/time.h
> > @@ -56,6 +56,9 @@ struct itimerval {
> > #define CLOCK_BOOTTIME_ALARM 9
> > #define CLOCK_SGI_CYCLE 10 /* Hardware specific */
> > #define CLOCK_TAI 11
> > +#define CLOCK_REALTIME_DEFERRABLE 12
> > +#define CLOCK_MONOTONIC_DEFERRABLE 13
> > +#define CLOCK_BOOTTIME_DEFERRABLE 14
>
> Adding the deferrable HRTIMER bases above is right, but I don't think we
> agreed on adding the _DEFERRABLE clockids.
>
> I'd instead prefer you use add a new TIMER_DEFERABLE flags argument, and
> use the combination of the clockid + flag to decide which HRTIMER base
> is used.
And how does that work with anything else than timerfd? If we add
deferrable posix clocks then we add them for the other interfaces
which take a clockid as well.
Thanks,
tglx
On 02/20/2014 01:18 PM, Thomas Gleixner wrote:
> On Thu, 20 Feb 2014, John Stultz wrote:
>> On 02/20/2014 12:40 AM, Alexey Perevalov wrote:
>>> diff --git a/include/uapi/linux/time.h b/include/uapi/linux/time.h
>>> index e75e1b6..bb8dc60 100644
>>> --- a/include/uapi/linux/time.h
>>> +++ b/include/uapi/linux/time.h
>>> @@ -56,6 +56,9 @@ struct itimerval {
>>> #define CLOCK_BOOTTIME_ALARM 9
>>> #define CLOCK_SGI_CYCLE 10 /* Hardware specific */
>>> #define CLOCK_TAI 11
>>> +#define CLOCK_REALTIME_DEFERRABLE 12
>>> +#define CLOCK_MONOTONIC_DEFERRABLE 13
>>> +#define CLOCK_BOOTTIME_DEFERRABLE 14
>> Adding the deferrable HRTIMER bases above is right, but I don't think we
>> agreed on adding the _DEFERRABLE clockids.
>>
>> I'd instead prefer you use add a new TIMER_DEFERABLE flags argument, and
>> use the combination of the clockid + flag to decide which HRTIMER base
>> is used.
> And how does that work with anything else than timerfd? If we add
> deferrable posix clocks then we add them for the other interfaces
> which take a clockid as well.
Other interfaces have flag arguments as well (for things like
TIMER_ABSTIME).
thanks
-john
On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> On 02/20/2014 03:12 PM, Thomas Gleixner wrote:
> > On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> >
> > > Hello.
> > >
> > > This is a combo patch set for support defferability in timerfd.
> > > Due implementation of timerfd is based on hrtimers and only on hrtimers,
> > > it was necessary to add such deferrability into hrtimers too.
> > Sigh, this is not about timerfd. Adding new clock ids exposes them to
> > all other hrtimer based interfaces as well.
> Posix timers, maybe something else,
> do you want to separate this patch set, or add to this patch set,
> 1. proper description
> 2. and if it'll touch posix-timer, posix_timers clocks should be extended too.
Did you even try to understand the scope of the deferrable clock id
changes?
I really wonder why you try to hack such an interface without even
remotely understanding how the related user space interfaces work.
Thanks,
tglx
On Thu, 20 Feb 2014, John Stultz wrote:
> On 02/20/2014 01:18 PM, Thomas Gleixner wrote:
> > On Thu, 20 Feb 2014, John Stultz wrote:
> >> On 02/20/2014 12:40 AM, Alexey Perevalov wrote:
> >>> diff --git a/include/uapi/linux/time.h b/include/uapi/linux/time.h
> >>> index e75e1b6..bb8dc60 100644
> >>> --- a/include/uapi/linux/time.h
> >>> +++ b/include/uapi/linux/time.h
> >>> @@ -56,6 +56,9 @@ struct itimerval {
> >>> #define CLOCK_BOOTTIME_ALARM 9
> >>> #define CLOCK_SGI_CYCLE 10 /* Hardware specific */
> >>> #define CLOCK_TAI 11
> >>> +#define CLOCK_REALTIME_DEFERRABLE 12
> >>> +#define CLOCK_MONOTONIC_DEFERRABLE 13
> >>> +#define CLOCK_BOOTTIME_DEFERRABLE 14
> >> Adding the deferrable HRTIMER bases above is right, but I don't think we
> >> agreed on adding the _DEFERRABLE clockids.
> >>
> >> I'd instead prefer you use add a new TIMER_DEFERABLE flags argument, and
> >> use the combination of the clockid + flag to decide which HRTIMER base
> >> is used.
> > And how does that work with anything else than timerfd? If we add
> > deferrable posix clocks then we add them for the other interfaces
> > which take a clockid as well.
> Other interfaces have flag arguments as well (for things like
> TIMER_ABSTIME).
Fair enough. Let me look tomorrow what implications that
has. Definitely a bit more intrusive, but probably more elegant.
Thanks,
tglx
On Thu, Feb 20, 2014 at 12:09:43PM +0100, Thomas Gleixner wrote:
> On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> > From: Anton Vorontsov <[email protected]>
> >
> > This patch implements a userland-side API for generic deferrable timers,
> > per linux/timer.h:
> >
> > * 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.
> >
> > These timers are crucial for power saving, i.e. periodic tasks that want
> > to work in background when the system is under use, but don't want to
> > cause wakeups themselves.
> >
> > The deferred timers are somewhat orthogonal to high-res external timers,
> > since the deferred timer is tied to the system load, not just to some
> > external decrementer source.
>
> Again this changelog makes no sense. What's orthogonal to high-res
> timers and why are they external?
Not trying to defend the current series, just felt the need clarify this
one.
By orthogonal I meant that comparing to high resolution timers' use cases,
deferred timers can be super-low resolution, super inaccurate. We don't
know exactly when they will fire, all we know is something like "every 0.2
seconds, iff the system/user is doing something, otherwise don't bother."
As for external (my bad, shouldn't invent personal terminology): the
hrtimers are tied to some clock source (which is "external" to me), but
deferred timers are mostly tied to the system's activity.
Thanks,
Anton
On Thu, Feb 20, 2014 at 11:52:03AM +0100, Thomas Gleixner wrote:
> > From: Anton Vorontsov <[email protected]>
> >
> > There is nothing hrtimer-specific inside the timerfd_tmrproc(), except
> > the function prototype. We're about to add other timer types, so factor
> > out generic timerfd_expire() helper from timerfd_tmrproc().
>
> This changelog is completely useless. How is timerfd_tmrproc, which is
> not a function but a function pointer, related to the patch?
>
> Moving duplicated code to a common function is nice, but ....
> > Signed-off-by: Anton Vorontsov <[email protected]>
> > Signed-off-by: Alexey Perevalov <[email protected]>
...
> Warnings are there to be ignored and testing of user space
> interfaces after a change is overrated, right?
>
> Aside of that you just blindly copied the original code w/o fixing up
> the now unnecessary line breaks.
Alexey,
While I appreciate the desire to be careful with authorship and stuff,
please remove my name as an author of this patch -- the current code has
nothing to do with my original work, and I surely don't want to take any
responsibility for it. This is a common practice if you modify someone's
patch to a great extend.
Thomas is bashing the thing, which has my name on it; although _my_ patch
did not produce any warnings, came with a completely different changelog,
and served completely different purposes.
Instead of rushing with resending yet another series, please actually read
Thomas' review.
Thanks,
Anton
On 02/21/2014 08:13 AM, Anton Vorontsov wrote:
> On Thu, Feb 20, 2014 at 11:52:03AM +0100, Thomas Gleixner wrote:
>>> From: Anton Vorontsov <[email protected]>
>>>
>>> There is nothing hrtimer-specific inside the timerfd_tmrproc(), except
>>> the function prototype. We're about to add other timer types, so factor
>>> out generic timerfd_expire() helper from timerfd_tmrproc().
>> This changelog is completely useless. How is timerfd_tmrproc, which is
>> not a function but a function pointer, related to the patch?
>>
>> Moving duplicated code to a common function is nice, but ....
>>> Signed-off-by: Anton Vorontsov <[email protected]>
>>> Signed-off-by: Alexey Perevalov <[email protected]>
> ...
>> Warnings are there to be ignored and testing of user space
>> interfaces after a change is overrated, right?
>>
>> Aside of that you just blindly copied the original code w/o fixing up
>> the now unnecessary line breaks.
> Alexey,
>
> While I appreciate the desire to be careful with authorship and stuff,
> please remove my name as an author of this patch -- the current code has
> nothing to do with my original work, and I surely don't want to take any
> responsibility for it. This is a common practice if you modify someone's
> patch to a great extend.
>
> Thomas is bashing the thing, which has my name on it; although _my_ patch
> did not produce any warnings, came with a completely different changelog,
> and served completely different purposes.
Sorry, it was introduced by me, while rebasing.
>
> Instead of rushing with resending yet another series, please actually read
> Thomas' review.
>
> Thanks,
>
> Anton
>
--
Best regards,
Alexey Perevalov
On Thu, 20 Feb 2014, Anton Vorontsov wrote:
> On Thu, Feb 20, 2014 at 12:09:43PM +0100, Thomas Gleixner wrote:
> > On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> > > From: Anton Vorontsov <[email protected]>
> > >
> > > This patch implements a userland-side API for generic deferrable timers,
> > > per linux/timer.h:
> > >
> > > * 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.
> > >
> > > These timers are crucial for power saving, i.e. periodic tasks that want
> > > to work in background when the system is under use, but don't want to
> > > cause wakeups themselves.
> > >
> > > The deferred timers are somewhat orthogonal to high-res external timers,
> > > since the deferred timer is tied to the system load, not just to some
> > > external decrementer source.
> >
> > Again this changelog makes no sense. What's orthogonal to high-res
> > timers and why are they external?
>
> Not trying to defend the current series, just felt the need clarify this
> one.
>
> By orthogonal I meant that comparing to high resolution timers' use cases,
> deferred timers can be super-low resolution, super inaccurate. We don't
> know exactly when they will fire, all we know is something like "every 0.2
> seconds, iff the system/user is doing something, otherwise don't bother."
Ok.
> As for external (my bad, shouldn't invent personal terminology): the
> hrtimers are tied to some clock source (which is "external" to me), but
> deferred timers are mostly tied to the system's activity.
I see what you mean, but that wants some different wording.
Thanks,
tglx
On Thu, 20 Feb 2014, Anton Vorontsov wrote:
> On Thu, Feb 20, 2014 at 11:52:03AM +0100, Thomas Gleixner wrote:
> Instead of rushing with resending yet another series, please actually read
> Thomas' review.
And please hold off until we sorted out the clockid vs. flags discussion.
Thanks,
tglx