In hrtimer_get_next_event we unconditionally lock/unlock spinlock, even if it's
not required (hrtimer_hres_active() != 0). This patch moves
locking/unlocking and mindelta range check inside the if clause,
so we don't execute unnecessary operations.
Signed-off-by: Stanislav Fomichev <[email protected]>
---
kernel/hrtimer.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 39f339dcbe93..ce21e5f6bcf0 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1166,9 +1166,9 @@ ktime_t hrtimer_get_next_event(void)
unsigned long flags;
int i;
- raw_spin_lock_irqsave(&cpu_base->lock, flags);
-
if (!hrtimer_hres_active()) {
+ raw_spin_lock_irqsave(&cpu_base->lock, flags);
+
for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
struct hrtimer *timer;
struct timerqueue_node *next;
@@ -1183,12 +1183,13 @@ ktime_t hrtimer_get_next_event(void)
if (delta.tv64 < mindelta.tv64)
mindelta.tv64 = delta.tv64;
}
- }
- raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
+ raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
+
+ if (mindelta.tv64 < 0)
+ mindelta.tv64 = 0;
+ }
- if (mindelta.tv64 < 0)
- mindelta.tv64 = 0;
return mindelta;
}
#endif
--
1.8.3.2
On 9 June 2014 14:53, Stanislav Fomichev <[email protected]> wrote:
> In hrtimer_get_next_event we unconditionally lock/unlock spinlock, even if it's
> not required (hrtimer_hres_active() != 0). This patch moves
> locking/unlocking and mindelta range check inside the if clause,
> so we don't execute unnecessary operations.
>
> Signed-off-by: Stanislav Fomichev <[email protected]>
> ---
> kernel/hrtimer.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index 39f339dcbe93..ce21e5f6bcf0 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -1166,9 +1166,9 @@ ktime_t hrtimer_get_next_event(void)
> unsigned long flags;
> int i;
>
> - raw_spin_lock_irqsave(&cpu_base->lock, flags);
> -
> if (!hrtimer_hres_active()) {
> + raw_spin_lock_irqsave(&cpu_base->lock, flags);
> +
> for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
> struct hrtimer *timer;
> struct timerqueue_node *next;
> @@ -1183,12 +1183,13 @@ ktime_t hrtimer_get_next_event(void)
> if (delta.tv64 < mindelta.tv64)
> mindelta.tv64 = delta.tv64;
> }
> - }
>
> - raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
> + raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
> +
> + if (mindelta.tv64 < 0)
> + mindelta.tv64 = 0;
> + }
>
> - if (mindelta.tv64 < 0)
> - mindelta.tv64 = 0;
> return mindelta;
> }
Reviewed-by: Viresh Kumar <[email protected]>
On Mon, 9 Jun 2014, Stanislav Fomichev wrote:
> In hrtimer_get_next_event we unconditionally lock/unlock spinlock, even if it's
> not required (hrtimer_hres_active() != 0). This patch moves
> locking/unlocking and mindelta range check inside the if clause,
> so we don't execute unnecessary operations.
What's wrong with simply doing:
if (!hrtimer_hres_active())
return mindelta;
That saves and indentation level and makes the code more
readable. Also the lockless check wants a comment why it is correct.
Thanks,
tglx
> What's wrong with simply doing:
>
> if (!hrtimer_hres_active())
> return mindelta;
Nothing wrong, just makes diff bigger :-)
> That saves and indentation level and makes the code more
> readable. Also the lockless check wants a comment why it is correct.
Ok, I'll follow with updated patch.
In hrtimer_get_next_event we unconditionally lock/unlock spinlock, even if it's
not required (hrtimer_hres_active() != 0). This patch adds fast path
when highres is active so we don't execute unnecessary operations.
We can safely do lockless check because:
- hrtimer_get_next_event is always called with interrupts disabled;
- we may switch to hres only from softirq handler with disabled interrupts.
Because we only care about hres_active which may be changed only from
local CPU, we can use interrupt context for synchronization.
run_timer_softirq
hrtimer_run_pending
hrtimer_switch_to_hres
local_irq_save <-
base->hres_active = 0
tick_nohz_idle_enter
local_irq_disable <-
__tick_nohz_idle_enter
tick_nohz_stop_sched_tick
get_next_timer_interrupt
cmp_next_hrtimer_event
hrtimer_get_next_event
check base->hres_active
irq_exit <- irq context
tick_irq_exit
tick_nohz_irq_exit
tick_nohz_full_stop_tick
tick_nohz_stop_sched_tick
... <see above>
__tick_nohz_idle_enter
... <see above>
Signed-off-by: Stanislav Fomichev <[email protected]>
---
kernel/hrtimer.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index ba340739c701..731be91e927f 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1167,23 +1167,24 @@ ktime_t hrtimer_get_next_event(void)
unsigned long flags;
int i;
+ if (hrtimer_hres_active())
+ return mindelta;
+
raw_spin_lock_irqsave(&cpu_base->lock, flags);
- if (!hrtimer_hres_active()) {
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
- struct hrtimer *timer;
- struct timerqueue_node *next;
+ for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
+ struct hrtimer *timer;
+ struct timerqueue_node *next;
- next = timerqueue_getnext(&base->active);
- if (!next)
- continue;
+ next = timerqueue_getnext(&base->active);
+ if (!next)
+ continue;
- timer = container_of(next, struct hrtimer, node);
- delta.tv64 = hrtimer_get_expires_tv64(timer);
- delta = ktime_sub(delta, base->get_time());
- if (delta.tv64 < mindelta.tv64)
- mindelta.tv64 = delta.tv64;
- }
+ timer = container_of(next, struct hrtimer, node);
+ delta.tv64 = hrtimer_get_expires_tv64(timer);
+ delta = ktime_sub(delta, base->get_time());
+ if (delta.tv64 < mindelta.tv64)
+ mindelta.tv64 = delta.tv64;
}
raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
--
1.8.3.2
On 9 June 2014 18:29, Stanislav Fomichev <[email protected]> wrote:
> In hrtimer_get_next_event we unconditionally lock/unlock spinlock, even if it's
> not required (hrtimer_hres_active() != 0). This patch adds fast path
> when highres is active so we don't execute unnecessary operations.
>
> We can safely do lockless check because:
> - hrtimer_get_next_event is always called with interrupts disabled;
> - we may switch to hres only from softirq handler with disabled interrupts.
>
> Because we only care about hres_active which may be changed only from
> local CPU, we can use interrupt context for synchronization.
>
> run_timer_softirq
> hrtimer_run_pending
> hrtimer_switch_to_hres
> local_irq_save <-
> base->hres_active = 0
>
> tick_nohz_idle_enter
> local_irq_disable <-
> __tick_nohz_idle_enter
> tick_nohz_stop_sched_tick
> get_next_timer_interrupt
> cmp_next_hrtimer_event
> hrtimer_get_next_event
> check base->hres_active
>
> irq_exit <- irq context
> tick_irq_exit
> tick_nohz_irq_exit
> tick_nohz_full_stop_tick
> tick_nohz_stop_sched_tick
> ... <see above>
> __tick_nohz_idle_enter
> ... <see above>
>
> Signed-off-by: Stanislav Fomichev <[email protected]>
> ---
> kernel/hrtimer.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index ba340739c701..731be91e927f 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -1167,23 +1167,24 @@ ktime_t hrtimer_get_next_event(void)
> unsigned long flags;
> int i;
>
> + if (hrtimer_hres_active())
> + return mindelta;
> +
> raw_spin_lock_irqsave(&cpu_base->lock, flags);
>
> - if (!hrtimer_hres_active()) {
> - for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
> - struct hrtimer *timer;
> - struct timerqueue_node *next;
> + for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
> + struct hrtimer *timer;
> + struct timerqueue_node *next;
>
> - next = timerqueue_getnext(&base->active);
> - if (!next)
> - continue;
> + next = timerqueue_getnext(&base->active);
> + if (!next)
> + continue;
>
> - timer = container_of(next, struct hrtimer, node);
> - delta.tv64 = hrtimer_get_expires_tv64(timer);
> - delta = ktime_sub(delta, base->get_time());
> - if (delta.tv64 < mindelta.tv64)
> - mindelta.tv64 = delta.tv64;
> - }
> + timer = container_of(next, struct hrtimer, node);
> + delta.tv64 = hrtimer_get_expires_tv64(timer);
> + delta = ktime_sub(delta, base->get_time());
> + if (delta.tv64 < mindelta.tv64)
> + mindelta.tv64 = delta.tv64;
> }
>
> raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
Even I was concerned about a bigger diff earlier and so didn't comment
to rewrite it this way :)
Codewise, looks much better..
Reviewed-by: Viresh Kumar <[email protected]>
On Mon, 9 Jun 2014, Stanislav Fomichev wrote:
> In hrtimer_get_next_event we unconditionally lock/unlock spinlock, even if it's
> not required (hrtimer_hres_active() != 0). This patch adds fast path
> when highres is active so we don't execute unnecessary operations.
>
> We can safely do lockless check because:
> - hrtimer_get_next_event is always called with interrupts disabled;
> - we may switch to hres only from softirq handler with disabled interrupts.
>
> Because we only care about hres_active which may be changed only from
> local CPU, we can use interrupt context for synchronization.
>
> run_timer_softirq
> hrtimer_run_pending
> hrtimer_switch_to_hres
> local_irq_save <-
> base->hres_active = 0
>
> tick_nohz_idle_enter
> local_irq_disable <-
> __tick_nohz_idle_enter
> tick_nohz_stop_sched_tick
> get_next_timer_interrupt
> cmp_next_hrtimer_event
> hrtimer_get_next_event
> check base->hres_active
>
> irq_exit <- irq context
> tick_irq_exit
> tick_nohz_irq_exit
> tick_nohz_full_stop_tick
> tick_nohz_stop_sched_tick
> ... <see above>
> __tick_nohz_idle_enter
> ... <see above>
Sigh. Is anybody actually reading and trying to understand what I
write in reviews?
> Also the lockless check wants a comment why it is correct.
Is it that hard? A comment is NOT a uberlenghty explanation in the
changelog. A comment starts with /* and ends with */ and is in the
code.
/*
* Called with interrupts disabled and therefor
* protected against a switch to high resolution mode.
*/
That's a comment, right?
Thanks,
tglx
On Mon, Jun 09, 2014 at 06:58:56PM +0530, Viresh Kumar wrote:
>
> Even I was concerned about a bigger diff earlier and so didn't comment
> to rewrite it this way :)
WTF is wrong with you people? Who bloody cares how big diffs are? The
clarity of the resulting code is _far_ more important.
Please as to stop taking bong hits.
> Sigh. Is anybody actually reading and trying to understand what I
> write in reviews?
>
> > Also the lockless check wants a comment why it is correct.
>
> Is it that hard? A comment is NOT a uberlenghty explanation in the
> changelog. A comment starts with /* and ends with */ and is in the
> code.
>
> /*
> * Called with interrupts disabled and therefor
> * protected against a switch to high resolution mode.
> */
>
> That's a comment, right?
Duh, I honestly thought you were talking about more details in the
changelog. How come you didn't like it? Piece of art :-)
Anyway, here is another respin of the patch.
Btw, maybe it also makes sense to convert _irqsave spinlocks to non-irq
ones? If we now have a comment telling we are called with interrupts
disabled, there is no way we need irq safe locks, right?
--
In hrtimer_get_next_event we unconditionally lock/unlock spinlock, even if it's
not required (hrtimer_hres_active() != 0). This patch adds fast path
when highres is active so we don't execute unnecessary operations.
Signed-off-by: Stanislav Fomichev <[email protected]>
---
kernel/hrtimer.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index ba340739c701..245c90e5626b 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1167,23 +1167,28 @@ ktime_t hrtimer_get_next_event(void)
unsigned long flags;
int i;
+ /*
+ * Called with interrupts disabled and therefore
+ * protected against a switch to high resolution mode.
+ */
+ if (hrtimer_hres_active())
+ return mindelta;
+
raw_spin_lock_irqsave(&cpu_base->lock, flags);
- if (!hrtimer_hres_active()) {
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
- struct hrtimer *timer;
- struct timerqueue_node *next;
+ for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
+ struct hrtimer *timer;
+ struct timerqueue_node *next;
- next = timerqueue_getnext(&base->active);
- if (!next)
- continue;
+ next = timerqueue_getnext(&base->active);
+ if (!next)
+ continue;
- timer = container_of(next, struct hrtimer, node);
- delta.tv64 = hrtimer_get_expires_tv64(timer);
- delta = ktime_sub(delta, base->get_time());
- if (delta.tv64 < mindelta.tv64)
- mindelta.tv64 = delta.tv64;
- }
+ timer = container_of(next, struct hrtimer, node);
+ delta.tv64 = hrtimer_get_expires_tv64(timer);
+ delta = ktime_sub(delta, base->get_time());
+ if (delta.tv64 < mindelta.tv64)
+ mindelta.tv64 = delta.tv64;
}
raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
--
1.8.3.2