* [PATCH] sched_clock: fix postinit no sched_clock function check
@ 2013-10-02 16:55 Santosh Shilimkar
2013-10-02 17:09 ` Will Deacon
0 siblings, 1 reply; 11+ messages in thread
From: Santosh Shilimkar @ 2013-10-02 16:55 UTC (permalink / raw)
To: linux-arm-kernel
The sched_clock code uses 2 levels of function pointers, sched_clock_func()
and read_sched_clock() but the no sched_clock check in postinit() just
checks read_sched_clock().
This leads to kernel falling back to jiffy based sched clock even in
presence of sched_clock_func() which is not desirable.
Fix the postinit() check to avoid the issue. Probably the issue is hidden
so far on most of the arm SOCs because of already existing sched_clock
registrations apart from arch_timer sched_clock. One can reproduce the
issue by just have arch_timer as sched_clock
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
kernel/time/sched_clock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index 0b479a6..15c4d78 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -179,7 +179,8 @@ void __init sched_clock_postinit(void)
* If no sched_clock function has been provided at that point,
* make it the final one one.
*/
- if (read_sched_clock == jiffy_sched_clock_read)
+ if ((read_sched_clock == jiffy_sched_clock_read) &&
+ (sched_clock_func == sched_clock_32))
setup_sched_clock(jiffy_sched_clock_read, 32, HZ);
sched_clock_poll(sched_clock_timer.data);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 16:55 [PATCH] sched_clock: fix postinit no sched_clock function check Santosh Shilimkar
@ 2013-10-02 17:09 ` Will Deacon
2013-10-02 17:14 ` Santosh Shilimkar
0 siblings, 1 reply; 11+ messages in thread
From: Will Deacon @ 2013-10-02 17:09 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 02, 2013 at 05:55:28PM +0100, Santosh Shilimkar wrote:
> The sched_clock code uses 2 levels of function pointers, sched_clock_func()
> and read_sched_clock() but the no sched_clock check in postinit() just
> checks read_sched_clock().
>
> This leads to kernel falling back to jiffy based sched clock even in
> presence of sched_clock_func() which is not desirable.
>
> Fix the postinit() check to avoid the issue. Probably the issue is hidden
> so far on most of the arm SOCs because of already existing sched_clock
> registrations apart from arch_timer sched_clock. One can reproduce the
> issue by just have arch_timer as sched_clock
Isn't this just an issue with the arch timer driver not calling
setup_sched_clock? Instead, we munge around with sched_clock_func directly,
which doesn't appear to be the way anybody else deals with this.
I'm not sure of the history though, so perhaps there's a reason for this...
Will
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 17:09 ` Will Deacon
@ 2013-10-02 17:14 ` Santosh Shilimkar
2013-10-02 17:22 ` Stephen Boyd
0 siblings, 1 reply; 11+ messages in thread
From: Santosh Shilimkar @ 2013-10-02 17:14 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 02 October 2013 01:09 PM, Will Deacon wrote:
> On Wed, Oct 02, 2013 at 05:55:28PM +0100, Santosh Shilimkar wrote:
>> The sched_clock code uses 2 levels of function pointers, sched_clock_func()
>> and read_sched_clock() but the no sched_clock check in postinit() just
>> checks read_sched_clock().
>>
>> This leads to kernel falling back to jiffy based sched clock even in
>> presence of sched_clock_func() which is not desirable.
>>
>> Fix the postinit() check to avoid the issue. Probably the issue is hidden
>> so far on most of the arm SOCs because of already existing sched_clock
>> registrations apart from arch_timer sched_clock. One can reproduce the
>> issue by just have arch_timer as sched_clock
>
> Isn't this just an issue with the arch timer driver not calling
> setup_sched_clock? Instead, we munge around with sched_clock_func directly,
> which doesn't appear to be the way anybody else deals with this.
>
I thought about that option as well but was not sure since even in that case
the check is not complete. We just ensure that function is popullated.
> I'm not sure of the history though, so perhaps there's a reason for this...
>
Am curious as well.
Regards,
Santosh
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 17:14 ` Santosh Shilimkar
@ 2013-10-02 17:22 ` Stephen Boyd
2013-10-02 17:27 ` Santosh Shilimkar
0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2013-10-02 17:22 UTC (permalink / raw)
To: linux-arm-kernel
On 10/02/13 10:14, Santosh Shilimkar wrote:
> On Wednesday 02 October 2013 01:09 PM, Will Deacon wrote:
>> On Wed, Oct 02, 2013 at 05:55:28PM +0100, Santosh Shilimkar wrote:
>>> The sched_clock code uses 2 levels of function pointers, sched_clock_func()
>>> and read_sched_clock() but the no sched_clock check in postinit() just
>>> checks read_sched_clock().
>>>
>>> This leads to kernel falling back to jiffy based sched clock even in
>>> presence of sched_clock_func() which is not desirable.
>>>
>>> Fix the postinit() check to avoid the issue. Probably the issue is hidden
>>> so far on most of the arm SOCs because of already existing sched_clock
>>> registrations apart from arch_timer sched_clock. One can reproduce the
>>> issue by just have arch_timer as sched_clock
>> Isn't this just an issue with the arch timer driver not calling
>> setup_sched_clock? Instead, we munge around with sched_clock_func directly,
>> which doesn't appear to be the way anybody else deals with this.
>>
> I thought about that option as well but was not sure since even in that case
> the check is not complete. We just ensure that function is popullated.
Yes, nothing is actually broken because sched_clock_func() won't try to
use the jiffy based read_sched_clock() function. I'm not sure we
actually need this patch besides to remove a useless timer that updates
the jiffy epoch. Can we wait until my 64-bit sched_clock patch series
lands in 3.13? It looks like I still need an ack from Will or Catalin on
the architected timer patch before the clocksource folks pick it up.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 17:22 ` Stephen Boyd
@ 2013-10-02 17:27 ` Santosh Shilimkar
2013-10-02 17:42 ` Stephen Boyd
0 siblings, 1 reply; 11+ messages in thread
From: Santosh Shilimkar @ 2013-10-02 17:27 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 02 October 2013 01:22 PM, Stephen Boyd wrote:
> On 10/02/13 10:14, Santosh Shilimkar wrote:
>> On Wednesday 02 October 2013 01:09 PM, Will Deacon wrote:
>>> On Wed, Oct 02, 2013 at 05:55:28PM +0100, Santosh Shilimkar wrote:
>>>> The sched_clock code uses 2 levels of function pointers, sched_clock_func()
>>>> and read_sched_clock() but the no sched_clock check in postinit() just
>>>> checks read_sched_clock().
>>>>
>>>> This leads to kernel falling back to jiffy based sched clock even in
>>>> presence of sched_clock_func() which is not desirable.
>>>>
>>>> Fix the postinit() check to avoid the issue. Probably the issue is hidden
>>>> so far on most of the arm SOCs because of already existing sched_clock
>>>> registrations apart from arch_timer sched_clock. One can reproduce the
>>>> issue by just have arch_timer as sched_clock
>>> Isn't this just an issue with the arch timer driver not calling
>>> setup_sched_clock? Instead, we munge around with sched_clock_func directly,
>>> which doesn't appear to be the way anybody else deals with this.
>>>
>> I thought about that option as well but was not sure since even in that case
>> the check is not complete. We just ensure that function is popullated.
>
> Yes, nothing is actually broken because sched_clock_func() won't try to
> use the jiffy based read_sched_clock() function. I'm not sure we
> actually need this patch besides to remove a useless timer that updates
> the jiffy epoch. Can we wait until my 64-bit sched_clock patch series
> lands in 3.13? It looks like I still need an ack from Will or Catalin on
> the architected timer patch before the clocksource folks pick it up.
>
Really... I have not created patch out of fun.
Its broken on my keystone machine at least where the sched_clock is
falling back on jiffy based sched_clock even in presence of arch_timer
sched_clock.
Regards,
Santosh
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 17:27 ` Santosh Shilimkar
@ 2013-10-02 17:42 ` Stephen Boyd
2013-10-02 17:48 ` Will Deacon
2013-10-02 18:14 ` Rob Herring
0 siblings, 2 replies; 11+ messages in thread
From: Stephen Boyd @ 2013-10-02 17:42 UTC (permalink / raw)
To: linux-arm-kernel
On 10/02/13 10:27, Santosh Shilimkar wrote:
> On Wednesday 02 October 2013 01:22 PM, Stephen Boyd wrote:
>> On 10/02/13 10:14, Santosh Shilimkar wrote:
>>> On Wednesday 02 October 2013 01:09 PM, Will Deacon wrote:
>>>> On Wed, Oct 02, 2013 at 05:55:28PM +0100, Santosh Shilimkar wrote:
>>>>> The sched_clock code uses 2 levels of function pointers, sched_clock_func()
>>>>> and read_sched_clock() but the no sched_clock check in postinit() just
>>>>> checks read_sched_clock().
>>>>>
>>>>> This leads to kernel falling back to jiffy based sched clock even in
>>>>> presence of sched_clock_func() which is not desirable.
>>>>>
>>>>> Fix the postinit() check to avoid the issue. Probably the issue is hidden
>>>>> so far on most of the arm SOCs because of already existing sched_clock
>>>>> registrations apart from arch_timer sched_clock. One can reproduce the
>>>>> issue by just have arch_timer as sched_clock
>>>> Isn't this just an issue with the arch timer driver not calling
>>>> setup_sched_clock? Instead, we munge around with sched_clock_func directly,
>>>> which doesn't appear to be the way anybody else deals with this.
>>>>
>>> I thought about that option as well but was not sure since even in that case
>>> the check is not complete. We just ensure that function is popullated.
>> Yes, nothing is actually broken because sched_clock_func() won't try to
>> use the jiffy based read_sched_clock() function. I'm not sure we
>> actually need this patch besides to remove a useless timer that updates
>> the jiffy epoch. Can we wait until my 64-bit sched_clock patch series
>> lands in 3.13? It looks like I still need an ack from Will or Catalin on
>> the architected timer patch before the clocksource folks pick it up.
>>
> Really... I have not created patch out of fun.
> Its broken on my keystone machine at least where the sched_clock is
> falling back on jiffy based sched_clock even in presence of arch_timer
> sched_clock.
How is that possible? sched_clock_func is only assigned by
arch/arm/kernel/arch_timer.c when the architected timer is detected and
sched_clock() in kernel/time/sched_clock.c calls that function pointer
unconditionally. The only way I see this happening is if the architected
timer rate is zero. I agree we will get two lines in the dmesg about
sched_clock and its not very clear which one is being used.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 17:42 ` Stephen Boyd
@ 2013-10-02 17:48 ` Will Deacon
2013-10-02 18:07 ` Santosh Shilimkar
2013-10-02 18:14 ` Rob Herring
1 sibling, 1 reply; 11+ messages in thread
From: Will Deacon @ 2013-10-02 17:48 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 02, 2013 at 06:42:40PM +0100, Stephen Boyd wrote:
> On 10/02/13 10:27, Santosh Shilimkar wrote:
> > Really... I have not created patch out of fun.
> > Its broken on my keystone machine at least where the sched_clock is
> > falling back on jiffy based sched_clock even in presence of arch_timer
> > sched_clock.
>
> How is that possible? sched_clock_func is only assigned by
> arch/arm/kernel/arch_timer.c when the architected timer is detected and
> sched_clock() in kernel/time/sched_clock.c calls that function pointer
> unconditionally. The only way I see this happening is if the architected
> timer rate is zero.
^^^^^^^^^^^^^^^^^^
*cough* CNTFRQ *cough*
:)
Will
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 17:48 ` Will Deacon
@ 2013-10-02 18:07 ` Santosh Shilimkar
2013-10-09 23:59 ` John Stultz
0 siblings, 1 reply; 11+ messages in thread
From: Santosh Shilimkar @ 2013-10-02 18:07 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 02 October 2013 01:48 PM, Will Deacon wrote:
> On Wed, Oct 02, 2013 at 06:42:40PM +0100, Stephen Boyd wrote:
>> On 10/02/13 10:27, Santosh Shilimkar wrote:
>>> Really... I have not created patch out of fun.
>>> Its broken on my keystone machine at least where the sched_clock is
>>> falling back on jiffy based sched_clock even in presence of arch_timer
>>> sched_clock.
>>
>> How is that possible? sched_clock_func is only assigned by
>> arch/arm/kernel/arch_timer.c when the architected timer is detected and
>> sched_clock() in kernel/time/sched_clock.c calls that function pointer
>> unconditionally. The only way I see this happening is if the architected
>> timer rate is zero.
> ^^^^^^^^^^^^^^^^^^
>
> *cough* CNTFRQ *cough*
>
:) CNTFRQ as such is fine. I think the below print mis-lead me mostly.
sched_clock: ARM arch timer >56 bits at 6144kHz, resolution 162ns
sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every 4294967286ms
So yes, now the subject patch actually just avoids the jiffy sched_clock()
registration and nothing else. Even without the patch arch_timer sched_clock
will be in use.
Regards,
Santosh
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 18:07 ` Santosh Shilimkar
@ 2013-10-09 23:59 ` John Stultz
2013-10-10 0:15 ` Santosh Shilimkar
0 siblings, 1 reply; 11+ messages in thread
From: John Stultz @ 2013-10-09 23:59 UTC (permalink / raw)
To: linux-arm-kernel
On 10/02/2013 11:07 AM, Santosh Shilimkar wrote:
> On Wednesday 02 October 2013 01:48 PM, Will Deacon wrote:
>> On Wed, Oct 02, 2013 at 06:42:40PM +0100, Stephen Boyd wrote:
>>> On 10/02/13 10:27, Santosh Shilimkar wrote:
>>>> Really... I have not created patch out of fun.
>>>> Its broken on my keystone machine at least where the sched_clock is
>>>> falling back on jiffy based sched_clock even in presence of arch_timer
>>>> sched_clock.
>>> How is that possible? sched_clock_func is only assigned by
>>> arch/arm/kernel/arch_timer.c when the architected timer is detected and
>>> sched_clock() in kernel/time/sched_clock.c calls that function pointer
>>> unconditionally. The only way I see this happening is if the architected
>>> timer rate is zero.
>> ^^^^^^^^^^^^^^^^^^
>>
>> *cough* CNTFRQ *cough*
>>
> :) CNTFRQ as such is fine. I think the below print mis-lead me mostly.
>
> sched_clock: ARM arch timer >56 bits at 6144kHz, resolution 162ns
> sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every 4294967286ms
>
> So yes, now the subject patch actually just avoids the jiffy sched_clock()
> registration and nothing else. Even without the patch arch_timer sched_clock
> will be in use.
Just wanted to follow up here, as I've not been paying close attention.
Is this issue then resolved, or is something still needed to be queued
for 3.12/3.13?
thanks
-john
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-09 23:59 ` John Stultz
@ 2013-10-10 0:15 ` Santosh Shilimkar
0 siblings, 0 replies; 11+ messages in thread
From: Santosh Shilimkar @ 2013-10-10 0:15 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 09 October 2013 07:59 PM, John Stultz wrote:
> On 10/02/2013 11:07 AM, Santosh Shilimkar wrote:
>> On Wednesday 02 October 2013 01:48 PM, Will Deacon wrote:
>>> On Wed, Oct 02, 2013 at 06:42:40PM +0100, Stephen Boyd wrote:
>>>> On 10/02/13 10:27, Santosh Shilimkar wrote:
>>>>> Really... I have not created patch out of fun.
>>>>> Its broken on my keystone machine at least where the sched_clock is
>>>>> falling back on jiffy based sched_clock even in presence of arch_timer
>>>>> sched_clock.
>>>> How is that possible? sched_clock_func is only assigned by
>>>> arch/arm/kernel/arch_timer.c when the architected timer is detected and
>>>> sched_clock() in kernel/time/sched_clock.c calls that function pointer
>>>> unconditionally. The only way I see this happening is if the architected
>>>> timer rate is zero.
>>> ^^^^^^^^^^^^^^^^^^
>>>
>>> *cough* CNTFRQ *cough*
>>>
>> :) CNTFRQ as such is fine. I think the below print mis-lead me mostly.
>>
>> sched_clock: ARM arch timer >56 bits at 6144kHz, resolution 162ns
>> sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every 4294967286ms
>>
>> So yes, now the subject patch actually just avoids the jiffy sched_clock()
>> registration and nothing else. Even without the patch arch_timer sched_clock
>> will be in use.
>
> Just wanted to follow up here, as I've not been paying close attention.
> Is this issue then resolved, or is something still needed to be queued
> for 3.12/3.13?
>
There is no regression as I initially thought. Patch fixes the
miss-leading sched_clock print and also prevents timer to handle
wrapping which is not needed.
So no big deal and I don't mind if we don't apply it.
Regards,
Santosh
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] sched_clock: fix postinit no sched_clock function check
2013-10-02 17:42 ` Stephen Boyd
2013-10-02 17:48 ` Will Deacon
@ 2013-10-02 18:14 ` Rob Herring
1 sibling, 0 replies; 11+ messages in thread
From: Rob Herring @ 2013-10-02 18:14 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 2, 2013 at 12:42 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 10/02/13 10:27, Santosh Shilimkar wrote:
>> On Wednesday 02 October 2013 01:22 PM, Stephen Boyd wrote:
>>> On 10/02/13 10:14, Santosh Shilimkar wrote:
>>>> On Wednesday 02 October 2013 01:09 PM, Will Deacon wrote:
>>>>> On Wed, Oct 02, 2013 at 05:55:28PM +0100, Santosh Shilimkar wrote:
>>>>>> The sched_clock code uses 2 levels of function pointers, sched_clock_func()
>>>>>> and read_sched_clock() but the no sched_clock check in postinit() just
>>>>>> checks read_sched_clock().
>>>>>>
>>>>>> This leads to kernel falling back to jiffy based sched clock even in
>>>>>> presence of sched_clock_func() which is not desirable.
>>>>>>
>>>>>> Fix the postinit() check to avoid the issue. Probably the issue is hidden
>>>>>> so far on most of the arm SOCs because of already existing sched_clock
>>>>>> registrations apart from arch_timer sched_clock. One can reproduce the
>>>>>> issue by just have arch_timer as sched_clock
>>>>> Isn't this just an issue with the arch timer driver not calling
>>>>> setup_sched_clock? Instead, we munge around with sched_clock_func directly,
>>>>> which doesn't appear to be the way anybody else deals with this.
>>>>>
>>>> I thought about that option as well but was not sure since even in that case
>>>> the check is not complete. We just ensure that function is popullated.
>>> Yes, nothing is actually broken because sched_clock_func() won't try to
>>> use the jiffy based read_sched_clock() function. I'm not sure we
>>> actually need this patch besides to remove a useless timer that updates
>>> the jiffy epoch. Can we wait until my 64-bit sched_clock patch series
>>> lands in 3.13? It looks like I still need an ack from Will or Catalin on
>>> the architected timer patch before the clocksource folks pick it up.
>>>
>> Really... I have not created patch out of fun.
>> Its broken on my keystone machine at least where the sched_clock is
>> falling back on jiffy based sched_clock even in presence of arch_timer
>> sched_clock.
>
> How is that possible? sched_clock_func is only assigned by
> arch/arm/kernel/arch_timer.c when the architected timer is detected and
> sched_clock() in kernel/time/sched_clock.c calls that function pointer
> unconditionally. The only way I see this happening is if the architected
> timer rate is zero. I agree we will get two lines in the dmesg about
> sched_clock and its not very clear which one is being used.
It does setup a timer to run to handle wrapping which should be
harmless, but isn't needed.
Rob
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-10-10 0:15 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-02 16:55 [PATCH] sched_clock: fix postinit no sched_clock function check Santosh Shilimkar
2013-10-02 17:09 ` Will Deacon
2013-10-02 17:14 ` Santosh Shilimkar
2013-10-02 17:22 ` Stephen Boyd
2013-10-02 17:27 ` Santosh Shilimkar
2013-10-02 17:42 ` Stephen Boyd
2013-10-02 17:48 ` Will Deacon
2013-10-02 18:07 ` Santosh Shilimkar
2013-10-09 23:59 ` John Stultz
2013-10-10 0:15 ` Santosh Shilimkar
2013-10-02 18:14 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).