clocksource: arc_timer: RTC: allow registration despite SMP

Submitted by Vineet Gupta on Feb. 2, 2017, 12:50 a.m.

Details

Message ID 1485996615-2511-1-git-send-email-vgupta@synopsys.com
State New
Headers show

Commit Message

Vineet Gupta Feb. 2, 2017, 12:50 a.m.
So far we didn't allow CPU private 64-bit RTC timer to register in SMP
as the individual counters may not be synchronized across cores.

However there is a situation when we build SMP kernel but want to use
the same image on UP as well as SMP hardware. Here we would certainly
want to use RTC, but current code doesn't allow as it only uses build
info (CONFIG_SMP) and not runtime info (num_online_cpus() or some such).

We can't possibly use num_online_cpus() anyways because clocksource
probe happens before other cpus are brought online

The simple fix is allow ETC probe for SMP and rely on higher rating of
GFRC to take over in general SMP case. We leave the pr_warn to notify
the user.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 drivers/clocksource/arc_timer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Daniel Lezcano Feb. 2, 2017, 2:27 p.m.
On Wed, Feb 01, 2017 at 04:50:15PM -0800, Vineet Gupta wrote:
> So far we didn't allow CPU private 64-bit RTC timer to register in SMP
> as the individual counters may not be synchronized across cores.

That is the case for the other archs and the per cpu clocksource are used, the
kernel is supposed to be immune against clocks drift. There are mechanisms to
have a synchronized view of the clock when they are per cpu and the kernel does
not compare clocks between cpus. 

I didn't realize that when we did the modification around this check.

May be you can double check these clocksources are really not suitable for SMP,
because if it is not the case the if !CONFIG_SMP could be simply removed.
 
A sidenote: RTC is confusing and should be changed to something else.

> However there is a situation when we build SMP kernel but want to use
> the same image on UP as well as SMP hardware. Here we would certainly
> want to use RTC, but current code doesn't allow as it only uses build
> info (CONFIG_SMP) and not runtime info (num_online_cpus() or some such).
> 
> We can't possibly use num_online_cpus() anyways because clocksource
> probe happens before other cpus are brought online
> 
> The simple fix is allow ETC probe for SMP and rely on higher rating of
> GFRC to take over in general SMP case. We leave the pr_warn to notify
> the user.
> 
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  drivers/clocksource/arc_timer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/arc_timer.c b/drivers/clocksource/arc_timer.c
> index 7517f959cba7..87f193794bf2 100644
> --- a/drivers/clocksource/arc_timer.c
> +++ b/drivers/clocksource/arc_timer.c
> @@ -145,10 +145,8 @@ static int __init arc_cs_setup_rtc(struct device_node *node)
>  	}
>  
>  	/* Local to CPU hence not usable in SMP */
> -	if (IS_ENABLED(CONFIG_SMP)) {
> +	if (IS_ENABLED(CONFIG_SMP))
>  		pr_warn("Local-64-bit-Ctr not usable in SMP");
> -		return -EINVAL;
> -	}
>  
>  	ret = arc_get_timer_clk(node);
>  	if (ret)
> -- 
> 2.7.4
>
Vineet Gupta Feb. 22, 2017, 1:16 a.m.
Hi Daniel,

Sorry for delayed response, was away from Linux for a bit :-(

On 02/02/2017 06:27 AM, Daniel Lezcano wrote:
> On Wed, Feb 01, 2017 at 04:50:15PM -0800, Vineet Gupta wrote:
>> So far we didn't allow CPU private 64-bit RTC timer to register in SMP
>> as the individual counters may not be synchronized across cores.
> 
> That is the case for the other archs and the per cpu clocksource are used, the
> kernel is supposed to be immune against clocks drift. There are mechanisms to
> have a synchronized view of the clock when they are per cpu and the kernel does
> not compare clocks between cpus. 

Right, we have tick broadcast etc. However as discussed before I really don't like
the overhead - specially given that SMP cores will have a truly cross-core-sync
GFRC timer and will serve as clocksrc anyways.

> I didn't realize that when we did the modification around this check.
> 
> May be you can double check these clocksources are really not suitable for SMP,
> because if it is not the case the if !CONFIG_SMP could be simply removed.

Technically with tick broadcast RTC could be used, but I would like to avoid that.
The usecase here is a CONFIG_SMP built kernel running on a UP only hardware (I was
playing with running same binary on UP / SMP hardware).

However it seems CONFIG_SMP overhead seems to be significant for production UP
hardware so this patch might not be important after all.

> A sidenote: RTC is confusing and should be changed to something else.

Yeah it is - but that is what hardware guys decided to name it

-Vineet

> 
>> However there is a situation when we build SMP kernel but want to use
>> the same image on UP as well as SMP hardware. Here we would certainly
>> want to use RTC, but current code doesn't allow as it only uses build
>> info (CONFIG_SMP) and not runtime info (num_online_cpus() or some such).
>>
>> We can't possibly use num_online_cpus() anyways because clocksource
>> probe happens before other cpus are brought online
>>
>> The simple fix is allow ETC probe for SMP and rely on higher rating of
>> GFRC to take over in general SMP case. We leave the pr_warn to notify
>> the user.
>>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>  drivers/clocksource/arc_timer.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/arc_timer.c b/drivers/clocksource/arc_timer.c
>> index 7517f959cba7..87f193794bf2 100644
>> --- a/drivers/clocksource/arc_timer.c
>> +++ b/drivers/clocksource/arc_timer.c
>> @@ -145,10 +145,8 @@ static int __init arc_cs_setup_rtc(struct device_node *node)
>>  	}
>>  
>>  	/* Local to CPU hence not usable in SMP */
>> -	if (IS_ENABLED(CONFIG_SMP)) {
>> +	if (IS_ENABLED(CONFIG_SMP))
>>  		pr_warn("Local-64-bit-Ctr not usable in SMP");
>> -		return -EINVAL;
>> -	}
>>  
>>  	ret = arc_get_timer_clk(node);
>>  	if (ret)
>> -- 
>> 2.7.4
>>
>

Patch hide | download patch | download mbox

diff --git a/drivers/clocksource/arc_timer.c b/drivers/clocksource/arc_timer.c
index 7517f959cba7..87f193794bf2 100644
--- a/drivers/clocksource/arc_timer.c
+++ b/drivers/clocksource/arc_timer.c
@@ -145,10 +145,8 @@  static int __init arc_cs_setup_rtc(struct device_node *node)
 	}
 
 	/* Local to CPU hence not usable in SMP */
-	if (IS_ENABLED(CONFIG_SMP)) {
+	if (IS_ENABLED(CONFIG_SMP))
 		pr_warn("Local-64-bit-Ctr not usable in SMP");
-		return -EINVAL;
-	}
 
 	ret = arc_get_timer_clk(node);
 	if (ret)