* [PATCH resend] clocksource: Defer override invalidation unless clock is unstable
@ 2016-07-26 21:24 Kyle Walker
2016-07-26 21:36 ` John Stultz
0 siblings, 1 reply; 5+ messages in thread
From: Kyle Walker @ 2016-07-26 21:24 UTC (permalink / raw)
To: kwalker, john.stultz, tglx; +Cc: linux-kernel
The clock_select() operation will attempt to use the clocksource override
to apply the desired clocksource when the "clocksource=" boot parameter is
supplied. However, in the event that "clocksource=tsc" is used on a system
where there is a more desireable clocksource available, the boot parameter
fails. This is due to the TSC clocksource being installed unvalidated, but
the override being invalidated during the initial run through
clocksource_done_booting().
To address this condition, the override_name is only invalidated for
unstable clocksources. Otherwise, the override is left intact until after
the watchdog has validated the clocksource as stable/unstable.
Signed-off-by: Kyle Walker <kwalker@redhat.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
---
Notes:
Resend due to no feedback on the initial submit. Thank you in advance!
kernel/time/clocksource.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 56ece14..4c1bb2a 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -600,9 +600,18 @@ static void __clocksource_select(bool skipcur)
*/
if (!(cs->flags & CLOCK_SOURCE_VALID_FOR_HRES) && oneshot) {
/* Override clocksource cannot be used. */
- pr_warn("Override clocksource %s is not HRT compatible - cannot switch while in HRT/NOHZ mode\n",
- cs->name);
- override_name[0] = 0;
+ if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
+ pr_warn("Override clocksource %s is unstable and not HRT compatible - cannot switch while in HRT/NOHZ mode\n",
+ cs->name);
+ override_name[0] = 0;
+ } else {
+ /*
+ * The override cannot be currently verified.
+ * Deferring to let the watchdog check.
+ */
+ pr_info("Override clocksource %s is not currently HRT compatible - deferring\n",
+ cs->name);
+ }
} else
/* Override clocksource can be used. */
best = cs;
--
2.5.5
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH resend] clocksource: Defer override invalidation unless clock is unstable
2016-07-26 21:24 [PATCH resend] clocksource: Defer override invalidation unless clock is unstable Kyle Walker
@ 2016-07-26 21:36 ` John Stultz
2016-07-27 14:29 ` Kyle Walker
[not found] ` <CAEPKNTJqRbV4navfqHx9uT9FFkX2kDRfR3rLEYntZz_u9ziGrw@mail.gmail.com>
0 siblings, 2 replies; 5+ messages in thread
From: John Stultz @ 2016-07-26 21:36 UTC (permalink / raw)
To: Kyle Walker; +Cc: Thomas Gleixner, lkml
Sorry for not getting back to you. This has been in my to-look-at list.
On Tue, Jul 26, 2016 at 2:24 PM, Kyle Walker <kwalker@redhat.com> wrote:
> The clock_select() operation will attempt to use the clocksource override
> to apply the desired clocksource when the "clocksource=" boot parameter is
> supplied. However, in the event that "clocksource=tsc" is used on a system
> where there is a more desireable clocksource available, the boot parameter
> fails. This is due to the TSC clocksource being installed unvalidated, but
> the override being invalidated during the initial run through
> clocksource_done_booting().
I've read this a few times, and I'm not sure I really understand it.
Can you give an example of a "more desirable clocksource" then the
TSC? Especially when the TSC was specified as a boot argument?
>
> To address this condition, the override_name is only invalidated for
> unstable clocksources. Otherwise, the override is left intact until after
> the watchdog has validated the clocksource as stable/unstable.
>
> Signed-off-by: Kyle Walker <kwalker@redhat.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-kernel@vger.kernel.org
> ---
>
> Notes:
> Resend due to no feedback on the initial submit. Thank you in advance!
>
> kernel/time/clocksource.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 56ece14..4c1bb2a 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -600,9 +600,18 @@ static void __clocksource_select(bool skipcur)
> */
> if (!(cs->flags & CLOCK_SOURCE_VALID_FOR_HRES) && oneshot) {
> /* Override clocksource cannot be used. */
> - pr_warn("Override clocksource %s is not HRT compatible - cannot switch while in HRT/NOHZ mode\n",
> - cs->name);
> - override_name[0] = 0;
> + if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
> + pr_warn("Override clocksource %s is unstable and not HRT compatible - cannot switch while in HRT/NOHZ mode\n",
> + cs->name);
> + override_name[0] = 0;
> + } else {
> + /*
> + * The override cannot be currently verified.
> + * Deferring to let the watchdog check.
> + */
> + pr_info("Override clocksource %s is not currently HRT compatible - deferring\n",
> + cs->name);
> + }
The logic here is confusing as well. So.. if the override is not HRT
compatible, we check if its stable or not? Once we're in HRT there's
not much likelyhood of us going into non HRT mode. I'm not sure what
the stability has to do with it here.
Sorry, could you explain the case you're running into in some further detail?
thanks
-john
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH resend] clocksource: Defer override invalidation unless clock is unstable
2016-07-26 21:36 ` John Stultz
@ 2016-07-27 14:29 ` Kyle Walker
2016-08-05 18:38 ` Kyle Walker
[not found] ` <CAEPKNTJqRbV4navfqHx9uT9FFkX2kDRfR3rLEYntZz_u9ziGrw@mail.gmail.com>
1 sibling, 1 reply; 5+ messages in thread
From: Kyle Walker @ 2016-07-27 14:29 UTC (permalink / raw)
To: John Stultz; +Cc: Thomas Gleixner, lkml
I'm so sorry for the duplicate, gmail managed to sneak in some HTML.
Resending due to the mailing list correctly blocking the initial send.
On Tue, Jul 26, 2016 at 5:36 PM, John Stultz <john.stultz@linaro.org> wrote:
> Sorry for not getting back to you. This has been in my to-look-at list.
No problem at all. Thanks for taking a look!
> On Tue, Jul 26, 2016 at 2:24 PM, Kyle Walker <kwalker@redhat.com> wrote:
>> The clock_select() operation will attempt to use the clocksource override
>> to apply the desired clocksource when the "clocksource=" boot parameter is
>> supplied. However, in the event that "clocksource=tsc" is used on a system
>> where there is a more desireable clocksource available, the boot parameter
>> fails. This is due to the TSC clocksource being installed unvalidated, but
>> the override being invalidated during the initial run through
>> clocksource_done_booting().
>
> I've read this a few times, and I'm not sure I really understand it.
>
> Can you give an example of a "more desirable clocksource" then the
> TSC? Especially when the TSC was specified as a boot argument?
>
I apologize for the confusion. By "more desireable", I mean that there is
another clocksource that the system is wanting to use by default. For
example, on Xen platforms, the "xen" clocksource is the "best" as
determined by clocksource_find_best(), being at the top of the list.
crash> list -s clocksource.name,rating clocksource.list -H clocksource_list
ffffffff81c0cb00
name = 0xffffffff817ad585 "xen"
rating = 400
ffffffff81a98540
name = 0xffffffff817c2873 "tsc"
rating = 300
ffffffff81aa6580
name = 0xffffffff817b118a "hpet"
rating = 250
ffffffff81b22b00
name = 0xffffffff817c02b7 "acpi_pm"
rating = 120
ffffffff81ab48c0
name = 0xffffffff817c05c6 "jiffies"
rating = 1
In that scenario, the xen clocksource would be used if no override was
specified.
>
> The logic here is confusing as well. So.. if the override is not HRT
> compatible, we check if its stable or not? Once we're in HRT there's
> not much likelyhood of us going into non HRT mode. I'm not sure what
> the stability has to do with it here.
>
> Sorry, could you explain the case you're running into in some further detail?
The issue I'm running into is that the override is not HRT compatible yet.
Though it will be later in the boot process, unless the clocksource watchdog
marks the clocksource as unstable.
The issue with the current implementation is that the override_name value is
disabled when the tsc is first checked, before the watchdog has a chance to
check it and mark it stable or unstable.
Without patch:
$ dmesg | grep -e clocksource
<snip>
clocksource: refined-jiffies: <snip>
Kernel command line: <snip> clocksource=tsc
clocksource: hpet: <snip>
clocksource: xen: <snip>
clocksource: jiffies: <snip>
clocksource: Switched to clocksource xen
clocksource: acpi_pm: <snip>
tsc: Refined TSC clocksource calibration: 2394.399 MHz
clocksource: tsc: <snip>
clocksource: Override clocksource tsc is not HRT compatible - <snip>
With patch:
$ dmesg | grep -e clocksource
<snip>
clocksource: refined-jiffies:<snip>
Kernel command line: <snip> clocksource=tsc
clocksource: hpet: <snip>
clocksource: xen: <snip>
clocksource: jiffies: <snip>
clocksource: Switched to clocksource xen
clocksource: acpi_pm: <snip>
tsc: Refined TSC clocksource calibration: 2394.461 MHz
clocksource: tsc: <snip>
clocksource: Override clocksource tsc is not currently HRT compatible
- deferring
clocksource: Switched to clocksource tsc
Please let me know if there is any further clarification needed. Have a good
one!
--
Kyle Walker
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH resend] clocksource: Defer override invalidation unless clock is unstable
2016-07-27 14:29 ` Kyle Walker
@ 2016-08-05 18:38 ` Kyle Walker
0 siblings, 0 replies; 5+ messages in thread
From: Kyle Walker @ 2016-08-05 18:38 UTC (permalink / raw)
To: John Stultz; +Cc: Thomas Gleixner, lkml
Good evening John,
On Wed, Jul 27, 2016 at 10:29 AM, Kyle Walker <kwalker@redhat.com> wrote:
> The issue I'm running into is that the override is not HRT compatible yet.
> Though it will be later in the boot process, unless the clocksource watchdog
> marks the clocksource as unstable.
>
> The issue with the current implementation is that the override_name value is
> disabled when the tsc is first checked, before the watchdog has a chance to
> check it and mark it stable or unstable.
>
> Without patch:
> $ dmesg | grep -e clocksource
> <snip>
> clocksource: refined-jiffies: <snip>
> Kernel command line: <snip> clocksource=tsc
> clocksource: hpet: <snip>
> clocksource: xen: <snip>
> clocksource: jiffies: <snip>
> clocksource: Switched to clocksource xen
> clocksource: acpi_pm: <snip>
> tsc: Refined TSC clocksource calibration: 2394.399 MHz
>
> clocksource: tsc: <snip>
> clocksource: Override clocksource tsc is not HRT compatible - <snip>
>
>
> With patch:
> $ dmesg | grep -e clocksource
> <snip>
> clocksource: refined-jiffies:<snip>
> Kernel command line: <snip> clocksource=tsc
> clocksource: hpet: <snip>
> clocksource: xen: <snip>
>
> clocksource: jiffies: <snip>
> clocksource: Switched to clocksource xen
> clocksource: acpi_pm: <snip>
> tsc: Refined TSC clocksource calibration: 2394.461 MHz
> clocksource: tsc: <snip>
> clocksource: Override clocksource tsc is not currently HRT compatible
> - deferring
> clocksource: Switched to clocksource tsc
>
Is there anything else needed from my end? Please let me know if there is
any further information or clarification I can provide.
Have a great evening!
--
Kyle Walker
^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAEPKNTJqRbV4navfqHx9uT9FFkX2kDRfR3rLEYntZz_u9ziGrw@mail.gmail.com>]
* Re: [PATCH resend] clocksource: Defer override invalidation unless clock is unstable
[not found] ` <CAEPKNTJqRbV4navfqHx9uT9FFkX2kDRfR3rLEYntZz_u9ziGrw@mail.gmail.com>
@ 2016-08-05 19:04 ` John Stultz
0 siblings, 0 replies; 5+ messages in thread
From: John Stultz @ 2016-08-05 19:04 UTC (permalink / raw)
To: Kyle Walker; +Cc: Thomas Gleixner, lkml, Martin Schwidefsky
On Wed, Jul 27, 2016 at 6:50 AM, Kyle Walker <kwalker@redhat.com> wrote:
> On Tue, Jul 26, 2016 at 5:36 PM, John Stultz <john.stultz@linaro.org> wrote:
>> The logic here is confusing as well. So.. if the override is not HRT
>> compatible, we check if its stable or not? Once we're in HRT there's
>> not much likelyhood of us going into non HRT mode. I'm not sure what
>> the stability has to do with it here.
>>
>> Sorry, could you explain the case you're running into in some further
>> detail?
>
> The issue I'm running into is that the override is not HRT compatible yet.
> Though it will be later in the boot process, unless the clocksource watchdog
> marks the clocksource as unstable.
>
> The issue with the current implementation is that the override_name value is
> disabled when the tsc is first checked, before the watchdog has a chance to
> check it and mark it stable or unstable.
Hrm. Ok. I've missed that the setting of a clocksource to being
VALID_FOR_HRES happens by the watchdog and was thinking it was more
like the IS_CONTINUOUS flag.
So with that detail made clear, the patch makes more sense. I'd
definitely clear up the change log to explain that detail:
"Clocksources don't get the VALID_FOR_HRES flag until they have been
checked by a watchdog. However, when using an override, the
clocksource_select logic will clear the override value if the
clocksources is not marked VALID_FOR_HRES on that check. When using
the boot arguments clocksource=<foo>, this selection can run before
the watchdog, and can cause the override to be incorrectly cleared."
Sorry for the slow response, keeping up with the merge window the last
two weeks has been a little crazy.
thanks
-john
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-05 19:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-26 21:24 [PATCH resend] clocksource: Defer override invalidation unless clock is unstable Kyle Walker
2016-07-26 21:36 ` John Stultz
2016-07-27 14:29 ` Kyle Walker
2016-08-05 18:38 ` Kyle Walker
[not found] ` <CAEPKNTJqRbV4navfqHx9uT9FFkX2kDRfR3rLEYntZz_u9ziGrw@mail.gmail.com>
2016-08-05 19:04 ` John Stultz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®