Re: [PATCH] time/cpuidle:Fixup fallout from hrtimer broadcast mode inclusion

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

Hi Thomas,

On 02/07/2014 11:27 PM, Thomas Gleixner wrote:
> On Fri, 7 Feb 2014, Preeti U Murthy wrote:
> 
>> The broadcast timer registration has to be done only when
>> GENERIC_CLOCKEVENTS_BROADCAST and TICK_ONESHOT config options are enabled.
> 
> Then we should compile that file only when those options are
> enabled. Where is the point to compile that code w/o the registration
> function?

Hmm of course. The delta patch is at the end.

Another concern I have is with regard to the periodic mode of broadcast .
We currently do not support the hrtimer mode of broadcast in periodic mode.
The BROADCAST_ON/OFF calls which take effect in periodic mode has not yet
been modified by this patchset to disable one CPU from going into deep idle,
since we expect the deep idle states to never be chosen by the cpuidle
governor in this mode. Do you think we should bother to modify this piece
of code at all?

On the same note, my understanding is that BROADCAST_ON/OFF takes effect
only in periodic mode, in oneshot mode it is a nop. But why do we expect
the CPUs to avail broadcast in periodic mode when they are not supposed
to be entering deep idle states? Am I missing something here? IOW what
is the point of periodic mode of broadcast? Is it for malfunctioning
local clock devices?

The delta patch below for fixing the compile time errors. This is based on
tip/timers/core branch.


time/cpuidle:Fixup fallout from hrtimer broadcast mode inclusion

From: Preeti U Murthy <preeti@linux.vnet.ibm.com>

The hrtimer mode of broadcast is supported only when
GENERIC_CLOCKEVENTS_BROADCAST and TICK_ONESHOT config options
are enabled. Hence compile in the functions for hrtimer mode
of broadcast only when these options are selected.
Also fix max_delta_ticks value for the pseudo clock device.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
---
 include/linux/clockchips.h           |    1 +
 kernel/time/Makefile                 |    5 ++++-
 kernel/time/tick-broadcast-hrtimer.c |    2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 20a7183..2e4cb67 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -207,6 +207,7 @@ static inline void clockevents_resume(void) {}
 
 static inline int clockevents_notify(unsigned long reason, void *arg) { return 0; }
 static inline int tick_check_broadcast_expired(void) { return 0; }
+static inline void tick_setup_hrtimer_broadcast(void) {};
 
 #endif
 
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index 06151ef..57a413f 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -3,7 +3,10 @@ obj-y += timeconv.o posix-clock.o alarmtimer.o
 
 obj-$(CONFIG_GENERIC_CLOCKEVENTS_BUILD)		+= clockevents.o
 obj-$(CONFIG_GENERIC_CLOCKEVENTS)		+= tick-common.o
-obj-$(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST)	+= tick-broadcast.o tick-broadcast-hrtimer.o
+ifeq ($(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST),y)
+ obj-y						+= tick-broadcast.o
+ obj-$(CONFIG_TICK_ONESHOT)			+= tick-broadcast-hrtimer.o
+endif
 obj-$(CONFIG_GENERIC_SCHED_CLOCK)		+= sched_clock.o
 obj-$(CONFIG_TICK_ONESHOT)			+= tick-oneshot.o
 obj-$(CONFIG_TICK_ONESHOT)			+= tick-sched.o
diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
index 9242527..eb682d5 100644
--- a/kernel/time/tick-broadcast-hrtimer.c
+++ b/kernel/time/tick-broadcast-hrtimer.c
@@ -82,7 +82,7 @@ static struct clock_event_device ce_broadcast_hrtimer = {
 	.min_delta_ns		= 1,
 	.max_delta_ns		= KTIME_MAX,
 	.min_delta_ticks	= 1,
-	.max_delta_ticks	= KTIME_MAX,
+	.max_delta_ticks	= ULONG_MAX,
 	.mult			= 1,
 	.shift			= 0,
 	.cpumask		= cpu_all_mask,



Thanks

Regards
Preeti U Murthy

> 
>> Also fix max_delta_ticks value for the pseudo clock device.
>>
>> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
>> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> ---
>>
>>  kernel/time/tick-broadcast-hrtimer.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
>> index 5591aaa..bc383ac 100644
>> --- a/kernel/time/tick-broadcast-hrtimer.c
>> +++ b/kernel/time/tick-broadcast-hrtimer.c
>> @@ -81,7 +81,7 @@ static struct clock_event_device ce_broadcast_hrtimer = {
>>  	.min_delta_ns		= 1,
>>  	.max_delta_ns		= KTIME_MAX,
>>  	.min_delta_ticks	= 1,
>> -	.max_delta_ticks	= KTIME_MAX,
>> +	.max_delta_ticks	= ULONG_MAX,
>>  	.mult			= 1,
>>  	.shift			= 0,
>>  	.cpumask		= cpu_all_mask,
>> @@ -102,9 +102,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
>>  	return HRTIMER_RESTART;
>>  }
>>  
>> +#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
>>  void tick_setup_hrtimer_broadcast(void)
>>  {
>>  	hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>>  	bctimer.function = bc_handler;
>>  	clockevents_register_device(&ce_broadcast_hrtimer);
>>  }
>> +#endif
>>
>>
>