schedule_timeout sleeps too long after dividing CPU frequency

Commit Message

Mason May 20, 2015, 9:56 p.m. UTC

On 20/05/2015 22:52, Arnd Bergmann wrote:

> On Wednesday 20 May 2015 22:41:33 Mason wrote:
>
>> Oh... I didn't even think it made sense (and was supported)
>> to select more than one machine...
>>
>> Is this related to CONFIG_ARCH_MULTIPLATFORM?
> 
> Yes. In the old days, each platform had its own entry in the top-level
> choice statement, which meant they were mutually exclusive. Most of
> them are converted now and can be put into a single kernel with
> CONFIG_ARCH_MULTIPLATFORM, and we stopped accepting new ones that
> don't do this a few years ago.

Is the following patch (more) acceptable:



Or perhaps the other way around?
i.e. feat_c3stop initialized to 0 (thus in the bss section)
and set to FEAT_C3STOP if "twd_never_stops" doesn't exist...

Russell, when you added the FEAT_C3STOP flag unconditionally in
commit 5388a6b266, didn't that potentially break platforms that
didn't expect the flag to be set?

Regards.

Comments

Arnd Bergmann May 20, 2015, 10:18 p.m. UTC | #1

On Wednesday 20 May 2015 23:56:31 Mason wrote:
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index 6591e26fc13f..3e867b2f067f 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -32,6 +32,7 @@ static void __iomem *twd_base;
>  static struct clk *twd_clk;
>  static unsigned long twd_timer_rate;
>  static DEFINE_PER_CPU(bool, percpu_setup_called);
> +static int feat_c3stop = CLOCK_EVT_FEAT_C3STOP;
>  
>  static struct clock_event_device __percpu *twd_evt;
>  static int twd_ppi;
> @@ -294,7 +295,7 @@ static void twd_timer_setup(void)
>  
>         clk->name = "local_timer";
>         clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
> -                       CLOCK_EVT_FEAT_C3STOP;
> +                       feat_c3stop;
>         clk->rating = 350;
>         clk->set_mode = twd_set_mode;
>         clk->set_next_event = twd_set_next_event;
> @@ -346,6 +347,7 @@ static int __init twd_local_timer_common_register(struct device_node *np)
>                 goto out_irq;
>  
>         twd_get_clock(np);
> +       if (of_get_property(np, "twd_never_stops", NULL)) feat_c3stop = 0;

of_property_read_bool(), and newline between the condition and the
assignment.

>         /*
>          * Immediately configure the timer on the boot CPU, unless we need
> 
> 
> Or perhaps the other way around?
> i.e. feat_c3stop initialized to 0 (thus in the bss section)
> and set to FEAT_C3STOP if "twd_never_stops" doesn't exist...

yes.

> Russell, when you added the FEAT_C3STOP flag unconditionally in
> commit 5388a6b266, didn't that potentially break platforms that
> didn't expect the flag to be set?

To take a step back, you should first figure out whether clearing
this flag is actually the correct behavior for your hardware, or
just happens to work by accident.

	Arnd

Russell King - ARM Linux May 20, 2015, 11:14 p.m. UTC | #2

On Wed, May 20, 2015 at 11:56:31PM +0200, Mason wrote:
> Russell, when you added the FEAT_C3STOP flag unconditionally in
> commit 5388a6b266, didn't that potentially break platforms that
> didn't expect the flag to be set?

Why would it break any platforms at the time it was merged?

You're only having problems because you don't provide a global timer
to the kernel, which the C3STOP feature does require - and that's
because your platform appears to be very limited in what it can
provide.  All other platforms at the time provided a global timer,
so the scenario you're facing never existed.

Maybe you also should read the discussion around this which is over 5
years old.  You can find some initial discussion via these message IDs:

[e-mail]

And the thread "SMP Local timer and power management" from May 2010.

Yes, making it conditional depending on the platform was mooted, but
it seemed unnecessary.  The fact that no one until now has had a
problem with it is testament to the approach being correct for the
hardware that was in use over the last five years.  No bug reports
means no problem.

Mason May 21, 2015, 9:56 a.m. UTC | #3

On 21/05/2015 01:14, Russell King - ARM Linux wrote:

Do you ever sleep? :-)

> On Wed, May 20, 2015 at 11:56:31PM +0200, Mason wrote:
>
>> Russell, when you added the FEAT_C3STOP flag unconditionally in
>> commit 5388a6b266, didn't that potentially break platforms that
>> didn't expect the flag to be set?
> 
> Why would it break any platforms at the time it was merged?
> 
> You're only having problems because you don't provide a global timer
> to the kernel, which the C3STOP feature does require - and that's
> because your platform appears to be very limited in what it can
> provide.

For the record, I don't think it is the platform that is limited;
it is my understanding of Linux internals and interrupt management
that is insufficient. (I'm working on it.)

AFAICT, most of the Linux know-how is not explicitly spelled out in
Documentation, it's often bits-and-pieces hidden in long ML threads.
(Sites like LWN are a blessing for grunts like me. I await eagerly
the release of LDD4.)

> All other platforms at the time provided a global timer, so the
> scenario you're facing never existed.

Sorry, my limitations are showing... Take mach-ux500 for example.
Did it provide a global timer at the time?

$ git checkout 5388a6b266
$ git grep clock.*event arch/arm/mach-ux500
arch/arm/mach-ux500/localtimer.c: * Setup the local clock events for a CPU.
arch/arm/mach-ux500/localtimer.c:void __cpuinit local_timer_setup(struct clock_event_device *evt)

> Maybe you also should read the discussion around this which is over 5
> years old.  You can find some initial discussion via these message IDs:
> 
> [e-mail]
> And the thread "SMP Local timer and power management" from May 2010.

I downloaded gmane's NNTP archive for linux-arm-kernel (415000 message),
going back to 2002.

$ grep alpine.LFD.2.00.1004171152080.3625@ gmane.linux.ports.arm.kernel.msf
$ grep -i "SMP Local timer and power management" gmane.linux.ports.arm.kernel.msf

No hits using gmane's search interface either.

> Yes, making it conditional depending on the platform was mooted, but
> it seemed unnecessary.  The fact that no one until now has had a
> problem with it is testament to the approach being correct for the
> hardware that was in use over the last five years.  No bug reports
> means no problem.

I agree with this pragmatic stance.

Regards.

Russell King - ARM Linux May 21, 2015, 10:20 a.m. UTC | #4

On Thu, May 21, 2015 at 11:56:20AM +0200, Mason wrote:
> On 21/05/2015 01:14, Russell King - ARM Linux wrote:
> 
> Do you ever sleep? :-)

Yes, normally between about midnight-1am until 9am London time.  All
other hours are normally spent trying to keep up with the email deluge
(which gives me no satisfication what so ever, it feels like I haven't
achieved anything) or trying to get some real practical work done.

> Sorry, my limitations are showing... Take mach-ux500 for example.
> Did it provide a global timer at the time?

Yes.

> $ git checkout 5388a6b266
> $ git grep clock.*event arch/arm/mach-ux500
> arch/arm/mach-ux500/localtimer.c: * Setup the local clock events for a CPU.
> arch/arm/mach-ux500/localtimer.c:void __cpuinit local_timer_setup(struct clock_event_device *evt)

I have to wonder wtf you are trying to prove me wrong... in any case,
your "example" is too short-sighted to be meaningful.  If you want to
try and find an example, you have to read the code, and follow the code
paths.  Let's take your example of ux500.

$ git diff -u 5388a6b266.. -- arch/arm/mach-ux500 arch/arm/plat-nomadik

and search the output of that.  What you'll find is that ux500 has:

-MACHINE_START(U8500, "ST-Ericsson U5500 Platform")
-       .timer          = &ux500_timer,

-struct sys_timer ux500_timer = {
-       .init   = ux500_timer_init,
-};

and tracing this code...

-static void __init ux500_timer_init(void)
 {
-#ifdef CONFIG_LOCAL_TIMERS
-       /* Setup the local timer base */
-       twd_base = __io_address(UX500_TWD_BASE);
-#endif
-       /* Setup the MTU base */
-       if (cpu_is_u8500ed())
-               mtu_base = __io_address(U8500_MTU0_BASE_ED);
-       else
-               mtu_base = __io_address(UX500_MTU0_BASE);
-
-       nmdk_timer_init();
 }

Now, nmdk_timer_init() is in arch/arm/plat-nomadik/timer.c, and that
sets up the clocksouce for timekeeping, and the global clock event.
I'll let you read that code.

> I downloaded gmane's NNTP archive for linux-arm-kernel (415000 message),
> going back to 2002.
> 
> $ grep alpine.LFD.2.00.1004171152080.3625@ gmane.linux.ports.arm.kernel.msf
> $ grep -i "SMP Local timer and power management" gmane.linux.ports.arm.kernel.msf
> 
> No hits using gmane's search interface either.

Yea, sorry, those messages weren't on a mailing list, but were a private
discussion between Thomas, Catalin, Santosh and others.  As it's private,
it can't be published without the consent of everyone involved.  As people
would have been talking on behalf of their companies, and they have moved
on, it's highly unlikely that the people could give consent now.

Patch

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 6591e26fc13f..3e867b2f067f 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -32,6 +32,7 @@  static void __iomem *twd_base;
 static struct clk *twd_clk;
 static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, percpu_setup_called);
+static int feat_c3stop = CLOCK_EVT_FEAT_C3STOP;
 
 static struct clock_event_device __percpu *twd_evt;
 static int twd_ppi;
@@ -294,7 +295,7 @@  static void twd_timer_setup(void)
 
        clk->name = "local_timer";
        clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
-                       CLOCK_EVT_FEAT_C3STOP;
+                       feat_c3stop;
        clk->rating = 350;
        clk->set_mode = twd_set_mode;
        clk->set_next_event = twd_set_next_event;
@@ -346,6 +347,7 @@  static int __init twd_local_timer_common_register(struct device_node *np)
                goto out_irq;
 
        twd_get_clock(np);
+       if (of_get_property(np, "twd_never_stops", NULL)) feat_c3stop = 0;
 
        /*
         * Immediately configure the timer on the boot CPU, unless we need