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 <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Russell King <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Santosh Shilimkar <[email protected]>
---
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
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
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
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
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
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
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
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
On Wed, Oct 2, 2013 at 12:42 PM, Stephen Boyd <[email protected]> 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
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
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