[PATCH] tick: check if broadcast device could really be stopped

[PATCH] tick: check if broadcast device could really be stopped

[Benjamin Gaignard]

If the CPU isn't able to go in states where the clock event will
be stopped we can ignore CLOCK_EVT_FEAT_C3STOP flag.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 kernel/time/tick-broadcast.c |  6 +++---
 kernel/time/tick-common.c    |  4 ++--
 kernel/time/tick-internal.h  | 12 ++++++++++++
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index e51778c312f1..5393778d7703 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -78,7 +78,7 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev,
 {
 	if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) ||
 	    (newdev->features & CLOCK_EVT_FEAT_PERCPU) ||
-	    (newdev->features & CLOCK_EVT_FEAT_C3STOP))
+	    tick_broadcast_could_stop(newdev))
 		return false;
 
 	if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT &&
@@ -188,7 +188,7 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 		 * Clear the broadcast bit for this cpu if the
 		 * device is not power state affected.
 		 */
-		if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
+		if (!tick_broadcast_could_stop(dev))
 			cpumask_clear_cpu(cpu, tick_broadcast_mask);
 		else
 			tick_device_setup_broadcast_func(dev);
@@ -368,7 +368,7 @@ void tick_broadcast_control(enum tick_broadcast_mode mode)
 	/*
 	 * Is the device not affected by the powerstate ?
 	 */
-	if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP))
+	if (!dev || !tick_broadcast_could_stop(dev))
 		goto out;
 
 	if (!tick_device_is_functional(dev))
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 59225b484e4e..a300ee941c56 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -72,7 +72,7 @@ int tick_is_oneshot_available(void)
 
 	if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT))
 		return 0;
-	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
+	if (!tick_broadcast_could_stop(dev))
 		return 1;
 	return tick_broadcast_oneshot_available();
 }
@@ -393,7 +393,7 @@ int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
 
-	if (!(td->evtdev->features & CLOCK_EVT_FEAT_C3STOP))
+	if (!tick_broadcast_could_stop(td->evtdev))
 		return 0;
 
 	return __tick_broadcast_oneshot_control(state);
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 7b2496136729..99aa7b5a8dae 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -2,6 +2,7 @@
 /*
  * tick internal variable and functions used by low/high res code
  */
+#include <linux/cpuidle.h>
 #include <linux/hrtimer.h>
 #include <linux/tick.h>
 
@@ -48,6 +49,17 @@ static inline void clockevent_set_state(struct clock_event_device *dev,
 	dev->state_use_accessors = state;
 }
 
+/**
+ * Return true if the cpu could go in states where the device will be stopped
+ */
+static inline int tick_broadcast_could_stop(struct clock_event_device *dev)
+{
+	struct cpuidle_driver *drv = cpuidle_get_driver();
+
+	return !!((dev->features & CLOCK_EVT_FEAT_C3STOP)
+		  && drv && drv->bctimer);
+}
+
 extern void clockevents_shutdown(struct clock_event_device *dev);
 extern void clockevents_exchange_device(struct clock_event_device *old,
 					struct clock_event_device *new);
-- 
2.15.0

Re: [PATCH] tick: check if broadcast device could really be stopped

[Thomas Gleixner]

On Wed, 9 Oct 2019, Benjamin Gaignard wrote:
> @@ -78,7 +78,7 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev,
>  {
>  	if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) ||
>  	    (newdev->features & CLOCK_EVT_FEAT_PERCPU) ||
> -	    (newdev->features & CLOCK_EVT_FEAT_C3STOP))
> +	    tick_broadcast_could_stop(newdev))

No. This might be called _before_ a cpuidle driver is available and then
when that driver is loaded and goes deep, everything goes south.

Aside of that it definitely breaks everything which does not use the
cpuidle stuff, which includes all machines affected by X86_BUG_AMD_APIC_C1E
and everything which uses the INTEL_IDLE driver.

Pretty much the same problem for all other places you changed.

Thanks,

	tglx

Re: [PATCH] tick: check if broadcast device could really be stopped

[Benjamin GAIGNARD]

On 10/14/19 2:56 PM, Thomas Gleixner wrote:
> On Wed, 9 Oct 2019, Benjamin Gaignard wrote:
>> @@ -78,7 +78,7 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev,
>>   {
>>   	if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) ||
>>   	    (newdev->features & CLOCK_EVT_FEAT_PERCPU) ||
>> -	    (newdev->features & CLOCK_EVT_FEAT_C3STOP))
>> +	    tick_broadcast_could_stop(newdev))
> No. This might be called _before_ a cpuidle driver is available and then
> when that driver is loaded and goes deep, everything goes south.

What could be the solution to let know to tick broadcast framework that 
this device

will not be stopped (because CPU won't go in idle) ?

I have tried to put "always-on" property on DT but it was a NACK too:

https://lkml.org/lkml/2019/9/27/164

Do I have miss a flag somewhere ?

Regards,

Benjamin

>
> Aside of that it definitely breaks everything which does not use the
> cpuidle stuff, which includes all machines affected by X86_BUG_AMD_APIC_C1E
> and everything which uses the INTEL_IDLE driver.
>
> Pretty much the same problem for all other places you changed.
> Thanks,
>
> 	tglx

Re: [PATCH] tick: check if broadcast device could really be stopped

[Thomas Gleixner]

On Mon, 14 Oct 2019, Benjamin GAIGNARD wrote:
> On 10/14/19 2:56 PM, Thomas Gleixner wrote:
> > On Wed, 9 Oct 2019, Benjamin Gaignard wrote:
> >> @@ -78,7 +78,7 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev,
> >>   {
> >>   	if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) ||
> >>   	    (newdev->features & CLOCK_EVT_FEAT_PERCPU) ||
> >> -	    (newdev->features & CLOCK_EVT_FEAT_C3STOP))
> >> +	    tick_broadcast_could_stop(newdev))
> > No. This might be called _before_ a cpuidle driver is available and then
> > when that driver is loaded and goes deep, everything goes south.
> 
> What could be the solution to let know to tick broadcast framework that 
> this device
> 
> will not be stopped (because CPU won't go in idle) ?
> 
> I have tried to put "always-on" property on DT but it was a NACK too:
> 
> https://lkml.org/lkml/2019/9/27/164
> 
> Do I have miss a flag somewhere ?

I don't understand what you are trying to achieve here. If the tick device,
i.e. the comparator stops working in deep idle states, then the device has
rightfully the CLOCK_EVT_FEAT_C3STOP (mis)feature set. Which means that the
system needs a fallback device for the deep idle case. If there is no
physical fallback device then you should enable the hrtimer based broadcast
pseudo tick device.

If the CPUs never go deep idle because there is no idle driver loaded or
the deep power state in which the comparator stops working is never
reached, then the broadcast hrtimer will never be used. It just eats a bit
of memory and a few extra instructions.

Thanks,

	tglx

Re: [PATCH] tick: check if broadcast device could really be stopped

[Benjamin GAIGNARD]

On 10/14/19 3:40 PM, Thomas Gleixner wrote:
> On Mon, 14 Oct 2019, Benjamin GAIGNARD wrote:
>> On 10/14/19 2:56 PM, Thomas Gleixner wrote:
>>> On Wed, 9 Oct 2019, Benjamin Gaignard wrote:
>>>> @@ -78,7 +78,7 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev,
>>>>    {
>>>>    	if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) ||
>>>>    	    (newdev->features & CLOCK_EVT_FEAT_PERCPU) ||
>>>> -	    (newdev->features & CLOCK_EVT_FEAT_C3STOP))
>>>> +	    tick_broadcast_could_stop(newdev))
>>> No. This might be called _before_ a cpuidle driver is available and then
>>> when that driver is loaded and goes deep, everything goes south.
>> What could be the solution to let know to tick broadcast framework that
>> this device
>>
>> will not be stopped (because CPU won't go in idle) ?
>>
>> I have tried to put "always-on" property on DT but it was a NACK too:
>>
>> https://lkml.org/lkml/2019/9/27/164
>>
>> Do I have miss a flag somewhere ?
> I don't understand what you are trying to achieve here. If the tick device,
> i.e. the comparator stops working in deep idle states, then the device has
> rightfully the CLOCK_EVT_FEAT_C3STOP (mis)feature set. Which means that the
> system needs a fallback device for the deep idle case. If there is no
> physical fallback device then you should enable the hrtimer based broadcast
> pseudo tick device.
>
> If the CPUs never go deep idle because there is no idle driver loaded or
> the deep power state in which the comparator stops working is never
> reached, then the broadcast hrtimer will never be used. It just eats a bit
> of memory and a few extra instructions.

Since CPUs won't go in deep idle I don't want to get a broadcast timer

but only an hrtimer to get accure latencies for the scheduler.

When arch arm timer doesn't set CLOCK_EVT_FEAT_C3STOP flag, my system

got a hrtimer and everything goes well. If arch arm timer set 
CLOCK_EVT_FEAT_C3STOP

hrtimer disappear (except if I add an additional broadcast timer).

What is the link between tick broadcast timer and hrtimer ? Does arch 
arm timer could only

implement them at the same time ?

Benjamin

>
> Thanks,
>
> 	tglx
>

Re: [PATCH] tick: check if broadcast device could really be stopped

[Thomas Gleixner]

On Mon, 14 Oct 2019, Benjamin GAIGNARD wrote:
> On 10/14/19 3:40 PM, Thomas Gleixner wrote:
> > I don't understand what you are trying to achieve here. If the tick device,
> > i.e. the comparator stops working in deep idle states, then the device has
> > rightfully the CLOCK_EVT_FEAT_C3STOP (mis)feature set. Which means that the
> > system needs a fallback device for the deep idle case. If there is no
> > physical fallback device then you should enable the hrtimer based broadcast
> > pseudo tick device.
> >
> > If the CPUs never go deep idle because there is no idle driver loaded or
> > the deep power state in which the comparator stops working is never
> > reached, then the broadcast hrtimer will never be used. It just eats a bit
> > of memory and a few extra instructions.
> 
> Since CPUs won't go in deep idle I don't want to get a broadcast timer
> but only an hrtimer to get accure latencies for the scheduler.

What's wrong with the broadcast timer if it is never utilized? It's there,
needs a few bytes of memory and that's it.

> When arch arm timer doesn't set CLOCK_EVT_FEAT_C3STOP flag, my system got
> a hrtimer and everything goes well.

Sure, but that's applicable to your particular system only and not a
generic solution. Neither your DT hack, nor the other one you posted.

> If arch arm timer set CLOCK_EVT_FEAT_C3STOP hrtimer disappear (except if
> I add an additional broadcast timer).

Right.

> What is the link between tick broadcast timer and hrtimer ? Does arch 
> arm timer could only implement them at the same time ?

If the clock event device is affected by deep power states, then the core
requires a fallback device, aka. broadcast timer, which makes sure that no
event is lost in case that the CPU goes into a deep power state. If no CPU
ever goes deep, the broadcast timer is there and unused.

Obviously the system cannot enable high resolution timers if the clock
event device is affected by power states.

Unless you have a solution which works under all circumstances (and the
current patch definitely does not) then you have to deal with the
requirement of the broadcast timer (either physical or software based).

"I don't want a broadcast timer falls" into the "I want a pony" realm.

Thanks,

	tglx

Re: [PATCH] tick: check if broadcast device could really be stopped

[Benjamin GAIGNARD]

On 10/14/19 4:28 PM, Thomas Gleixner wrote:
> On Mon, 14 Oct 2019, Benjamin GAIGNARD wrote:
>> On 10/14/19 3:40 PM, Thomas Gleixner wrote:
>>> I don't understand what you are trying to achieve here. If the tick device,
>>> i.e. the comparator stops working in deep idle states, then the device has
>>> rightfully the CLOCK_EVT_FEAT_C3STOP (mis)feature set. Which means that the
>>> system needs a fallback device for the deep idle case. If there is no
>>> physical fallback device then you should enable the hrtimer based broadcast
>>> pseudo tick device.
>>>
>>> If the CPUs never go deep idle because there is no idle driver loaded or
>>> the deep power state in which the comparator stops working is never
>>> reached, then the broadcast hrtimer will never be used. It just eats a bit
>>> of memory and a few extra instructions.
>> Since CPUs won't go in deep idle I don't want to get a broadcast timer
>> but only an hrtimer to get accure latencies for the scheduler.
> What's wrong with the broadcast timer if it is never utilized? It's there,
> needs a few bytes of memory and that's it.
>
>> When arch arm timer doesn't set CLOCK_EVT_FEAT_C3STOP flag, my system got
>> a hrtimer and everything goes well.
> Sure, but that's applicable to your particular system only and not a
> generic solution. Neither your DT hack, nor the other one you posted.
>
>> If arch arm timer set CLOCK_EVT_FEAT_C3STOP hrtimer disappear (except if
>> I add an additional broadcast timer).
> Right.
>
>> What is the link between tick broadcast timer and hrtimer ? Does arch
>> arm timer could only implement them at the same time ?
> If the clock event device is affected by deep power states, then the core
> requires a fallback device, aka. broadcast timer, which makes sure that no
> event is lost in case that the CPU goes into a deep power state. If no CPU
> ever goes deep, the broadcast timer is there and unused.
>
> Obviously the system cannot enable high resolution timers if the clock
> event device is affected by power states.
>
> Unless you have a solution which works under all circumstances (and the
> current patch definitely does not) then you have to deal with the
> requirement of the broadcast timer (either physical or software based).

If I follow you I need, for my system, a software based broadcast timer 
(tick-broadcast-hrtimer ?).

If that is correct I 'just' need to add a call to 
tick_setup_hrtimer_broadcast() in arch/arm/kernel/time.c

Regards,

Benjamin

>
> "I don't want a broadcast timer falls" into the "I want a pony" realm.
>
> Thanks,
>
> 	tglx
>

Re: [PATCH] tick: check if broadcast device could really be stopped

[Thomas Gleixner]

On Mon, 14 Oct 2019, Benjamin GAIGNARD wrote:
> On 10/14/19 4:28 PM, Thomas Gleixner wrote:
> > Unless you have a solution which works under all circumstances (and the
> > current patch definitely does not) then you have to deal with the
> > requirement of the broadcast timer (either physical or software based).
> 
> If I follow you I need, for my system, a software based broadcast timer 
> (tick-broadcast-hrtimer ?).

Yes, if you don't have a physical one.
 
> If that is correct I 'just' need to add a call to
> tick_setup_hrtimer_broadcast() in arch/arm/kernel/time.c

Correct.

Thanks,

	tglx