2014-02-20 16:24:26

by Alexey Perevalov

[permalink] [raw]
Subject: [PATCH v4 0/6] Deferrable timers support for hrtimers/timerfd API

Hello,

This patch set introduces deferrable functionality for hrtimers and add ability
to use it from timerfd_create syscall.
It means user-space processes are able to not wake up the system from cpuidle state
in case of periodic job. And it's the main goal of this patch set for us and for team
sent such patch set before.
I hope it will find users in kernel space too, cause it could avoid timer_list drawbacks,
such as worst case when many,many timers are gonna to be expired.


Kernel already has deferrable timer funcionality based on timer_list. But deferrable
functionality inside hrtimers makes timerfd implemenation trivial for such case.

It's based on github.com/torvalds/linux.git git repository on top of
e95003c3f9ccbfa7ab9d265e6eb703ee2fa4cfe7

Alexey Perevalov (3):
Replace ternary operator to macro
tracing/trivial: Add CLOCK_BOOTIME and CLOCK_TAI for human readable
clockid trace
tracing/trivial: Add CLOCK_*_DEFERRABLE for tracing clockids

Anton Vorontsov (2):
timerfd: Move repeated logic into timerfd_rearm()
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/trace/events/timer.h | 25 ++++++++++++++---
include/uapi/linux/time.h | 3 ++
kernel/hrtimer.c | 62 +++++++++++++++++++++++++++++++++---------
5 files changed, 100 insertions(+), 40 deletions(-)

--
1.7.9.5


2014-02-20 16:24:36

by Alexey Perevalov

[permalink] [raw]
Subject: [PATCH v4 1/6] Replace ternary operator to macro

For extensibility purpose it's better to have _switch_ type mechanism
for representing human readable CLOCKID* and HRMODE.

Signed-off-by: Alexey Perevalov <[email protected]>
---
include/trace/events/timer.h | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index 68c2c20..185b2c6 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -8,6 +8,16 @@
#include <linux/hrtimer.h>
#include <linux/timer.h>

+#define clockid_to_string(clockid) \
+ __print_symbolic(clockid, \
+ { CLOCK_MONOTONIC, "CLOCK_MONOTONIC" }, \
+ { CLOCK_REALTIME, "CLOCK_REALTIME" })
+
+#define hrmode_to_string(hrmode) \
+ __print_symbolic(hrmode, \
+ { HRTIMER_MODE_ABS, "HRTIMER_MODE_ABS" }, \
+ { HRTIMER_MODE_REL, "HRTIMER_MODE_REL" })
+
DECLARE_EVENT_CLASS(timer_class,

TP_PROTO(struct timer_list *timer),
@@ -147,10 +157,8 @@ TRACE_EVENT(hrtimer_init,
),

TP_printk("hrtimer=%p clockid=%s mode=%s", __entry->hrtimer,
- __entry->clockid == CLOCK_REALTIME ?
- "CLOCK_REALTIME" : "CLOCK_MONOTONIC",
- __entry->mode == HRTIMER_MODE_ABS ?
- "HRTIMER_MODE_ABS" : "HRTIMER_MODE_REL")
+ clockid_to_string(__entry->clockid),
+ hrmode_to_string(__entry->mode))
);

/**
--
1.7.9.5

2014-02-20 16:24:42

by Alexey Perevalov

[permalink] [raw]
Subject: [PATCH v4 2/6] tracing/trivial: Add CLOCK_BOOTIME and CLOCK_TAI for human readable clockid trace

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

2014-02-20 16:24:49

by Alexey Perevalov

[permalink] [raw]
Subject: [PATCH v4 6/6] tracing/trivial: Add CLOCK_*_DEFERRABLE for tracing clockids

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

2014-02-20 16:24:45

by Alexey Perevalov

[permalink] [raw]
Subject: [PATCH v4 3/6] hrtimer: Add support for deferrable timer into the hrtimer

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

2014-02-20 16:25:15

by Alexey Perevalov

[permalink] [raw]
Subject: [PATCH v4 5/6] timerfd: Add support for deferrable timers

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.

Currently, the implementation is based on high resolution timers. It
provides the same overrun count as for none deferrable timers. What's why
it doesn't not bring any new functionality into timerfd implementation,
except clockid validation.

Signed-off-by: Anton Vorontsov <[email protected]>
Signed-off-by: Alexey Perevalov <[email protected]>
---
fs/timerfd.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/timerfd.c b/fs/timerfd.c
index 4a349cb..c132fd2 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -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

2014-02-20 16:25:42

by Alexey Perevalov

[permalink] [raw]
Subject: [PATCH v4 4/6] timerfd: Move repeated logic into timerfd_rearm()

From: Anton Vorontsov <[email protected]>

This patch introduces timerfd_rearm(), this small helper is used to
forward and restart the hrtimer.

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..4a349cb 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 = 0;
+
+ 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

2014-02-20 20:49:11

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] Replace ternary operator to macro

On Thu, 20 Feb 2014, Alexey Perevalov wrote:

The subject line lacks any information in which part of the kernel you
are doing this.

> For extensibility purpose it's better to have _switch_ type mechanism
> for representing human readable CLOCKID* and HRMODE.
>
> Signed-off-by: Alexey Perevalov <[email protected]>
> ---
> include/trace/events/timer.h | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
> index 68c2c20..185b2c6 100644
> --- a/include/trace/events/timer.h
> +++ b/include/trace/events/timer.h
> @@ -8,6 +8,16 @@
> #include <linux/hrtimer.h>
> #include <linux/timer.h>
>
> +#define clockid_to_string(clockid) \
> + __print_symbolic(clockid, \
> + { CLOCK_MONOTONIC, "CLOCK_MONOTONIC" }, \
> + { CLOCK_REALTIME, "CLOCK_REALTIME" })
> +
> +#define hrmode_to_string(hrmode) \
> + __print_symbolic(hrmode, \
> + { HRTIMER_MODE_ABS, "HRTIMER_MODE_ABS" }, \
> + { HRTIMER_MODE_REL, "HRTIMER_MODE_REL" })
> +
> DECLARE_EVENT_CLASS(timer_class,
>
> TP_PROTO(struct timer_list *timer),
> @@ -147,10 +157,8 @@ TRACE_EVENT(hrtimer_init,
> ),
>
> TP_printk("hrtimer=%p clockid=%s mode=%s", __entry->hrtimer,
> - __entry->clockid == CLOCK_REALTIME ?
> - "CLOCK_REALTIME" : "CLOCK_MONOTONIC",
> - __entry->mode == HRTIMER_MODE_ABS ?
> - "HRTIMER_MODE_ABS" : "HRTIMER_MODE_REL")
> + clockid_to_string(__entry->clockid),
> + hrmode_to_string(__entry->mode))
> );
>
> /**
> --
> 1.7.9.5
>
>

2014-02-20 20:49:45

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH v4 2/6] tracing/trivial: Add CLOCK_BOOTIME and CLOCK_TAI for human readable clockid trace

On Thu, 20 Feb 2014, Alexey Perevalov wrote:

> These clockids is also used in current hrtimer implementation.

tracing/trivial ???

It's a functional patch related to tracing.

> 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
>
>

2014-02-20 21:13:07

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH v4 4/6] timerfd: Move repeated logic into timerfd_rearm()

On Thu, 20 Feb 2014, Alexey Perevalov wrote:
> From: Anton Vorontsov <[email protected]>
>
> This patch introduces timerfd_rearm(), this small helper is used to
> forward and restart the hrtimer.

And for what is this helper used for?

> 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..4a349cb 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 = 0;
> +
> + 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;

I really give up. You did not even try to read and understand my
review points.

No, you went and mechanicallly fixed the compiler warning in the most
idiotic way one can come up with.

What's wrong with writing it:

{
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;
}

Can you spot the difference? Just for the record:

1) It fixes the issue with proper assignments instead of pointlessly
initializing the variable to 0 and then add the return value.

2) It removes the useless line break which makes the code simpler to
read.

I told you to do #2 explicitely, but you decided to ignore it.

I did not tell you how to do #1, but I did not expect at all that
anyone might come up with that horrible solution.

This is not some random homework assignment in high school where you
might get away with the "It compiles so what" mentality.

Seriously, if you come back with another set of half baken patches,
you're going to end up in the utter-waste-of-time section of my
.procmailrc.

I have quite some patience with people, but I really have better
things to do than wasting my scarce time with advisory resistant
wannabe kernel developers.

Thanks,

tglx

Hint: Take your time. Read carefully what I asked for and let your
coworkers look over the patches including the subject lines and
the changelogs before you send another half baken crap out in a
haste.

2014-02-26 02:53:14

by Andy Lutomirski

[permalink] [raw]
Subject: Re: [PATCH v4 5/6] timerfd: Add support for deferrable timers

On 02/20/2014 08:23 AM, 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.

Please don't. This API sucks for all kinds of reasons:

- Why is it a new kind of clock?
- How deferrable is deferrable?
- It adds new core code, which serves no purpose (the problem is
already solved).

On the other hand, if you added a fancier version of timerfd_settime
that could explicitly set the slack value (or, equivalently, the
earliest and latest allowable times), that could be quite useful.

It's often bugged me that timer slack is per-process.

--Andy