Re: [PATCH] Sanity check sysfs clocksource changes

Thomas Gleixner
2009-05-05T06:49:23Z

On Mon, 4 May 2009, john stultz wrote:

> On Fri, 2009-05-01 at 13:10 -0700, Andrew Morton wrote:
> > What's the status of
> > clocksource-sanity-check-sysfs-clocksource-changes.patch, btw? I have
> > it marked as "still an RFC", but it's been sitting here since January?
>
> Thanks for reminding me! I lost track of that one. Still waiting on
> feedback from Thomas.
>
> Thomas? What do you think? Original patch below, applies with fuzz and
> builds.
>
> -john
>
>
> Hey all,
> Thomas, Andrew and Ingo pointed out that we don't have any safety
> checks in the clocksource sysfs entries to make sure sysadmins don't try
> to change the clocksource to a non high-res timer capable clocksource
> (such as jiffies) when high-res timers (HRT) is enabled. Doing so will
> likely hang a system.
>
> This patch tries to correct this by filtering non HRT clocksources from
> available_clocksources and not accepting non HRT clocksources with HRT
> is is enabled.
>
> This has been lightly tested, and seems to work, but there may be some
> drawbacks.
>
> One issue I realized was that when TSCs disqualified, they are marked as
> not CLOCK_SOURCE_VALID_FOR_HRES. This means on boxes with unsycned TSCs,
> the only available clocksource may be the slower acpi_pm and the user
> will not be able to override it with the TSC as is currently possible.
> (even if that seems ill-advised).
>
> So Thomas, what do you think? Should we just use
> CLOCK_SOURCE_IS_CONTINUOUS flag or is CLOCK_SOURCE_VALID_FOR_HRES really
> what we want?

Hmm. If we switched to highres and discarded TSC already then we
should not go back to it.

> Signed-off-by: John Stultz <johnstul@us.ibm.com>
>
>
> diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
> index bd37078..05f67f0 100644
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -305,7 +305,7 @@ static inline int hrtimer_is_hres_active(struct hrtimer *timer)
>
> extern ktime_t ktime_get(void);
> extern ktime_t ktime_get_real(void);
> -
> +extern int hrtimer_hres_active(void);
>
> DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
>
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index 2dc30c5..fa4abdc 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -476,7 +476,7 @@ static inline int hrtimer_is_hres_enabled(void)
> /*
> * Is the high resolution mode active ?
> */
> -static inline int hrtimer_hres_active(void)
> +int hrtimer_hres_active(void)
> {
> return __get_cpu_var(hrtimer_bases).hres_active;
> }
> @@ -689,7 +689,7 @@ static int hrtimer_switch_to_hres(void)
>
> #else
>
> -static inline int hrtimer_hres_active(void) { return 0; }
> +int hrtimer_hres_active(void) { return 0; }

We want to keep that static inline and move it to hrtimer.h to allow
gcc to optimize stuff out.

Thanks,

tglx