At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have per cpu-base 'active_bases' field which is updated on addition/removal of hrtimers.
This patch creates for_each_active_base() which uses this existing infrastructure to only iterate over active bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Hi,
I am trying to upstream useful parts of my long cleanup series which never got attention due to my bad approach. Please see if it looks fine, I will send it upstream then.
It doesn't have anything to do with ONESHOT_STOPPED series :).
kernel/hrtimer.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 3ab2899..c751322 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -109,6 +109,19 @@ static inline int hrtimer_clockid_to_base(clockid_t clock_id)
/* + * for_each_active_base: iterate over all active clock bases + * @_index: 'int' variable for internal purpose + * @_base: holds pointer to a active clock base + * @_cpu_base: cpu base to iterate on + * @_active_bases: 'unsigned int' variable for internal purpose + */ +#define for_each_active_base(_index, _base, _cpu_base, _active_bases) \ + for ((_active_bases) = (_cpu_base)->active_bases; \ + (_index) = ffs(_active_bases), \ + (_base) = (_cpu_base)->clock_base + (_index) - 1, (_index); \ + (_active_bases) &= ~(1 << ((_index) - 1))) + +/* * Get the coarse grained time at the softirq based on xtime and * wall_to_monotonic. */
While removing hrtimers, in __remove_hrtimer(), we may call hrtimer_force_reprogram() to reprogram clockevent device. And right after calling hrtimer_force_reprogram() we update 'cpu_base->active_bases' to reflect which clock-bases have pending timers.
hrtimer_force_reprogram() iterates over all clock-bases, of a particular cpu-base, to find hrtimer that will expire next.
Next commit will update hrtimer_force_reprogram() to use newly created for_each_active_base() routine which is dependent on the updated value of ->active_bases.
If timer being removed was the last one queued, then we will try to find next expiry for an empty clock-base as ->active_bases isn't updated yet. timerqueue_getnext() will return NULL and we will get 'NULL pointer dereference' issue in hrtimer_force_reprogram().
To fix this, update ->active_bases before calling hrtimer_force_reprogram().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/hrtimer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index c751322..3aae27a 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -938,6 +938,9 @@ static void __remove_hrtimer(struct hrtimer *timer,
next_timer = timerqueue_getnext(&base->active); timerqueue_del(&base->active, &timer->node); + if (!timerqueue_getnext(&base->active)) + base->cpu_base->active_bases &= ~(1 << base->index); + if (&timer->node == next_timer) { #ifdef CONFIG_HIGH_RES_TIMERS /* Reprogram the clock event device. if enabled */ @@ -951,8 +954,6 @@ static void __remove_hrtimer(struct hrtimer *timer, } #endif } - if (!timerqueue_getnext(&base->active)) - base->cpu_base->active_bases &= ~(1 << base->index); out: timer->state = newstate; }
On 06/09/2014 09:38 PM, Viresh Kumar wrote:
While removing hrtimers, in __remove_hrtimer(), we may call hrtimer_force_reprogram() to reprogram clockevent device. And right after calling hrtimer_force_reprogram() we update 'cpu_base->active_bases' to reflect which clock-bases have pending timers.
hrtimer_force_reprogram() iterates over all clock-bases, of a particular cpu-base, to find hrtimer that will expire next.
Next commit will update hrtimer_force_reprogram() to use newly created for_each_active_base() routine which is dependent on the updated value of ->active_bases.
If timer being removed was the last one queued, then we will try to find next expiry for an empty clock-base as ->active_bases isn't updated yet. timerqueue_getnext() will return NULL and we will get 'NULL pointer dereference' issue in hrtimer_force_reprogram().
To fix this, update ->active_bases before calling hrtimer_force_reprogram().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/hrtimer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index c751322..3aae27a 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -938,6 +938,9 @@ static void __remove_hrtimer(struct hrtimer *timer,
next_timer = timerqueue_getnext(&base->active); timerqueue_del(&base->active, &timer->node);
- if (!timerqueue_getnext(&base->active))
base->cpu_base->active_bases &= ~(1 << base->index);- if (&timer->node == next_timer) {
#ifdef CONFIG_HIGH_RES_TIMERS /* Reprogram the clock event device. if enabled */ @@ -951,8 +954,6 @@ static void __remove_hrtimer(struct hrtimer *timer, } #endif }
- if (!timerqueue_getnext(&base->active))
base->cpu_base->active_bases &= ~(1 << base->index);out: timer->state = newstate; }
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have for_each_active_base() now, which can be used at these places to generate more efficient code.
This patch updates all possible sites which were iterating over all possible clock-bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/hrtimer.c | 56 ++++++++++++++++++++++---------------------------------- 1 file changed, 22 insertions(+), 34 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 3aae27a..4dec883 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -556,20 +556,16 @@ static void hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal) { int i; - struct hrtimer_clock_base *base = cpu_base->clock_base; + struct hrtimer_clock_base *base; + struct hrtimer *timer; ktime_t expires, expires_next; + unsigned int active_bases;
expires_next.tv64 = KTIME_MAX;
- 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; - timer = container_of(next, struct hrtimer, node); - + for_each_active_base(i, base, cpu_base, active_bases) { + timer = container_of(timerqueue_getnext(&base->active), + struct hrtimer, node); expires = ktime_sub(hrtimer_get_expires(timer), base->offset); /* * clock_was_set() has changed base->offset so the @@ -1168,23 +1164,19 @@ EXPORT_SYMBOL_GPL(hrtimer_get_remaining); ktime_t hrtimer_get_next_event(void) { struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases); - struct hrtimer_clock_base *base = cpu_base->clock_base; + struct hrtimer_clock_base *base; ktime_t delta, mindelta = { .tv64 = KTIME_MAX }; + struct hrtimer *timer; + unsigned int active_bases; unsigned long flags; int i;
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; - - next = timerqueue_getnext(&base->active); - if (!next) - continue; - - timer = container_of(next, struct hrtimer, node); + for_each_active_base(i, base, cpu_base, active_bases) { + timer = container_of(timerqueue_getnext(&base->active), + struct hrtimer, node); delta.tv64 = hrtimer_get_expires_tv64(timer); delta = ktime_sub(delta, base->get_time()); if (delta.tv64 < mindelta.tv64) @@ -1307,7 +1299,9 @@ static void __run_hrtimer(struct hrtimer *timer, ktime_t *now) void hrtimer_interrupt(struct clock_event_device *dev) { struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases); + struct hrtimer_clock_base *base; ktime_t expires_next, now, entry_time, delta; + unsigned int active_bases; int i, retries = 0;
BUG_ON(!cpu_base->hres_active); @@ -1327,15 +1321,10 @@ retry: */ cpu_base->expires_next.tv64 = KTIME_MAX;
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) { - struct hrtimer_clock_base *base; + for_each_active_base(i, base, cpu_base, active_bases) { struct timerqueue_node *node; ktime_t basenow;
- if (!(cpu_base->active_bases & (1 << i))) - continue; - - base = cpu_base->clock_base + i; basenow = ktime_add(now, base->offset);
while ((node = timerqueue_getnext(&base->active))) { @@ -1506,16 +1495,13 @@ void hrtimer_run_queues(void) struct timerqueue_node *node; struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases); struct hrtimer_clock_base *base; + unsigned int active_bases; int index, gettime = 1;
if (hrtimer_hres_active()) return;
- for (index = 0; index < HRTIMER_MAX_CLOCK_BASES; index++) { - base = &cpu_base->clock_base[index]; - if (!timerqueue_getnext(&base->active)) - continue; - + for_each_active_base(index, base, cpu_base, active_bases) { if (gettime) { hrtimer_get_softirq_time(cpu_base); gettime = 0; @@ -1735,6 +1721,8 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base, static void migrate_hrtimers(int scpu) { struct hrtimer_cpu_base *old_base, *new_base; + struct hrtimer_clock_base *clock_base; + unsigned int active_bases; int i;
BUG_ON(cpu_online(scpu)); @@ -1750,9 +1738,9 @@ static void migrate_hrtimers(int scpu) raw_spin_lock(&new_base->lock); raw_spin_lock_nested(&old_base->lock, SINGLE_DEPTH_NESTING);
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) { - migrate_hrtimer_list(&old_base->clock_base[i], - &new_base->clock_base[i]); + for_each_active_base(i, clock_base, old_base, active_bases) { + migrate_hrtimer_list(clock_base, + &new_base->clock_base[clock_base->index]); }
raw_spin_unlock(&old_base->lock);
On 06/09/2014 09:38 PM, Viresh Kumar wrote:
At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have for_each_active_base() now, which can be used at these places to generate more efficient code.
This patch updates all possible sites which were iterating over all possible clock-bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/hrtimer.c | 56 ++++++++++++++++++++++---------------------------------- 1 file changed, 22 insertions(+), 34 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 3aae27a..4dec883 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -556,20 +556,16 @@ static void hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal) { int i;
- struct hrtimer_clock_base *base = cpu_base->clock_base;
struct hrtimer_clock_base *base;
struct hrtimer *timer; ktime_t expires, expires_next;
unsigned int active_bases;
expires_next.tv64 = KTIME_MAX;
- 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;timer = container_of(next, struct hrtimer, node);
- for_each_active_base(i, base, cpu_base, active_bases) {
timer = container_of(timerqueue_getnext(&base->active),struct hrtimer, node);
- clock_was_set() has changed base->offset so the
@@ -1168,23 +1164,19 @@ EXPORT_SYMBOL_GPL(hrtimer_get_remaining); ktime_t hrtimer_get_next_event(void) { struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
- struct hrtimer_clock_base *base = cpu_base->clock_base;
struct hrtimer_clock_base *base; ktime_t delta, mindelta = { .tv64 = KTIME_MAX };
struct hrtimer *timer;
unsigned int active_bases; unsigned long flags; int i;
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;next = timerqueue_getnext(&base->active);if (!next)continue;timer = container_of(next, struct hrtimer, node);
for_each_active_base(i, base, cpu_base, active_bases) {timer = container_of(timerqueue_getnext(&base->active),struct hrtimer, node); delta.tv64 = hrtimer_get_expires_tv64(timer); delta = ktime_sub(delta, base->get_time()); if (delta.tv64 < mindelta.tv64)@@ -1307,7 +1299,9 @@ static void __run_hrtimer(struct hrtimer *timer, ktime_t *now) void hrtimer_interrupt(struct clock_event_device *dev) { struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
struct hrtimer_clock_base *base; ktime_t expires_next, now, entry_time, delta;
unsigned int active_bases; int i, retries = 0;
BUG_ON(!cpu_base->hres_active);
@@ -1327,15 +1321,10 @@ retry: */ cpu_base->expires_next.tv64 = KTIME_MAX;
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
struct hrtimer_clock_base *base;
- for_each_active_base(i, base, cpu_base, active_bases) { struct timerqueue_node *node; ktime_t basenow;
if (!(cpu_base->active_bases & (1 << i)))continue;base = cpu_base->clock_base + i;basenow = ktime_add(now, base->offset);
while ((node = timerqueue_getnext(&base->active))) {
@@ -1506,16 +1495,13 @@ void hrtimer_run_queues(void) struct timerqueue_node *node; struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases); struct hrtimer_clock_base *base;
unsigned int active_bases; int index, gettime = 1;
if (hrtimer_hres_active()) return;
- for (index = 0; index < HRTIMER_MAX_CLOCK_BASES; index++) {
base = &cpu_base->clock_base[index];if (!timerqueue_getnext(&base->active))continue;
- for_each_active_base(index, base, cpu_base, active_bases) { if (gettime) { hrtimer_get_softirq_time(cpu_base); gettime = 0;
@@ -1735,6 +1721,8 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base, static void migrate_hrtimers(int scpu) { struct hrtimer_cpu_base *old_base, *new_base;
struct hrtimer_clock_base *clock_base;
unsigned int active_bases; int i;
BUG_ON(cpu_online(scpu));
@@ -1750,9 +1738,9 @@ static void migrate_hrtimers(int scpu) raw_spin_lock(&new_base->lock); raw_spin_lock_nested(&old_base->lock, SINGLE_DEPTH_NESTING);
- for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
migrate_hrtimer_list(&old_base->clock_base[i],&new_base->clock_base[i]);
for_each_active_base(i, clock_base, old_base, active_bases) {
migrate_hrtimer_list(clock_base,&new_base->clock_base[clock_base->index]);}
raw_spin_unlock(&old_base->lock);
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
Hi Viresh,
On Mon, Jun 09, 2014 at 09:38:10PM +0530, Viresh Kumar wrote:
At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have for_each_active_base() now, which can be used at these places to generate more efficient code.
This patch updates all possible sites which were iterating over all possible clock-bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Quite often it's good to split patches as we can. But here splitting patch 1 and 3 makes review slightly harder because we need to get back to patch 1 to check the correctness of some parts of patch 3.
What would you think about merging both?
Thanks.
On 16 June 2014 18:09, Frederic Weisbecker fweisbec@gmail.com wrote:
On Mon, Jun 09, 2014 at 09:38:10PM +0530, Viresh Kumar wrote:
At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have for_each_active_base() now, which can be used at these places to generate more efficient code.
This patch updates all possible sites which were iterating over all possible clock-bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Quite often it's good to split patches as we can. But here splitting patch 1 and 3 makes review slightly harder because we need to get back to patch 1 to check the correctness of some parts of patch 3.
What would you think about merging both?
Hmm, I am fine with both as soon as patches goes upstream :) And so you suggest this: 2/3 as 1/2 1/3+3/3 as 2/2
Will do that while sending upstream..
On Mon, Jun 16, 2014 at 08:07:45PM +0530, Viresh Kumar wrote:
On 16 June 2014 18:09, Frederic Weisbecker fweisbec@gmail.com wrote:
On Mon, Jun 09, 2014 at 09:38:10PM +0530, Viresh Kumar wrote:
At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have for_each_active_base() now, which can be used at these places to generate more efficient code.
This patch updates all possible sites which were iterating over all possible clock-bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Quite often it's good to split patches as we can. But here splitting patch 1 and 3 makes review slightly harder because we need to get back to patch 1 to check the correctness of some parts of patch 3.
What would you think about merging both?
Hmm, I am fine with both as soon as patches goes upstream :) And so you suggest this: 2/3 as 1/2 1/3+3/3 as 2/2
Right!
Will do that while sending upstream..
Thanks!
On 06/09/2014 09:38 PM, Viresh Kumar wrote:
At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have per cpu-base 'active_bases' field which is updated on addition/removal of hrtimers.
This patch creates for_each_active_base() which uses this existing infrastructure to only iterate over active bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi,
I am trying to upstream useful parts of my long cleanup series which never got attention due to my bad approach. Please see if it looks fine, I will send it upstream then.
It doesn't have anything to do with ONESHOT_STOPPED series :).
kernel/hrtimer.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 3ab2899..c751322 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -109,6 +109,19 @@ static inline int hrtimer_clockid_to_base(clockid_t clock_id)
/*
- for_each_active_base: iterate over all active clock bases
- @_index: 'int' variable for internal purpose
- @_base: holds pointer to a active clock base
- @_cpu_base: cpu base to iterate on
- @_active_bases: 'unsigned int' variable for internal purpose
- */
+#define for_each_active_base(_index, _base, _cpu_base, _active_bases) \
- for ((_active_bases) = (_cpu_base)->active_bases; \
(_index) = ffs(_active_bases), \(_base) = (_cpu_base)->clock_base + (_index) - 1, (_index); \(_active_bases) &= ~(1 << ((_index) - 1)))
Shouldn't the index = ffs().. and _base = (cpu_base).. come in the third part of the for loop. These two statements must get executed during each loop iteration along with the negation of the appropriate bit of _active_bases right?
Regards Preeti U Murthy
+/*
- Get the coarse grained time at the softirq based on xtime and
- wall_to_monotonic.
*/
On 9 June 2014 22:42, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
+#define for_each_active_base(_index, _base, _cpu_base, _active_bases) \
for ((_active_bases) = (_cpu_base)->active_bases; \(_index) = ffs(_active_bases), \(_base) = (_cpu_base)->clock_base + (_index) - 1, (_index); \(_active_bases) &= ~(1 << ((_index) - 1)))Shouldn't the index = ffs().. and _base = (cpu_base).. come in the third part of the for loop. These two statements must get executed during each loop iteration along with the negation of the appropriate bit of _active_bases right?
_index is used for conditional check of for loop and so must be done in second part only..
_base will be used by the body of for loop and so that must stay as well in the second part. Only the active_bases were required to be cleared after the body has gone through a iteration and so is present in third part.
Hi Viresh,
On 06/09/2014 09:38 PM, Viresh Kumar wrote:
At several instances we iterate over all possible clock-bases for a particular cpu-base. Whereas, we only need to iterate over active bases.
We already have per cpu-base 'active_bases' field which is updated on addition/removal of hrtimers.
This patch creates for_each_active_base() which uses this existing infrastructure to only iterate over active bases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi,
I am trying to upstream useful parts of my long cleanup series which never got attention due to my bad approach. Please see if it looks fine, I will send it upstream then.
It doesn't have anything to do with ONESHOT_STOPPED series :).
kernel/hrtimer.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 3ab2899..c751322 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -109,6 +109,19 @@ static inline int hrtimer_clockid_to_base(clockid_t clock_id)
/*
- for_each_active_base: iterate over all active clock bases
- @_index: 'int' variable for internal purpose
- @_base: holds pointer to a active clock base
- @_cpu_base: cpu base to iterate on
- @_active_bases: 'unsigned int' variable for internal purpose
- */
+#define for_each_active_base(_index, _base, _cpu_base, _active_bases) \
- for ((_active_bases) = (_cpu_base)->active_bases; \
(_index) = ffs(_active_bases), \(_base) = (_cpu_base)->clock_base + (_index) - 1, (_index); \(_active_bases) &= ~(1 << ((_index) - 1)))+/*
- Get the coarse grained time at the softirq based on xtime and
- wall_to_monotonic.
*/
This patch series look good to me.
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
linaro-kernel@lists.linaro.org
-
Frederic Weisbecker -
Preeti U Murthy -
Viresh Kumar