tick: bugfixes
cover tick: bugfixes
Hi Thomas,
As suggested by you (https://lkml.org/lkml/2014/4/14/797), this is the first lot of changes I have. These are all potential bug fixes (Sorry if I haven't read the most obvious code correctly at some place :) ).
Patch 2/5 isn't a bug fix but was required as a dependency for 3/5.
Some discussions already happened for 5/5 here:
https://lkml.org/lkml/2014/4/9/243
https://lkml.org/lkml/2014/4/9/346
I have tried to mark stable release wherever possible.
Viresh Kumar (5):
tick-common: fix wrong check in tick_check_replacement()
tick-common: don't check tick_oneshot_mode_active() from
tick_check_preferred()
tick-common: do additional checks in tick_check_preferred()
tick-sched: don't call update_wall_time() when delta is lesser than
tick_period
tick-sched: replace tick_nohz_active with tick_nohz_enabled in
tick_nohz_switch_to_nohz()
kernel/time/tick-common.c | 29 +++++++++++++++++++---------- kernel/time/tick-sched.c | 34 ++++++++++++++++++---------------- 2 files changed, 37 insertions(+), 26 deletions(-)
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH RESEND 2/5 tick-common: don't check tick_oneshot_mode_active() from tick_check_preferred()
If 'curdev' passed to tick_check_preferred() is the current clock_event_device then these two checks look exactly same, because td->mode is set to TICKDEV_MODE_ONESHOT only when the event device has ONESHOT feature.
if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; if (tick_oneshot_mode_active()) return false;
Now left the case where 'curdev' is not the current clock_event_device. This can happen from the sequence started from clockevents_replace(). Here we are trying to find the best possible device that we should choose. And so even in this case we don't need the above check as we aren't really worried about the current device.
So, the second check can be removed.
--- kernel/time/tick-common.c | 2 -- 1 file changed, 2 deletions(-)
kernel/time/tick-common.c +0 -2
@@ -256,8 +256,6 @@ static bool tick_check_preferred(struct clock_event_device *curdev,if (!(newdev->features &CLOCK_EVT_FEAT_ONESHOT )) {if (curdev && (curdev->features &CLOCK_EVT_FEAT_ONESHOT ))return false; -if (tick_oneshot_mode_active ()) -return false; }/*
Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH V1 Resend 2/5] tick-common: don't check tick_oneshot_mode_active() from tick_check_preferred()
Quoted 10 lines · Viresh Kumar
If 'curdev' passed to
tick_check_preferred()is the currentclock_event_devicethen these two checks look exactly same, becausetd->modeis set toTICKDEV_MODE_ONESHOTonly when the event device has ONESHOT feature.if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; if (tick_oneshot_mode_active()) return false;
Quoted 5 lines
Now left the case where 'curdev' is not the current
clock_event_device. This can happen from the sequence started fromclockevents_replace(). Here we are trying to find the best possible device that we should choose. And so even in this case we don't need the above check as we aren't really worried about the current device.
Wrong. If curdev is NULL, you might select a device w/o ONESHOT if the system is in oneshot mode. Go figure.
Thanks,
tglx
Re: [PATCH V1 Resend 2/5] tick-common: don't check tick_oneshot_mode_active() from tick_check_preferred()
Quoted 12 lines · Thomas Gleixner
Quoted 10 lines · Viresh Kumar
If 'curdev' passed to
tick_check_preferred()is the currentclock_event_devicethen these two checks look exactly same, becausetd->modeis set toTICKDEV_MODE_ONESHOTonly when the event device has ONESHOT feature.if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; if (tick_oneshot_mode_active()) return false;
Quoted 8 lines
Quoted 5 lines
Now left the case where 'curdev' is not the current
clock_event_device. This can happen from the sequence started fromclockevents_replace(). Here we are trying to find the best possible device that we should choose. And so even in this case we don't need the above check as we aren't really worried about the current device.Wrong. If curdev is NULL, you might select a device w/o ONESHOT if the system is in oneshot mode. Go figure.
Okay, so the logs must have another case where curdev is NULL. But codewise we are already taking care of that here:
return !curdev ||
newdev->rating > curdev->rating ||
!cpumask_equal(curdev->cpumask, newdev->cpumask);And so this patch wouldn't harm. And this is preserved in the next patch (3/5) as well, which adds checks for other cases as well.
PATCH RESEND 3/5 tick-common: do additional checks in tick_check_preferred()
In reply to Viresh KumarWe return false from tick_check_preferred() if newdev doesn't have ONESHOT feature but curdev has, but we don't return true when newdev has ONESHOT and curdev doesn't. Instead we go on, check ratings and other things in that case.
This patch tries to fix this by rewriting some portion of this code and adds sufficient comments to make logic clear.
--- kernel/time/tick-common.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
kernel/time/tick-common.c +18 -7
@@ -252,18 +252,29 @@ static bool tick_check_percpu(struct clock_event_device *curdev,static bool tick_check_preferred (struct clock_event_device *curdev,struct clock_event_device *newdev) { -/* Prefer oneshot capable device */ -if (!(newdev->features &CLOCK_EVT_FEAT_ONESHOT )) { -if (curdev && (curdev->features &CLOCK_EVT_FEAT_ONESHOT )) -return false; - } +if (!curdev) +return true; + +/* +* Prefer oneshot capable device. +* return values based on if ONESHOT is available or not: +* +* curdev newdev operation +* 0 0 check priority +* 0 1 return true +* 1 0 return false +* 1 1 check priority +*/ + +if ((newdev->features &CLOCK_EVT_FEAT_ONESHOT ) ^ + (curdev->features &CLOCK_EVT_FEAT_ONESHOT )) +return newdev->features &CLOCK_EVT_FEAT_ONESHOT ;/* * Use the higher rated one, but prefer a CPU local device with a lower * rating than a non-CPU local device */ -return !curdev || - newdev->rating > curdev->rating || +return newdev->rating > curdev->rating || !cpumask_equal (curdev->cpumask, newdev->cpumask); }
Signature · 1 line
1.7.12.rc2.18.g61b472e
A message that is not in the archive (sent off-list or not yet received): 486a02efe0246635aaba786e24b42d316438bf3b.1397537987.git.viresh.kumar@linaro.org
Re: [PATCH V1 Resend 1/5] tick-common: fix wrong check in tick_check_replacement()
B1;3202;0c
Quoted 8 lines · Viresh Kumar
tick_check_replacement()returns if a replacement ofclock_event_deviceis possible or not. It does this as the first check:if (tick_check_percpu(curdev, newdev, smp_processor_id())) return false;This looks wrong as we are returning false when
tick_check_percpu()returned true. Probably Thomas forgot '!' here in his commit:03e13cf5e?
Come on. You can do better changelogs.
"This looks wrong" is definitely not a good description of the problem.
Either you know WHY it is wrong, then you say so. If not, then you can send an RFC.
I fixed the changelog up this time.
Thanks,
tglx
Re: [PATCH V1 Resend 1/5] tick-common: fix wrong check in tick_check_replacement()
Quoted 1 line · Thomas Gleixner
B1;3202;0c
What does this mean ??
Quoted 5 lines · Viresh Kumar
Quoted 4 lines
tick_check_replacement()returns if a replacement ofclock_event_deviceis possible or not. It does this as the first check:
Quoted 7 lines
Quoted 5 lines
if (
tick_check_percpu(curdev, newdev, smp_processor_id()))
return false;This looks wrong as we are returning false when
tick_check_percpu()returned true. Probably Thomas forgot '!' here in his commit:03e13cf5e?Come on. You can do better changelogs.
:(
Quoted 7 lines
"This looks wrong" is definitely not a good description of the problem.
Either you know WHY it is wrong, then you say so. If not, then you can send an RFC.
I fixed the changelog up this time.
Thanks, will take care of such stuff in future.
tip-bot for Viresh Kumar
merge bot[tip:timers/urgent] tick-common: Fix wrong check in tick_check_replacement()
In reply to a message not in the archivetick-common: Fix wrong check in tick_check_replacement()
Commit-ID: 521c42990e9d561ed5ed9f501f07639d0512b3c9 Gitweb: http://git.kernel.org/tip/521c42990e9d561ed5ed9f501f07639d0512b3c9 Author: Viresh Kumar <[email protected]> AuthorDate: Tue, 15 Apr 2014 10:54:37 +0530 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 15 Apr 2014 20:26:44 +0200
tick-common: Fix wrong check in tick_check_replacement()
tick_check_replacement() returns if a replacement of clock_event_device is possible or not. It does this as the first check:
if (tick_check_percpu(curdev, newdev, smp_processor_id())) return false;
Thats wrong. tick_check_percpu() returns true when the device is useable. Check for false instead.
[ tglx: Massaged changelog ]
--- kernel/time/tick-common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/tick-common.c +1 -1
@@ -276,7 +276,7 @@ static bool tick_check_preferred(struct clock_event_device *curdev,bool tick_check_replacement (struct clock_event_device *curdev,struct clock_event_device *newdev) { -if (tick_check_percpu (curdev, newdev,smp_processor_id ())) +if (!tick_check_percpu (curdev, newdev,smp_processor_id ()))return false;return tick_check_preferred (curdev, newdev);
A message that is not in the archive (sent off-list or not yet received): 80afb18a494b0bd9710975bcc4de134ae323c74f.1397537987.git.viresh.kumar@linaro.org
Re: [PATCH V1 Resend 4/5] tick-sched: don't call update_wall_time() when delta is lesser than tick_period
Quoted 9 lines · Viresh Kumar
In
tick_do_update_jiffies64()we are processing ticks only if delta is greater thantick_period. This is what we are supposed to do here and it broke a bit with this patch:commit
47a1b796306356f358e515149d86baf0cc6bf007Author: John Stultz <[email protected]> Date: Thu Dec 12 13:10:55 2013 -0800tick/timekeeping: Call
update_wall_timeoutside the jiffies lock
Please look how I massaged the change log. There is no point in copying the whole gunk.
Quoted 3 lines
With above patch, we might end up calling
update_wall_time()even if delta is found to be smaller thattick_period. Fix this by reversing the check and returning early.
Well.
Quoted 6 lines
Cc <[email protected]> # v3.14+Cc John Stultz <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- kernel/time/tick-sched.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
That's not how we do bug fixes if they can be done with 3 lines of change. See the commit.
Thanks,
tglx
Re: [PATCH V1 Resend 4/5] tick-sched: don't call update_wall_time() when delta is lesser than tick_period
Quoted 6 lines · Thomas Gleixner
Quoted 4 lines · Viresh Kumar
In
tick_do_update_jiffies64()we are processing ticks only if delta is greater thantick_period. This is what we are supposed to do here and it broke a bit with this patch:
Quoted 8 lines
Quoted 5 lines
commit
47a1b796306356f358e515149d86baf0cc6bf007Author: John Stultz <[email protected]> Date: Thu Dec 12 13:10:55 2013 -0800tick/timekeeping: Call
update_wall_timeoutside the jiffies lockPlease look how I massaged the change log. There is no point in copying the whole gunk.
I see.. Nice.
Quoted 13 lines
Quoted 3 lines
With above patch, we might end up calling
update_wall_time()even if delta is found to be smaller thattick_period. Fix this by reversing the check and returning early.Well.
Quoted 6 lines
Cc <[email protected]> # v3.14+Cc John Stultz <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- kernel/time/tick-sched.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
Quoted 2 lines
That's not how we do bug fixes if they can be done with 3 lines of change. See the commit.
I tried that initially but with these changes as well (which must be done now ??), which probably makes it more clear ?:
kernel/time/tick-sched.c +3 -3
@@ -84,12 +84,12 @@ static void tick_do_update_jiffies64(ktime_t now)/* Keep the tick_next_period variable up to date */ tick_next_period =ktime_add (last_jiffies_update, tick_period); + +write_sequnlock (&jiffies_lock); +update_wall_time (); }else {write_sequnlock (&jiffies_lock); -return ; } -write_sequnlock (&jiffies_lock); -update_wall_time (); }/*
tip-bot for Viresh Kumar
merge bot[tip:timers/urgent] tick-sched: Don't call update_wall_time() when delta is lesser than tick_period
In reply to a message not in the archivetick-sched: Don't call update_wall_time() when delta is lesser than tick_period
Commit-ID: 03e6bdc5c4d0fc166bfd5d3cf749a5a0c1b5b1bd Gitweb: http://git.kernel.org/tip/03e6bdc5c4d0fc166bfd5d3cf749a5a0c1b5b1bd Author: Viresh Kumar <[email protected]> AuthorDate: Tue, 15 Apr 2014 10:54:40 +0530 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 15 Apr 2014 20:26:45 +0200
tick-sched: Don't call update_wall_time() when delta is lesser than tick_period
In tick_do_update_jiffies64() we are processing ticks only if delta is greater than tick_period. This is what we are supposed to do here and it broke a bit with this patch:
commit 47a1b796 (tick/timekeeping: Call update_wall_time outside the jiffies lock)
With above patch, we might end up calling update_wall_time() even if delta is found to be smaller that tick_period. Fix this by returning when the delta is less than tick period.
[ tglx: Made it a 3 liner and massaged changelog ]
--- kernel/time/tick-sched.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/time/tick-sched.c +3 -0
@@ -84,6 +84,9 @@ static void tick_do_update_jiffies64(ktime_t now)/* Keep the tick_next_period variable up to date */ tick_next_period =ktime_add (last_jiffies_update, tick_period); + }else { +write_sequnlock (&jiffies_lock); +return ; }write_sequnlock (&jiffies_lock);update_wall_time ();