| Message ID | 20130809172757.GD14845@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 09, 2013 at 10:27:57AM -0700, Stephen Boyd wrote: > On 08/09, Daniel Lezcano wrote: > > > > yes, but at least the broadcast mechanism should send an IPI to cpu0 to > > wake it up, no ? As Stephen stated this kind of configuration should has > > never been tested before so the tick broadcast code is not handling this > > case properly IMHO. > > > > If you have a per-cpu tick device that isn't suffering from > FEAT_C3_STOP why wouldn't you use that for the tick versus a > per-cpu tick device that has FEAT_C3_STOP? It sounds like there > is a bug in the preference logic or you should boost the rating > of the arm global timer above the twd. Does this patch help? It > should make the arm global timer the tick device and whatever the > cadence timer you have into the broadcast device. I'm not sure I'm getting this right. But neither the cadence_ttc nor the arm_global_timer have the FEAT_C3_STOP flag set. So, shouldn't they be treated equally even with your change? Sören
On 08/09/13 10:48, Sören Brinkmann wrote: > On Fri, Aug 09, 2013 at 10:27:57AM -0700, Stephen Boyd wrote: >> On 08/09, Daniel Lezcano wrote: >>> yes, but at least the broadcast mechanism should send an IPI to cpu0 to >>> wake it up, no ? As Stephen stated this kind of configuration should has >>> never been tested before so the tick broadcast code is not handling this >>> case properly IMHO. >>> >> If you have a per-cpu tick device that isn't suffering from >> FEAT_C3_STOP why wouldn't you use that for the tick versus a >> per-cpu tick device that has FEAT_C3_STOP? It sounds like there >> is a bug in the preference logic or you should boost the rating >> of the arm global timer above the twd. Does this patch help? It >> should make the arm global timer the tick device and whatever the >> cadence timer you have into the broadcast device. > I'm not sure I'm getting this right. But neither the cadence_ttc nor the > arm_global_timer have the FEAT_C3_STOP flag set. So, shouldn't they be > treated equally even with your change? The cadence_ttc is a global clockevent, i.e. the irq can interrupt any CPU, and it has a rating of 200. The arm global timer is a per-cpu clockevent with a rating of 300. The TWD is a per-cpu clockevent with a rating of 350. Because the arm global timer is rated higher than the cadence_ttc but less than the TWD the arm global timer will fill in the broadcast spot and the TWD will fill in the tick position. We really want the arm global timer to fill in the tick position and the cadence_ttc to fill in the broadcast spot (although the cadence_ttc should never be needed because the arm global timer doesn't need help in deep idle states). Unless I got lost in all the combinations of tests you've done so far?
On 08/09/2013 07:27 PM, Stephen Boyd wrote: > On 08/09, Daniel Lezcano wrote: >> >> yes, but at least the broadcast mechanism should send an IPI to cpu0 to >> wake it up, no ? As Stephen stated this kind of configuration should has >> never been tested before so the tick broadcast code is not handling this >> case properly IMHO. >> > > If you have a per-cpu tick device that isn't suffering from > FEAT_C3_STOP why wouldn't you use that for the tick versus a > per-cpu tick device that has FEAT_C3_STOP? It sounds like there > is a bug in the preference logic or you should boost the rating > of the arm global timer above the twd. Does this patch help? It > should make the arm global timer the tick device and whatever the > cadence timer you have into the broadcast device. > > ---8<---- > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index 218bcb5..d3539e5 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -77,6 +77,9 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > return false; > > + if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) > + return false; Yes, that makes sense to prevent local timer devices to be a broadcast one. In the case of the arm global timer, each cpu will register their timer, so the test will be ok but is it possible the cpu0 registers the timers for the other cpus ? > return !curdev || newdev->rating > curdev->rating; > } > > diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c > index 64522ec..1628b9f 100644 > --- a/kernel/time/tick-common.c > +++ b/kernel/time/tick-common.c > @@ -245,6 +245,15 @@ static bool tick_check_preferred(struct clock_event_device *curdev, > } > > /* > + * Prefer tick devices that don't suffer from FEAT_C3_STOP > + * regardless of their rating > + */ > + if (curdev && cpumask_equal(curdev->cpumask, newdev->cpumask) && > + !(newdev->features & CLOCK_EVT_FEAT_C3STOP) && > + (curdev->features & CLOCK_EVT_FEAT_C3STOP)) > + return true; That sounds reasonable, but what is the acceptable gap between the ratings ? I am wondering if there isn't too much heuristic in the tick code... > + > + /* > * Use the higher rated one, but prefer a CPU local device with a lower > * rating than a non-CPU local device > */ >
On Fri, Aug 09, 2013 at 10:27:57AM -0700, Stephen Boyd wrote: > On 08/09, Daniel Lezcano wrote: > > > > yes, but at least the broadcast mechanism should send an IPI to cpu0 to > > wake it up, no ? As Stephen stated this kind of configuration should has > > never been tested before so the tick broadcast code is not handling this > > case properly IMHO. > > > > If you have a per-cpu tick device that isn't suffering from > FEAT_C3_STOP why wouldn't you use that for the tick versus a > per-cpu tick device that has FEAT_C3_STOP? It sounds like there > is a bug in the preference logic or you should boost the rating > of the arm global timer above the twd. Does this patch help? It > should make the arm global timer the tick device and whatever the > cadence timer you have into the broadcast device. I finally got to test your patch. Unfortunately, it makes the system hang even earlier: Starting kernel ... Uncompressing Linux... done, booting the kernel. [ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Linux version 3.11.0-rc3-00002-g391ac9b (sorenb@xsjandreislx) (gcc version 4.7.2 (Sourcery CodeBench Lite 2012.09-104) ) #98 SMP PREEMPT Mon Aug 12 08:59:34 PDT 2013 [ 0.000000] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7), cr=18c5387d [ 0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache [ 0.000000] Machine: Xilinx Zynq Platform, model: Zynq ZC706 Development Board [ 0.000000] bootconsole [earlycon0] enabled [ 0.000000] cma: CMA: reserved 16 MiB at 2e800000 [ 0.000000] Memory policy: ECC disabled, Data cache writealloc [ 0.000000] PERCPU: Embedded 9 pages/cpu @c149c000 s14720 r8192 d13952 u36864 [ 0.000000] Built 1 zonelists in Zone order, mobility grouping on. Total pages: 260624 [ 0.000000] Kernel command line: console=ttyPS0,115200 earlyprintk [ 0.000000] PID hash table entries: 4096 (order: 2, 16384 bytes) [ 0.000000] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes) [ 0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes) [ 0.000000] Memory: 1004928K/1048576K available (4891K kernel code, 307K rwdata, 1564K rodata, 338K init, 5699K bss, 43648K reserved, 270336K highmem) [ 0.000000] Virtual kernel memory layout: [ 0.000000] vector : 0xffff0000 - 0xffff1000 ( 4 kB) [ 0.000000] fixmap : 0xfff00000 - 0xfffe0000 ( 896 kB) [ 0.000000] vmalloc : 0xf0000000 - 0xff000000 ( 240 MB) [ 0.000000] lowmem : 0xc0000000 - 0xef800000 ( 760 MB) [ 0.000000] pkmap : 0xbfe00000 - 0xc0000000 ( 2 MB) [ 0.000000] modules : 0xbf000000 - 0xbfe00000 ( 14 MB) [ 0.000000] .text : 0xc0008000 - 0xc0656128 (6457 kB) [ 0.000000] .init : 0xc0657000 - 0xc06ab980 ( 339 kB) [ 0.000000] .data : 0xc06ac000 - 0xc06f8c20 ( 308 kB) [ 0.000000] .bss : 0xc06f8c20 - 0xc0c89aa4 (5700 kB) [ 0.000000] Preemptible hierarchical RCU implementation. [ 0.000000] RCU lockdep checking is enabled. [ 0.000000] Additional per-CPU info printed with stalls. [ 0.000000] RCU restricting CPUs from NR_CPUS=4 to nr_cpu_ids=2. [ 0.000000] NR_IRQS:16 nr_irqs:16 16 [ 0.000000] slcr mapped to f0004000 [ 0.000000] Zynq clock init [ 0.000000] sched_clock: 32 bits at 333MHz, resolution 3ns, wraps every 12884ms [ 0.000000] ttc0 #0 at f0006000, irq=43 [ 0.000000] Console: colour dummy device 80x30 [ 0.000000] Lock dependency validator: Copyright (c) 2006 Red Hat, Inc., Ingo Molnar [ 0.000000] ... MAX_LOCKDEP_SUBCLASSES: 8 [ 0.000000] ... MAX_LOCK_DEPTH: 48 [ 0.000000] ... MAX_LOCKDEP_KEYS: 8191 [ 0.000000] ... CLASSHASH_SIZE: 4096 [ 0.000000] ... MAX_LOCKDEP_ENTRIES: 16384 [ 0.000000] ... MAX_LOCKDEP_CHAINS: 32768 [ 0.000000] ... CHAINHASH_SIZE: 16384 [ 0.000000] memory used by lock dependency info: 3695 kB [ 0.000000] per task-struct memory footprint: 1152 bytes [ 0.057541] Calibrating delay loop... 1325.46 BogoMIPS (lpj=6627328) [ 0.100248] pid_max: default: 32768 minimum: 301 [ 0.103294] Mount-cache hash table entries: 512 [ 0.114364] CPU: Testing write buffer coherency: ok [ 0.114513] ftrace: allocating 16143 entries in 48 pages [ 0.155012] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 Sören
On 08/12/2013 06:03 PM, Sören Brinkmann wrote: > On Fri, Aug 09, 2013 at 10:27:57AM -0700, Stephen Boyd wrote: >> On 08/09, Daniel Lezcano wrote: >>> >>> yes, but at least the broadcast mechanism should send an IPI to cpu0 to >>> wake it up, no ? As Stephen stated this kind of configuration should has >>> never been tested before so the tick broadcast code is not handling this >>> case properly IMHO. >>> >> >> If you have a per-cpu tick device that isn't suffering from >> FEAT_C3_STOP why wouldn't you use that for the tick versus a >> per-cpu tick device that has FEAT_C3_STOP? It sounds like there >> is a bug in the preference logic or you should boost the rating >> of the arm global timer above the twd. Does this patch help? It >> should make the arm global timer the tick device and whatever the >> cadence timer you have into the broadcast device. > > I finally got to test your patch. Unfortunately, it makes the system > hang even earlier: [ ... ] Can you boot with maxcpus=1 and then give the result of timer_list ?
On Mon, Aug 12, 2013 at 06:08:46PM +0200, Daniel Lezcano wrote: > On 08/12/2013 06:03 PM, Sören Brinkmann wrote: > > On Fri, Aug 09, 2013 at 10:27:57AM -0700, Stephen Boyd wrote: > >> On 08/09, Daniel Lezcano wrote: > >>> > >>> yes, but at least the broadcast mechanism should send an IPI to cpu0 to > >>> wake it up, no ? As Stephen stated this kind of configuration should has > >>> never been tested before so the tick broadcast code is not handling this > >>> case properly IMHO. > >>> > >> > >> If you have a per-cpu tick device that isn't suffering from > >> FEAT_C3_STOP why wouldn't you use that for the tick versus a > >> per-cpu tick device that has FEAT_C3_STOP? It sounds like there > >> is a bug in the preference logic or you should boost the rating > >> of the arm global timer above the twd. Does this patch help? It > >> should make the arm global timer the tick device and whatever the > >> cadence timer you have into the broadcast device. > > > > I finally got to test your patch. Unfortunately, it makes the system > > hang even earlier: > > [ ... ] > > Can you boot with maxcpus=1 and then give the result of timer_list ? That doesn't seem to help anymore. It hangs too. Same picture w/ and w/o 'maxcpus', except for [ 0.000000] Kernel command line: console=ttyPS0,115200 earlyprintk maxcpus=1 Sören
On 08/12/13 03:53, Daniel Lezcano wrote: > On 08/09/2013 07:27 PM, Stephen Boyd wrote: >> On 08/09, Daniel Lezcano wrote: >>> yes, but at least the broadcast mechanism should send an IPI to cpu0 to >>> wake it up, no ? As Stephen stated this kind of configuration should has >>> never been tested before so the tick broadcast code is not handling this >>> case properly IMHO. >>> >> If you have a per-cpu tick device that isn't suffering from >> FEAT_C3_STOP why wouldn't you use that for the tick versus a >> per-cpu tick device that has FEAT_C3_STOP? It sounds like there >> is a bug in the preference logic or you should boost the rating >> of the arm global timer above the twd. Does this patch help? It >> should make the arm global timer the tick device and whatever the >> cadence timer you have into the broadcast device. >> >> ---8<---- >> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c >> index 218bcb5..d3539e5 100644 >> --- a/kernel/time/tick-broadcast.c >> +++ b/kernel/time/tick-broadcast.c >> @@ -77,6 +77,9 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, >> !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) >> return false; >> >> + if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) >> + return false; > Yes, that makes sense to prevent local timer devices to be a broadcast one. > > In the case of the arm global timer, each cpu will register their timer, > so the test will be ok but is it possible the cpu0 registers the timers > for the other cpus ? As far as I know every tick device has to be registered on the CPU that it will be used on. See tick_check_percpu(). > >> return !curdev || newdev->rating > curdev->rating; >> } >> >> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c >> index 64522ec..1628b9f 100644 >> --- a/kernel/time/tick-common.c >> +++ b/kernel/time/tick-common.c >> @@ -245,6 +245,15 @@ static bool tick_check_preferred(struct clock_event_device *curdev, >> } >> >> /* >> + * Prefer tick devices that don't suffer from FEAT_C3_STOP >> + * regardless of their rating >> + */ >> + if (curdev && cpumask_equal(curdev->cpumask, newdev->cpumask) && >> + !(newdev->features & CLOCK_EVT_FEAT_C3STOP) && >> + (curdev->features & CLOCK_EVT_FEAT_C3STOP)) >> + return true; > That sounds reasonable, but what is the acceptable gap between the > ratings ? I am wondering if there isn't too much heuristic in the tick > code... Yes I wonder if we should just change the ratings of the clockevents. But it feels to me like the rating should only matter if the two are equal in features. Otherwise we can use the features to determine what we want.
On 08/12/2013 06:23 PM, Stephen Boyd wrote: > On 08/12/13 03:53, Daniel Lezcano wrote: >> On 08/09/2013 07:27 PM, Stephen Boyd wrote: >>> On 08/09, Daniel Lezcano wrote: >>>> yes, but at least the broadcast mechanism should send an IPI to cpu0 to >>>> wake it up, no ? As Stephen stated this kind of configuration should has >>>> never been tested before so the tick broadcast code is not handling this >>>> case properly IMHO. >>>> >>> If you have a per-cpu tick device that isn't suffering from >>> FEAT_C3_STOP why wouldn't you use that for the tick versus a >>> per-cpu tick device that has FEAT_C3_STOP? It sounds like there >>> is a bug in the preference logic or you should boost the rating >>> of the arm global timer above the twd. Does this patch help? It >>> should make the arm global timer the tick device and whatever the >>> cadence timer you have into the broadcast device. >>> >>> ---8<---- >>> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c >>> index 218bcb5..d3539e5 100644 >>> --- a/kernel/time/tick-broadcast.c >>> +++ b/kernel/time/tick-broadcast.c >>> @@ -77,6 +77,9 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, >>> !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) >>> return false; >>> >>> + if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) >>> + return false; >> Yes, that makes sense to prevent local timer devices to be a broadcast one. >> >> In the case of the arm global timer, each cpu will register their timer, >> so the test will be ok but is it possible the cpu0 registers the timers >> for the other cpus ? > > As far as I know every tick device has to be registered on the CPU that > it will be used on. See tick_check_percpu(). Ah, ok I see. Thx for the pointer. >>> return !curdev || newdev->rating > curdev->rating; >>> } >>> >>> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c >>> index 64522ec..1628b9f 100644 >>> --- a/kernel/time/tick-common.c >>> +++ b/kernel/time/tick-common.c >>> @@ -245,6 +245,15 @@ static bool tick_check_preferred(struct clock_event_device *curdev, >>> } >>> >>> /* >>> + * Prefer tick devices that don't suffer from FEAT_C3_STOP >>> + * regardless of their rating >>> + */ >>> + if (curdev && cpumask_equal(curdev->cpumask, newdev->cpumask) && >>> + !(newdev->features & CLOCK_EVT_FEAT_C3STOP) && >>> + (curdev->features & CLOCK_EVT_FEAT_C3STOP)) >>> + return true; >> That sounds reasonable, but what is the acceptable gap between the >> ratings ? I am wondering if there isn't too much heuristic in the tick >> code... > > Yes I wonder if we should just change the ratings of the clockevents. > But it feels to me like the rating should only matter if the two are > equal in features. Otherwise we can use the features to determine what > we want. Is it desirable for real time system ? (I am not expert in this area, so may be this question has no sense :)
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 218bcb5..d3539e5 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -77,6 +77,9 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; + if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) + return false; + return !curdev || newdev->rating > curdev->rating; } diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 64522ec..1628b9f 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -245,6 +245,15 @@ static bool tick_check_preferred(struct clock_event_device *curdev, } /* + * Prefer tick devices that don't suffer from FEAT_C3_STOP + * regardless of their rating + */ + if (curdev && cpumask_equal(curdev->cpumask, newdev->cpumask) && + !(newdev->features & CLOCK_EVT_FEAT_C3STOP) && + (curdev->features & CLOCK_EVT_FEAT_C3STOP)) + return true; + + /* * Use the higher rated one, but prefer a CPU local device with a lower * rating than a non-CPU local device */