Patchwork [1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources

login
register
mail settings
Submitter Stephen Boyd
Date Aug. 22, 2013, 5:06 p.m.
Message ID <1377191201-14696-1-git-send-email-sboyd@codeaurora.org>
Download mbox | patch
Permalink /patch/269119/
State New
Headers show

Comments

Stephen Boyd - Aug. 22, 2013, 5:06 p.m.
On most ARM systems the per-cpu clockevents are truly per-cpu in
the sense that they can't be controlled on any other CPU besides
the CPU that they interrupt. If one of these clockevents were to
become a broadcast source we will run into a lot of trouble
because the broadcast source is enabled on the first CPU to go
into deep idle (if that CPU suffers from FEAT_C3_STOP) and that
could be a different CPU than what the clockevent is interrupting
(or even worse the CPU that the clockevent interrupts could be
offline).

Theoretically it's possible to support per-cpu clockevents as the
broadcast source but so far we haven't needed this and supporting
it is rather complicated. Let's just deny the possibility for now
until this becomes a reality (let's hope it never does!).

Reported-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 kernel/time/tick-broadcast.c | 3 +++
 1 file changed, 3 insertions(+)
Santosh Shilimkar - Aug. 22, 2013, 5:33 p.m.
On Thursday 22 August 2013 01:06 PM, Stephen Boyd wrote:
> On some ARM systems there are two sets of per-cpu timers: the TWD
> timers and the global timers. The TWD timers are rated at 350 and
> the global timers are rated at 300 but the TWD timers suffer from
> FEAT_C3_STOP while the arm global timers don't. The tick device
> selection logic doesn't consider the FEAT_C3_STOP flag so we'll
> always end up with the TWD as the tick device although we don't
> want that.
> 
> Extend the preference logic to take the FEAT_C3_STOP flag into
> account and always prefer tick devices that don't suffer from
> FEAT_C3_STOP even if their rating is lower than tick devices that
> do suffer from FEAT_C3_STOP. This will remove the need to do any
> broadcasting on such ARM systems.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  kernel/time/tick-common.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index 64522ec..3ae437d 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -244,12 +244,22 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
>  			return false;
>  	}
>  
> +	if (!curdev)
> +		return true;
> +
> +	/* Always prefer a tick device that doesn't suffer from FEAT_C3STOP */
> +	if (!(newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
> +			(curdev->features & CLOCK_EVT_FEAT_C3STOP))
> +		return true;
> +	if ((newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
> +			!(curdev->features & CLOCK_EVT_FEAT_C3STOP))
> +		return false;
> +
I don't think preferring the clock-event which doesn't suffer
from FEAT_C3STOP is a good idea if the quality of the time source
is not same. Generally the global timers are slow and far away from
CPU(cycle cost). So as long as we don't get impacted because of low power
states, the tick should run out of local timers which are faster access
as well as high resolution.

Regards,
Santosh
Stephen Boyd - Aug. 22, 2013, 5:40 p.m.
On 08/22/13 10:33, Santosh Shilimkar wrote:
> On Thursday 22 August 2013 01:06 PM, Stephen Boyd wrote:
>> On some ARM systems there are two sets of per-cpu timers: the TWD
>> timers and the global timers. The TWD timers are rated at 350 and
>> the global timers are rated at 300 but the TWD timers suffer from
>> FEAT_C3_STOP while the arm global timers don't. The tick device
>> selection logic doesn't consider the FEAT_C3_STOP flag so we'll
>> always end up with the TWD as the tick device although we don't
>> want that.
>>
>> Extend the preference logic to take the FEAT_C3_STOP flag into
>> account and always prefer tick devices that don't suffer from
>> FEAT_C3_STOP even if their rating is lower than tick devices that
>> do suffer from FEAT_C3_STOP. This will remove the need to do any
>> broadcasting on such ARM systems.
>>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>>  kernel/time/tick-common.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
>> index 64522ec..3ae437d 100644
>> --- a/kernel/time/tick-common.c
>> +++ b/kernel/time/tick-common.c
>> @@ -244,12 +244,22 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
>>  			return false;
>>  	}
>>  
>> +	if (!curdev)
>> +		return true;
>> +
>> +	/* Always prefer a tick device that doesn't suffer from FEAT_C3STOP */
>> +	if (!(newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
>> +			(curdev->features & CLOCK_EVT_FEAT_C3STOP))
>> +		return true;
>> +	if ((newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
>> +			!(curdev->features & CLOCK_EVT_FEAT_C3STOP))
>> +		return false;
>> +
> I don't think preferring the clock-event which doesn't suffer
> from FEAT_C3STOP is a good idea if the quality of the time source
> is not same. Generally the global timers are slow and far away from
> CPU(cycle cost). So as long as we don't get impacted because of low power
> states, the tick should run out of local timers which are faster access
> as well as high resolution.
>

Fair enough. I have no data either way to convince anyone that this is a
good or bad idea so this patch should have probably been an RFC. Are you
hinting at something like switching to a per-cpu timer that doesn't
suffer from FEAT_C3_STOP when a CPU goes into a deep idle state?
Interesting idea but I think I'll leave that to someone else if they
really care to do that.
Santosh Shilimkar - Aug. 22, 2013, 5:48 p.m.
On Thursday 22 August 2013 01:40 PM, Stephen Boyd wrote:
> On 08/22/13 10:33, Santosh Shilimkar wrote:
>> On Thursday 22 August 2013 01:06 PM, Stephen Boyd wrote:
>>> On some ARM systems there are two sets of per-cpu timers: the TWD
>>> timers and the global timers. The TWD timers are rated at 350 and
>>> the global timers are rated at 300 but the TWD timers suffer from
>>> FEAT_C3_STOP while the arm global timers don't. The tick device
>>> selection logic doesn't consider the FEAT_C3_STOP flag so we'll
>>> always end up with the TWD as the tick device although we don't
>>> want that.
>>>
>>> Extend the preference logic to take the FEAT_C3_STOP flag into
>>> account and always prefer tick devices that don't suffer from
>>> FEAT_C3_STOP even if their rating is lower than tick devices that
>>> do suffer from FEAT_C3_STOP. This will remove the need to do any
>>> broadcasting on such ARM systems.
>>>
>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>> ---
>>>  kernel/time/tick-common.c | 14 ++++++++++++--
>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
>>> index 64522ec..3ae437d 100644
>>> --- a/kernel/time/tick-common.c
>>> +++ b/kernel/time/tick-common.c
>>> @@ -244,12 +244,22 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
>>>  			return false;
>>>  	}
>>>  
>>> +	if (!curdev)
>>> +		return true;
>>> +
>>> +	/* Always prefer a tick device that doesn't suffer from FEAT_C3STOP */
>>> +	if (!(newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
>>> +			(curdev->features & CLOCK_EVT_FEAT_C3STOP))
>>> +		return true;
>>> +	if ((newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
>>> +			!(curdev->features & CLOCK_EVT_FEAT_C3STOP))
>>> +		return false;
>>> +
>> I don't think preferring the clock-event which doesn't suffer
>> from FEAT_C3STOP is a good idea if the quality of the time source
>> is not same. Generally the global timers are slow and far away from
>> CPU(cycle cost). So as long as we don't get impacted because of low power
>> states, the tick should run out of local timers which are faster access
>> as well as high resolution.
>>
> 
> Fair enough. I have no data either way to convince anyone that this is a
> good or bad idea so this patch should have probably been an RFC. Are you
> hinting at something like switching to a per-cpu timer that doesn't
> suffer from FEAT_C3_STOP when a CPU goes into a deep idle state?
> Interesting idea but I think I'll leave that to someone else if they
> really care to do that.
> 
If the per-CPU timer don't suffer from C3_STOP, there is no need to
switch at all and the per CPU tick continue to runs on CPU local
timers. We need to switch to a broadcast device(no affected by C3_STOP)
only for the cases where the per-CPU timer wakeup don't work in CPU
low power states.

Are we talking about a hardware where one of the CPU from a cluster
has a local timer which is not affected by C3_STOP where as rest
of the CPU local timers are ? If yes, it must be crazy hardware and
we should not care too much about that.

Regards,
Santosh
Stephen Boyd - Aug. 22, 2013, 6:31 p.m.
On 08/22, Santosh Shilimkar wrote:
> On Thursday 22 August 2013 01:40 PM, Stephen Boyd wrote:
> > On 08/22/13 10:33, Santosh Shilimkar wrote:
> >> On Thursday 22 August 2013 01:06 PM, Stephen Boyd wrote:
> >>> On some ARM systems there are two sets of per-cpu timers: the TWD
> >>> timers and the global timers. The TWD timers are rated at 350 and
> >>> the global timers are rated at 300 but the TWD timers suffer from
> >>> FEAT_C3_STOP while the arm global timers don't. The tick device
> >>> selection logic doesn't consider the FEAT_C3_STOP flag so we'll
> >>> always end up with the TWD as the tick device although we don't
> >>> want that.
> >>>
> >>> Extend the preference logic to take the FEAT_C3_STOP flag into
> >>> account and always prefer tick devices that don't suffer from
> >>> FEAT_C3_STOP even if their rating is lower than tick devices that
> >>> do suffer from FEAT_C3_STOP. This will remove the need to do any
> >>> broadcasting on such ARM systems.
> >>>
> >>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> >>> ---
> >>>  kernel/time/tick-common.c | 14 ++++++++++++--
> >>>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> >>> index 64522ec..3ae437d 100644
> >>> --- a/kernel/time/tick-common.c
> >>> +++ b/kernel/time/tick-common.c
> >>> @@ -244,12 +244,22 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
> >>>  			return false;
> >>>  	}
> >>>  
> >>> +	if (!curdev)
> >>> +		return true;
> >>> +
> >>> +	/* Always prefer a tick device that doesn't suffer from FEAT_C3STOP */
> >>> +	if (!(newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
> >>> +			(curdev->features & CLOCK_EVT_FEAT_C3STOP))
> >>> +		return true;
> >>> +	if ((newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
> >>> +			!(curdev->features & CLOCK_EVT_FEAT_C3STOP))
> >>> +		return false;
> >>> +
> >> I don't think preferring the clock-event which doesn't suffer
> >> from FEAT_C3STOP is a good idea if the quality of the time source
> >> is not same. Generally the global timers are slow and far away from
> >> CPU(cycle cost). So as long as we don't get impacted because of low power
> >> states, the tick should run out of local timers which are faster access
> >> as well as high resolution.
> >>
> > 
> > Fair enough. I have no data either way to convince anyone that this is a
> > good or bad idea so this patch should have probably been an RFC. Are you
> > hinting at something like switching to a per-cpu timer that doesn't
> > suffer from FEAT_C3_STOP when a CPU goes into a deep idle state?
> > Interesting idea but I think I'll leave that to someone else if they
> > really care to do that.
> > 
> If the per-CPU timer don't suffer from C3_STOP, there is no need to
> switch at all and the per CPU tick continue to runs on CPU local
> timers. We need to switch to a broadcast device(no affected by C3_STOP)
> only for the cases where the per-CPU timer wakeup don't work in CPU
> low power states.
> 
> Are we talking about a hardware where one of the CPU from a cluster
> has a local timer which is not affected by C3_STOP where as rest
> of the CPU local timers are ? If yes, it must be crazy hardware and
> we should not care too much about that.

We're talking about a device with the TWD and the arm global
timers where each CPU has a TWD and an arm global timer and the
TWD stops during deep idle but the global timer doesn't.
Put another way, two timers per CPU where one doesn't work all
the time.
Santosh Shilimkar - Aug. 22, 2013, 9:06 p.m.
On Thursday 22 August 2013 02:31 PM, Stephen Boyd wrote:
> On 08/22, Santosh Shilimkar wrote:
>> On Thursday 22 August 2013 01:40 PM, Stephen Boyd wrote:
>>> On 08/22/13 10:33, Santosh Shilimkar wrote:
>>>> On Thursday 22 August 2013 01:06 PM, Stephen Boyd wrote:
>>>>> On some ARM systems there are two sets of per-cpu timers: the TWD
>>>>> timers and the global timers. The TWD timers are rated at 350 and
>>>>> the global timers are rated at 300 but the TWD timers suffer from
>>>>> FEAT_C3_STOP while the arm global timers don't. The tick device
>>>>> selection logic doesn't consider the FEAT_C3_STOP flag so we'll
>>>>> always end up with the TWD as the tick device although we don't
>>>>> want that.
>>>>>
>>>>> Extend the preference logic to take the FEAT_C3_STOP flag into
>>>>> account and always prefer tick devices that don't suffer from
>>>>> FEAT_C3_STOP even if their rating is lower than tick devices that
>>>>> do suffer from FEAT_C3_STOP. This will remove the need to do any
>>>>> broadcasting on such ARM systems.
>>>>>
>>>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>>>> ---
>>>>>  kernel/time/tick-common.c | 14 ++++++++++++--
>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
>>>>> index 64522ec..3ae437d 100644
>>>>> --- a/kernel/time/tick-common.c
>>>>> +++ b/kernel/time/tick-common.c
>>>>> @@ -244,12 +244,22 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
>>>>>  			return false;
>>>>>  	}
>>>>>  
>>>>> +	if (!curdev)
>>>>> +		return true;
>>>>> +
>>>>> +	/* Always prefer a tick device that doesn't suffer from FEAT_C3STOP */
>>>>> +	if (!(newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
>>>>> +			(curdev->features & CLOCK_EVT_FEAT_C3STOP))
>>>>> +		return true;
>>>>> +	if ((newdev->features & CLOCK_EVT_FEAT_C3STOP) &&
>>>>> +			!(curdev->features & CLOCK_EVT_FEAT_C3STOP))
>>>>> +		return false;
>>>>> +
>>>> I don't think preferring the clock-event which doesn't suffer
>>>> from FEAT_C3STOP is a good idea if the quality of the time source
>>>> is not same. Generally the global timers are slow and far away from
>>>> CPU(cycle cost). So as long as we don't get impacted because of low power
>>>> states, the tick should run out of local timers which are faster access
>>>> as well as high resolution.
>>>>
>>>
>>> Fair enough. I have no data either way to convince anyone that this is a
>>> good or bad idea so this patch should have probably been an RFC. Are you
>>> hinting at something like switching to a per-cpu timer that doesn't
>>> suffer from FEAT_C3_STOP when a CPU goes into a deep idle state?
>>> Interesting idea but I think I'll leave that to someone else if they
>>> really care to do that.
>>>
>> If the per-CPU timer don't suffer from C3_STOP, there is no need to
>> switch at all and the per CPU tick continue to runs on CPU local
>> timers. We need to switch to a broadcast device(no affected by C3_STOP)
>> only for the cases where the per-CPU timer wakeup don't work in CPU
>> low power states.
>>
>> Are we talking about a hardware where one of the CPU from a cluster
>> has a local timer which is not affected by C3_STOP where as rest
>> of the CPU local timers are ? If yes, it must be crazy hardware and
>> we should not care too much about that.
> 
> We're talking about a device with the TWD and the arm global
> timers where each CPU has a TWD and an arm global timer and the
> TWD stops during deep idle but the global timer doesn't.
> Put another way, two timers per CPU where one doesn't work all
> the time.
> 
This global timer is really not global timer then since its
per CPU. So in this case, you don't need to use TWD's and
hence no need of broadcast. You should rather not register the
TWD's since you have better per-CPU timer which is alive in
the low power states as well and hence that should be registered
as a per CPU clock-event. If the current ARM timer code
doesn't support this, it should be enhanced to be able to
achieve above.

Regards,
Santosh
Soren Brinkmann - Aug. 22, 2013, 11:38 p.m.
On Thu, Aug 22, 2013 at 10:06:40AM -0700, Stephen Boyd wrote:
> On most ARM systems the per-cpu clockevents are truly per-cpu in
> the sense that they can't be controlled on any other CPU besides
> the CPU that they interrupt. If one of these clockevents were to
> become a broadcast source we will run into a lot of trouble
> because the broadcast source is enabled on the first CPU to go
> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that
> could be a different CPU than what the clockevent is interrupting
> (or even worse the CPU that the clockevent interrupts could be
> offline).
> 
> Theoretically it's possible to support per-cpu clockevents as the
> broadcast source but so far we haven't needed this and supporting
> it is rather complicated. Let's just deny the possibility for now
> until this becomes a reality (let's hope it never does!).
> 
> Reported-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com>

This fixes the issue I reported when enabling the global timer on Zynq.
The global timer is prevented from becoming the broadcast device and my
system boots.

	Thanks,
	Sören
Soren Brinkmann - Sept. 5, 2013, 4:53 p.m.
On Thu, Aug 22, 2013 at 10:06:40AM -0700, Stephen Boyd wrote:
> On most ARM systems the per-cpu clockevents are truly per-cpu in
> the sense that they can't be controlled on any other CPU besides
> the CPU that they interrupt. If one of these clockevents were to
> become a broadcast source we will run into a lot of trouble
> because the broadcast source is enabled on the first CPU to go
> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that
> could be a different CPU than what the clockevent is interrupting
> (or even worse the CPU that the clockevent interrupts could be
> offline).
> 
> Theoretically it's possible to support per-cpu clockevents as the
> broadcast source but so far we haven't needed this and supporting
> it is rather complicated. Let's just deny the possibility for now
> until this becomes a reality (let's hope it never does!).
> 
> Reported-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Has this been merged anywhere?

	Thanks,
	Sören

Patch

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 218bcb5..d3539e5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -77,6 +77,9 @@  static bool tick_check_broadcast_device(struct clock_event_device *curdev,
 	    !(newdev->features & CLOCK_EVT_FEAT_ONESHOT))
 		return false;
 
+	if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id())))
+		return false;
+
 	return !curdev || newdev->rating > curdev->rating;
 }