| Message ID | 1380732928-13897-1-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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 <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
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
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);
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(-)