* [resend] Timer broadcast question [not found] ` <5123C299.3080005@linaro.org> @ 2013-02-21 6:19 ` Santosh Shilimkar 2013-02-21 9:01 ` Daniel Lezcano 2013-02-21 22:01 ` [PATCH 1/2] time : pass broadcast device parameter Daniel Lezcano 1 sibling, 1 reply; 13+ messages in thread From: Santosh Shilimkar @ 2013-02-21 6:19 UTC (permalink / raw) To: linux-arm-kernel On Tuesday 19 February 2013 11:51 PM, Daniel Lezcano wrote: > On 02/19/2013 07:10 PM, Thomas Gleixner wrote: >> On Tue, 19 Feb 2013, Daniel Lezcano wrote: >>> I am working on identifying the different wakeup sources from the >>> interrupts and I have a question regarding the timer broadcast. >>> >>> The broadcast timer is setup to the next event and that will wake up any >>> idle cpu belonging to the "broadcast cpumask", right ? >>> >>> The cpu which has been woken up will look for each cpu the next-event >>> and send an IPI to wake it up. >>> >>> Although, it is possible the sender of this IPI may not be concerned by >>> the timer expiration and has been woken up just for sending the IPI, right ? >> >> Correct. >> >>> If this is correct, is it possible to setup the timer irq affinity to a >>> cpu which will be concerned by the timer expiration ? so we prevent an >>> unnecessary wake up for a cpu. >> >> It is possible, but we never implemented it. >> >> If we go there, we want to make that conditional on a property flag, >> because some interrupt controllers especially on x86 only allow to >> move the affinity from interrupt context, which is pointless. > > Thanks Thomas for your quick answer. I will write a RFC patchset. > Last year I implemented the affinity hook for broad-cast code and experimented with it. Since the system I was using was dual core, it wasn't much beneficial and hence gave up later. I did remember discussing the approach with few folks in the conference. Patch in the end of the email (also attached) for generic broadcast code. I didn't look at all corner case though. In arch code then you need to setup "broadcast_affinity" hook which should be able to get handle of the arch irqchip and call the respective affinity handler. Just 3 lines function should do the trick. As Thomas said, effectiveness of such optimization solely depends on how well the affinity (in low powers) supported by your IRQ chip. Hope this is helpful for you. Regards, Santosh From d70f2d48ec08a3f1d73187c49b16e4e60f81a50c Mon Sep 17 00:00:00 2001 From: Santosh Shilimkar <santosh.shilimkar@ti.com> Date: Wed, 25 Jul 2012 03:42:33 +0530 Subject: [PATCH] tick-broadcast: Add tick road-cast affinity suport Current tick broad-cast code has affinity set to the boot CPU and hence the boot CPU will always wakeup from low power states when broad cast timer is armed even if the next expiry event doesn't belong to it. Patch adds broadcast affinity functionality to avoid above and let the tick framework set the affinity of the event for the CPU it belongs. Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> --- include/linux/clockchips.h | 2 ++ kernel/time/tick-broadcast.c | 13 ++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 8a7096f..5488cdc 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -95,6 +95,8 @@ struct clock_event_device { unsigned long retries; void (*broadcast)(const struct cpumask *mask); + void (*broadcast_affinity) + (const struct cpumask *mask, int irq); void (*set_mode)(enum clock_event_mode mode, struct clock_event_device *); void (*suspend)(struct clock_event_device *); diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index f113755..2ec2425 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -39,6 +39,8 @@ static void tick_broadcast_clear_oneshot(int cpu); static inline void tick_broadcast_clear_oneshot(int cpu) { } #endif +static inline void dummy_broadcast_affinity(const struct cpumask *mask, + int irq) { } /* * Debugging: see timer_list.c */ @@ -485,14 +487,19 @@ void tick_broadcast_oneshot_control(unsigned long reason) if (!cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) { cpumask_set_cpu(cpu, tick_get_broadcast_oneshot_mask()); clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); - if (dev->next_event.tv64 < bc->next_event.tv64) + if (dev->next_event.tv64 < bc->next_event.tv64) { tick_broadcast_set_event(dev->next_event, 1); + bc->broadcast_affinity( + tick_get_broadcast_oneshot_mask(), bc->irq); + } } } else { if (cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) { cpumask_clear_cpu(cpu, tick_get_broadcast_oneshot_mask()); clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + bc->broadcast_affinity( + tick_get_broadcast_oneshot_mask(), bc->irq); if (dev->next_event.tv64 != KTIME_MAX) tick_program_event(dev->next_event, 1); } @@ -536,6 +543,10 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc) bc->event_handler = tick_handle_oneshot_broadcast; + /* setup dummy broadcast affinity handler if not provided */ + if (bc->broadcast_affinity) + bc->broadcast_affinity = dummy_broadcast_affinity; + /* Take the do_timer update */ tick_do_timer_cpu = cpu; -- 1.7.9.5 -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-tick-broadcast-Add-tick-road-cast-affinity-suport.patch Type: text/x-patch Size: 3001 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130221/721d4f50/attachment.bin> ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [resend] Timer broadcast question 2013-02-21 6:19 ` [resend] Timer broadcast question Santosh Shilimkar @ 2013-02-21 9:01 ` Daniel Lezcano 2013-02-21 9:14 ` Santosh Shilimkar 0 siblings, 1 reply; 13+ messages in thread From: Daniel Lezcano @ 2013-02-21 9:01 UTC (permalink / raw) To: linux-arm-kernel On 02/21/2013 07:19 AM, Santosh Shilimkar wrote: > On Tuesday 19 February 2013 11:51 PM, Daniel Lezcano wrote: >> On 02/19/2013 07:10 PM, Thomas Gleixner wrote: >>> On Tue, 19 Feb 2013, Daniel Lezcano wrote: >>>> I am working on identifying the different wakeup sources from the >>>> interrupts and I have a question regarding the timer broadcast. >>>> >>>> The broadcast timer is setup to the next event and that will wake up >>>> any >>>> idle cpu belonging to the "broadcast cpumask", right ? >>>> >>>> The cpu which has been woken up will look for each cpu the next-event >>>> and send an IPI to wake it up. >>>> >>>> Although, it is possible the sender of this IPI may not be concerned by >>>> the timer expiration and has been woken up just for sending the IPI, >>>> right ? >>> >>> Correct. >>> >>>> If this is correct, is it possible to setup the timer irq affinity to a >>>> cpu which will be concerned by the timer expiration ? so we prevent an >>>> unnecessary wake up for a cpu. >>> >>> It is possible, but we never implemented it. >>> >>> If we go there, we want to make that conditional on a property flag, >>> because some interrupt controllers especially on x86 only allow to >>> move the affinity from interrupt context, which is pointless. >> >> Thanks Thomas for your quick answer. I will write a RFC patchset. >> > Last year I implemented the affinity hook for broad-cast code and > experimented with it. Since the system I was using was dual core, > it wasn't much beneficial and hence gave up later. I did remember > discussing the approach with few folks in the conference. I did a brief test with a similar patch on a ARM u8500 board. The timer is tied with CPU0 by default, setting the dynamic irq affinity reduce considerably the number of IPI. The difference with your patch is the affinity is set to one CPU, the first one which is supposed to be wake up by the timer expiration. This is easy to spot with a small program doing usleep wired on CPU1. We see CPU0 waking up to send an IPI to CPU1 and going to idle again. I don't know how that behaves with OMAP4 with this patch (which I guess it is the board you used), but the coupled idle state traces could be ambiguous if you relied on it to check the benefit of this patch. IMO, it is worth to implement such solution and perhaps we can extend it to optimize the package idle time with the generic power domain tied with the irq. Anyway, it is a random thought let's see that later :) > Patch in the end of the email (also attached) for generic broadcast > code. I didn't look at all corner case though. In arch code then > you need to setup "broadcast_affinity" hook which should be able > to get handle of the arch irqchip and call the respective affinity > handler. Just 3 lines function should do the trick. > > As Thomas said, effectiveness of such optimization solely depends > on how well the affinity (in low powers) supported by your IRQ chip. > > Hope this is helpful for you. Thanks a lot for your patch and your feedbacks. -- Daniel > > From d70f2d48ec08a3f1d73187c49b16e4e60f81a50c Mon Sep 17 00:00:00 2001 > From: Santosh Shilimkar <santosh.shilimkar@ti.com> > Date: Wed, 25 Jul 2012 03:42:33 +0530 > Subject: [PATCH] tick-broadcast: Add tick road-cast affinity suport > > Current tick broad-cast code has affinity set to the boot CPU and hence > the boot CPU will always wakeup from low power states when broad cast timer > is armed even if the next expiry event doesn't belong to it. > > Patch adds broadcast affinity functionality to avoid above and let the > tick framework set the affinity of the event for the CPU it belongs. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > include/linux/clockchips.h | 2 ++ > kernel/time/tick-broadcast.c | 13 ++++++++++++- > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > index 8a7096f..5488cdc 100644 > --- a/include/linux/clockchips.h > +++ b/include/linux/clockchips.h > @@ -95,6 +95,8 @@ struct clock_event_device { > unsigned long retries; > > void (*broadcast)(const struct cpumask *mask); > + void (*broadcast_affinity) > + (const struct cpumask *mask, int irq); > void (*set_mode)(enum clock_event_mode mode, > struct clock_event_device *); > void (*suspend)(struct clock_event_device *); > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index f113755..2ec2425 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -39,6 +39,8 @@ static void tick_broadcast_clear_oneshot(int cpu); > static inline void tick_broadcast_clear_oneshot(int cpu) { } > #endif > > +static inline void dummy_broadcast_affinity(const struct cpumask *mask, > + int irq) { } > /* > * Debugging: see timer_list.c > */ > @@ -485,14 +487,19 @@ void tick_broadcast_oneshot_control(unsigned long > reason) > if (!cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) { > cpumask_set_cpu(cpu, tick_get_broadcast_oneshot_mask()); > clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); > - if (dev->next_event.tv64 < bc->next_event.tv64) > + if (dev->next_event.tv64 < bc->next_event.tv64) { > tick_broadcast_set_event(dev->next_event, 1); > + bc->broadcast_affinity( > + tick_get_broadcast_oneshot_mask(), bc->irq); > + } > } > } else { > if (cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) { > cpumask_clear_cpu(cpu, > tick_get_broadcast_oneshot_mask()); > clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); > + bc->broadcast_affinity( > + tick_get_broadcast_oneshot_mask(), bc->irq); > if (dev->next_event.tv64 != KTIME_MAX) > tick_program_event(dev->next_event, 1); > } > @@ -536,6 +543,10 @@ void tick_broadcast_setup_oneshot(struct > clock_event_device *bc) > > bc->event_handler = tick_handle_oneshot_broadcast; > > + /* setup dummy broadcast affinity handler if not provided */ > + if (bc->broadcast_affinity) > + bc->broadcast_affinity = dummy_broadcast_affinity; > + > /* Take the do_timer update */ > tick_do_timer_cpu = cpu; > -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog * English - detected * English * French * English * French <javascript:void(0);> <#> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [resend] Timer broadcast question 2013-02-21 9:01 ` Daniel Lezcano @ 2013-02-21 9:14 ` Santosh Shilimkar 0 siblings, 0 replies; 13+ messages in thread From: Santosh Shilimkar @ 2013-02-21 9:14 UTC (permalink / raw) To: linux-arm-kernel On Thursday 21 February 2013 02:31 PM, Daniel Lezcano wrote: > On 02/21/2013 07:19 AM, Santosh Shilimkar wrote: >> On Tuesday 19 February 2013 11:51 PM, Daniel Lezcano wrote: >>> On 02/19/2013 07:10 PM, Thomas Gleixner wrote: >>>> On Tue, 19 Feb 2013, Daniel Lezcano wrote: >>>>> I am working on identifying the different wakeup sources from the >>>>> interrupts and I have a question regarding the timer broadcast. >>>>> >>>>> The broadcast timer is setup to the next event and that will wake up >>>>> any >>>>> idle cpu belonging to the "broadcast cpumask", right ? >>>>> >>>>> The cpu which has been woken up will look for each cpu the next-event >>>>> and send an IPI to wake it up. >>>>> >>>>> Although, it is possible the sender of this IPI may not be concerned by >>>>> the timer expiration and has been woken up just for sending the IPI, >>>>> right ? >>>> >>>> Correct. >>>> >>>>> If this is correct, is it possible to setup the timer irq affinity to a >>>>> cpu which will be concerned by the timer expiration ? so we prevent an >>>>> unnecessary wake up for a cpu. >>>> >>>> It is possible, but we never implemented it. >>>> >>>> If we go there, we want to make that conditional on a property flag, >>>> because some interrupt controllers especially on x86 only allow to >>>> move the affinity from interrupt context, which is pointless. >>> >>> Thanks Thomas for your quick answer. I will write a RFC patchset. >>> >> Last year I implemented the affinity hook for broad-cast code and >> experimented with it. Since the system I was using was dual core, >> it wasn't much beneficial and hence gave up later. I did remember >> discussing the approach with few folks in the conference. > > I did a brief test with a similar patch on a ARM u8500 board. The timer > is tied with CPU0 by default, setting the dynamic irq affinity reduce > considerably the number of IPI. The difference with your patch is the > affinity is set to one CPU, the first one which is supposed to be wake > up by the timer expiration. > > This is easy to spot with a small program doing usleep wired on CPU1. > > We see CPU0 waking up to send an IPI to CPU1 and going to idle again. > > I don't know how that behaves with OMAP4 with this patch (which I guess > it is the board you used), but the coupled idle state traces could be > ambiguous if you relied on it to check the benefit of this patch. > Across OMAP4 and OMAP5 based devices, only the general purpose OMAP5 devices the approach was useful. Rest of the devices had constraints of master CPU(CPU0) waking up first always which in turns means pining the affinity to that CPU always which the current code already does. That was also another reason I didn't persue it further. > IMO, it is worth to implement such solution and perhaps we can extend it > to optimize the package idle time with the generic power domain tied > with the irq. Anyway, it is a random thought let's see that later :) > It is surely a good optimization especially for multi-core CPUIdle. >> Patch in the end of the email (also attached) for generic broadcast >> code. I didn't look at all corner case though. In arch code then >> you need to setup "broadcast_affinity" hook which should be able >> to get handle of the arch irqchip and call the respective affinity >> handler. Just 3 lines function should do the trick. >> >> As Thomas said, effectiveness of such optimization solely depends >> on how well the affinity (in low powers) supported by your IRQ chip. >> >> Hope this is helpful for you. > > Thanks a lot for your patch and your feedbacks. > Am glad that it was helpful. Regards, Santosh ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] time : pass broadcast device parameter [not found] ` <5123C299.3080005@linaro.org> 2013-02-21 6:19 ` [resend] Timer broadcast question Santosh Shilimkar @ 2013-02-21 22:01 ` Daniel Lezcano 2013-02-21 22:01 ` [PATCH 2/2][RFC] time : set broadcast irq affinity Daniel Lezcano 2013-02-26 8:45 ` [PATCH 1/2] time : pass broadcast device parameter Viresh Kumar 1 sibling, 2 replies; 13+ messages in thread From: Daniel Lezcano @ 2013-02-21 22:01 UTC (permalink / raw) To: linux-arm-kernel The broadcast timer could be passed as parameter to the function instead of using again tick_broadcast_device.evtdev which was previously used in the caller function. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- kernel/time/tick-broadcast.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index f113755..baf9b0e7 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -370,10 +370,9 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void) return to_cpumask(tick_broadcast_oneshot_mask); } -static int tick_broadcast_set_event(ktime_t expires, int force) +static int tick_broadcast_set_event(struct clock_event_device *bc, + ktime_t expires, int force) { - struct clock_event_device *bc = tick_broadcast_device.evtdev; - if (bc->mode != CLOCK_EVT_MODE_ONESHOT) clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); @@ -443,7 +442,7 @@ again: * Rearm the broadcast device. If event expired, * repeat the above */ - if (tick_broadcast_set_event(next_event, 0)) + if (tick_broadcast_set_event(dev, next_event, 0)) goto again; } raw_spin_unlock(&tick_broadcast_lock); @@ -486,7 +485,7 @@ void tick_broadcast_oneshot_control(unsigned long reason) cpumask_set_cpu(cpu, tick_get_broadcast_oneshot_mask()); clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); if (dev->next_event.tv64 < bc->next_event.tv64) - tick_broadcast_set_event(dev->next_event, 1); + tick_broadcast_set_event(bc, dev->next_event, 1); } } else { if (cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) { @@ -555,7 +554,7 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc) clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); tick_broadcast_init_next_event(to_cpumask(tmpmask), tick_next_period); - tick_broadcast_set_event(tick_next_period, 1); + tick_broadcast_set_event(bc, tick_next_period, 1); } else bc->next_event.tv64 = KTIME_MAX; } else { -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2][RFC] time : set broadcast irq affinity 2013-02-21 22:01 ` [PATCH 1/2] time : pass broadcast device parameter Daniel Lezcano @ 2013-02-21 22:01 ` Daniel Lezcano 2013-02-22 17:55 ` Jacob Pan 2013-02-26 8:45 ` [PATCH 1/2] time : pass broadcast device parameter Viresh Kumar 1 sibling, 1 reply; 13+ messages in thread From: Daniel Lezcano @ 2013-02-21 22:01 UTC (permalink / raw) To: linux-arm-kernel When a cpu goes to a deep idle state where its local timer is shutdown, it notifies the time frame work to use the broadcast timer instead. Unfortunately, the broadcast device could wake up any CPU, including an idle one which is not concerned by the wake up at all. This implies, in the worst case, an idle CPU will wake up to send an IPI to another idle cpu. This patch solves this by setting the irq affinity to the cpu concerned by the nearest timer event, by this way, the CPU which is wake up is guarantee to be the one concerned by the next event and we are safe with unnecessary wakeup for another idle CPU. As the irq affinity is not supported by all the archs, a flag is needed to specify which clocksource can handle it. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- include/linux/clockchips.h | 1 + kernel/time/tick-broadcast.c | 39 ++++++++++++++++++++++++++++++++------- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 8a7096f..5cedb27 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -54,6 +54,7 @@ enum clock_event_nofitiers { */ #define CLOCK_EVT_FEAT_C3STOP 0x000008 #define CLOCK_EVT_FEAT_DUMMY 0x000010 +#define CLOCK_EVT_FEAT_DYNIRQ 0x000020 /** * struct clock_event_device - clock event device descriptor diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index baf9b0e7..cbd6737 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -370,13 +370,36 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void) return to_cpumask(tick_broadcast_oneshot_mask); } -static int tick_broadcast_set_event(struct clock_event_device *bc, +/* + * Set broadcast interrupt affinity + */ +static void tick_broadcast_set_affinity(struct clock_event_device *bc, int cpu) +{ + struct cpumask cpumask; + + if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ)) + return; + + cpumask_clear(&cpumask); + cpumask_set_cpu(cpu, &cpumask); + irq_set_affinity(bc->irq, &cpumask); +} + +static int tick_broadcast_set_event(struct clock_event_device *bc, int cpu, ktime_t expires, int force) { + int ret; + if (bc->mode != CLOCK_EVT_MODE_ONESHOT) clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); - return clockevents_program_event(bc, expires, force); + ret = clockevents_program_event(bc, expires, force); + if (ret) + return ret; + + tick_broadcast_set_affinity(bc, cpu); + + return 0; } int tick_resume_broadcast_oneshot(struct clock_event_device *bc) @@ -405,7 +428,7 @@ static void tick_handle_oneshot_broadcast(struct clock_event_device *dev) { struct tick_device *td; ktime_t now, next_event; - int cpu; + int cpu, next_cpu; raw_spin_lock(&tick_broadcast_lock); again: @@ -418,8 +441,10 @@ again: td = &per_cpu(tick_cpu_device, cpu); if (td->evtdev->next_event.tv64 <= now.tv64) cpumask_set_cpu(cpu, to_cpumask(tmpmask)); - else if (td->evtdev->next_event.tv64 < next_event.tv64) + else if (td->evtdev->next_event.tv64 < next_event.tv64) { next_event.tv64 = td->evtdev->next_event.tv64; + next_cpu = cpu; + } } /* @@ -442,7 +467,7 @@ again: * Rearm the broadcast device. If event expired, * repeat the above */ - if (tick_broadcast_set_event(dev, next_event, 0)) + if (tick_broadcast_set_event(dev, next_cpu, next_event, 0)) goto again; } raw_spin_unlock(&tick_broadcast_lock); @@ -485,7 +510,7 @@ void tick_broadcast_oneshot_control(unsigned long reason) cpumask_set_cpu(cpu, tick_get_broadcast_oneshot_mask()); clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); if (dev->next_event.tv64 < bc->next_event.tv64) - tick_broadcast_set_event(bc, dev->next_event, 1); + tick_broadcast_set_event(bc, cpu, dev->next_event, 1); } } else { if (cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) { @@ -554,7 +579,7 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc) clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); tick_broadcast_init_next_event(to_cpumask(tmpmask), tick_next_period); - tick_broadcast_set_event(bc, tick_next_period, 1); + tick_broadcast_set_event(bc, cpu, tick_next_period, 1); } else bc->next_event.tv64 = KTIME_MAX; } else { -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2][RFC] time : set broadcast irq affinity 2013-02-21 22:01 ` [PATCH 2/2][RFC] time : set broadcast irq affinity Daniel Lezcano @ 2013-02-22 17:55 ` Jacob Pan 2013-02-22 18:45 ` Thomas Gleixner 2013-02-25 22:50 ` Daniel Lezcano 0 siblings, 2 replies; 13+ messages in thread From: Jacob Pan @ 2013-02-22 17:55 UTC (permalink / raw) To: linux-arm-kernel On Thu, 21 Feb 2013 23:01:23 +0100 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > +/* > + * Set broadcast interrupt affinity > + */ > +static void tick_broadcast_set_affinity(struct clock_event_device > *bc, int cpu) +{ > + struct cpumask cpumask; > + > + if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ)) > + return; > + > + cpumask_clear(&cpumask); > + cpumask_set_cpu(cpu, &cpumask); > + irq_set_affinity(bc->irq, &cpumask); would it be more efficient to keep track of the current bc->irq affinity via cpumask then set it only if it is different? -- Thanks, Jacob ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2][RFC] time : set broadcast irq affinity 2013-02-22 17:55 ` Jacob Pan @ 2013-02-22 18:45 ` Thomas Gleixner 2013-02-25 22:50 ` Daniel Lezcano 1 sibling, 0 replies; 13+ messages in thread From: Thomas Gleixner @ 2013-02-22 18:45 UTC (permalink / raw) To: linux-arm-kernel On Fri, 22 Feb 2013, Jacob Pan wrote: > On Thu, 21 Feb 2013 23:01:23 +0100 > Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > +/* > > + * Set broadcast interrupt affinity > > + */ > > +static void tick_broadcast_set_affinity(struct clock_event_device > > *bc, int cpu) +{ > > + struct cpumask cpumask; > > + > > + if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ)) > > + return; > > + > > + cpumask_clear(&cpumask); > > + cpumask_set_cpu(cpu, &cpumask); > > + irq_set_affinity(bc->irq, &cpumask); > would it be more efficient to keep track of the current bc->irq affinity > via cpumask then set it only if it is different? You beat me :) ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2][RFC] time : set broadcast irq affinity 2013-02-22 17:55 ` Jacob Pan 2013-02-22 18:45 ` Thomas Gleixner @ 2013-02-25 22:50 ` Daniel Lezcano 2013-02-25 23:00 ` Jacob Pan 1 sibling, 1 reply; 13+ messages in thread From: Daniel Lezcano @ 2013-02-25 22:50 UTC (permalink / raw) To: linux-arm-kernel On 02/22/2013 06:55 PM, Jacob Pan wrote: > On Thu, 21 Feb 2013 23:01:23 +0100 > Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> +/* >> + * Set broadcast interrupt affinity >> + */ >> +static void tick_broadcast_set_affinity(struct clock_event_device >> *bc, int cpu) +{ >> + struct cpumask cpumask; >> + >> + if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ)) >> + return; >> + >> + cpumask_clear(&cpumask); >> + cpumask_set_cpu(cpu, &cpumask); >> + irq_set_affinity(bc->irq, &cpumask); > would it be more efficient to keep track of the current bc->irq affinity > via cpumask then set it only if it is different? Do you mean a cpumask static variable ? and something like: if (!cpumask_test_cpu(cpu, &affinitymask)) { cpumask_set_cpu(cpu, &affinitymask); irq_set_affinity(bc->irq, &affinitymask) } -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2][RFC] time : set broadcast irq affinity 2013-02-25 22:50 ` Daniel Lezcano @ 2013-02-25 23:00 ` Jacob Pan 0 siblings, 0 replies; 13+ messages in thread From: Jacob Pan @ 2013-02-25 23:00 UTC (permalink / raw) To: linux-arm-kernel On Mon, 25 Feb 2013 23:50:23 +0100 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > Do you mean a cpumask static variable ? and something like: > > if (!cpumask_test_cpu(cpu, &affinitymask)) { > cpumask_set_cpu(cpu, &affinitymask); > irq_set_affinity(bc->irq, &affinitymask) > } yeah. but i think you can use the cpumask in struct clock_event_device. -- Thanks, Jacob ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] time : pass broadcast device parameter 2013-02-21 22:01 ` [PATCH 1/2] time : pass broadcast device parameter Daniel Lezcano 2013-02-21 22:01 ` [PATCH 2/2][RFC] time : set broadcast irq affinity Daniel Lezcano @ 2013-02-26 8:45 ` Viresh Kumar 2013-02-26 11:30 ` Daniel Lezcano 2013-02-26 11:31 ` Daniel Lezcano 1 sibling, 2 replies; 13+ messages in thread From: Viresh Kumar @ 2013-02-26 8:45 UTC (permalink / raw) To: linux-arm-kernel On 22 February 2013 03:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > The broadcast timer could be passed as parameter to the function > instead of using again tick_broadcast_device.evtdev which was > previously used in the caller function. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> I know you are going for another round with this patchset and was just trying v1. I did my tests on ARM Vexpress - TC2, big.LITTLE Arch. Tested-by: Viresh Kumar <viresh.kumar@linaro.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] time : pass broadcast device parameter 2013-02-26 8:45 ` [PATCH 1/2] time : pass broadcast device parameter Viresh Kumar @ 2013-02-26 11:30 ` Daniel Lezcano 2013-02-26 11:31 ` Daniel Lezcano 1 sibling, 0 replies; 13+ messages in thread From: Daniel Lezcano @ 2013-02-26 11:30 UTC (permalink / raw) To: linux-arm-kernel On 02/26/2013 09:45 AM, Viresh Kumar wrote: > On 22 February 2013 03:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> The broadcast timer could be passed as parameter to the function >> instead of using again tick_broadcast_device.evtdev which was >> previously used in the caller function. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > I know you are going for another round with this patchset and was just > trying v1. > > I did my tests on ARM Vexpress - TC2, big.LITTLE Arch. > > Tested-by: Viresh Kumar <viresh.kumar@linaro.org> Thanks Viresh for testing. -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] time : pass broadcast device parameter 2013-02-26 8:45 ` [PATCH 1/2] time : pass broadcast device parameter Viresh Kumar 2013-02-26 11:30 ` Daniel Lezcano @ 2013-02-26 11:31 ` Daniel Lezcano 2013-02-26 12:14 ` Viresh Kumar 1 sibling, 1 reply; 13+ messages in thread From: Daniel Lezcano @ 2013-02-26 11:31 UTC (permalink / raw) To: linux-arm-kernel On 02/26/2013 09:45 AM, Viresh Kumar wrote: > On 22 February 2013 03:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> The broadcast timer could be passed as parameter to the function >> instead of using again tick_broadcast_device.evtdev which was >> previously used in the caller function. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > I know you are going for another round with this patchset and was just > trying v1. > > I did my tests on ARM Vexpress - TC2, big.LITTLE Arch. > > Tested-by: Viresh Kumar <viresh.kumar@linaro.org> Oh, by the way, could send me the patch to set the flag to the timer device ? I will include it to the patchset. Thanks -- Daniel -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] time : pass broadcast device parameter 2013-02-26 11:31 ` Daniel Lezcano @ 2013-02-26 12:14 ` Viresh Kumar 0 siblings, 0 replies; 13+ messages in thread From: Viresh Kumar @ 2013-02-26 12:14 UTC (permalink / raw) To: linux-arm-kernel On 26 February 2013 17:01, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > Oh, by the way, could send me the patch to set the flag to the timer > device ? I will include it to the patchset. Sure. Find it attached too as gmail may break it. commit 14422c760bb5b2485867f3efb7842d296081ad86 Author: Viresh Kumar <viresh.kumar@linaro.org> Date: Fri Feb 22 12:42:39 2013 +0530 ARM/timer-sp: Set dynamic irq affinity When a cpu goes to a deep idle state where its local timer is shutdown, it notifies the time frame work to use the broadcast timer instead. Unfortunately, the broadcast device could wake up any CPU, including an idle one which is not concerned by the wake up at all. This implies, in the worst case, an idle CPU will wake up to send an IPI to another idle cpu. This patch fixes this for ARM platforms using timer-sp, by setting CLOCK_EVT_FEAT_DYNIRQ feature. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- arch/arm/common/timer-sp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index 9d2d3ba..ae3c0f9 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -158,7 +158,8 @@ static int sp804_set_next_event(unsigned long next, } static struct clock_event_device sp804_clockevent = { - .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | + CLOCK_EVT_FEAT_DYNIRQ, .set_mode = sp804_set_mode, .set_next_event = sp804_set_next_event, .rating = 300, -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-ARM-timer-sp-Set-dynamic-irq-affinity.patch Type: application/octet-stream Size: 1493 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130226/78fd6ceb/attachment.obj> ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-02-26 12:14 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5123BE35.8070902@linaro.org>
[not found] ` <alpine.LFD.2.02.1302191904560.22263@ionos>
[not found] ` <5123C299.3080005@linaro.org>
2013-02-21 6:19 ` [resend] Timer broadcast question Santosh Shilimkar
2013-02-21 9:01 ` Daniel Lezcano
2013-02-21 9:14 ` Santosh Shilimkar
2013-02-21 22:01 ` [PATCH 1/2] time : pass broadcast device parameter Daniel Lezcano
2013-02-21 22:01 ` [PATCH 2/2][RFC] time : set broadcast irq affinity Daniel Lezcano
2013-02-22 17:55 ` Jacob Pan
2013-02-22 18:45 ` Thomas Gleixner
2013-02-25 22:50 ` Daniel Lezcano
2013-02-25 23:00 ` Jacob Pan
2013-02-26 8:45 ` [PATCH 1/2] time : pass broadcast device parameter Viresh Kumar
2013-02-26 11:30 ` Daniel Lezcano
2013-02-26 11:31 ` Daniel Lezcano
2013-02-26 12:14 ` Viresh Kumar
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).