Timer broadcast question

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 ?

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.

Quoted 11 lines · Daniel Lezcano

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.

Quoted 3 lines

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.

Quoted 19 lines · Thomas Gleixner

Quoted 11 lines · Daniel Lezcano

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 26 lines · Daniel Lezcano

Quoted 24 lines · Thomas Gleixner

Quoted 11 lines · Daniel Lezcano

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.

Quoted 3 lines

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.

Quoted 1 line

Thanks Thomas for your quick answer. I will write a RFC patchset.

I'm curious what the use case is. I played with this code awhile ago, and AFAICT it's not used on sensible (i.e. modern) systems. Is there anything other than old x86 machines that needs it?

Quoted 19 lines · Thomas Gleixner

Quoted 11 lines · Daniel Lezcano

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

From d70f2d48ec08a3f1d73187c49b16e4e60f81a50c Mon Sep 17 00:00:00 2001

Quoted 28 lines · Daniel Lezcano

Quoted 26 lines · Thomas Gleixner

Quoted 7 lines · Daniel Lezcano

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 ?

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

From d70f2d48ec08a3f1d73187c49b16e4e60f81a50c Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <[email protected]>
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 <[email protected]>

---
 include/linux/clockchips.h   |    2 ++
 kernel/time/tick-broadcast.c |   13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

include/linux/clockchips.h +2 -0

index 8a7096f..5488cdc 100644
@@ -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 *);

kernel/time/tick-broadcast.c +8 -1

index f113755..2ec2425 100644
@@ -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);

         }

kernel/time/tick-broadcast.c +4 -0

@@ -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;

Quoted 29 lines · Santosh Shilimkar

Quoted 28 lines · Daniel Lezcano

Quoted 26 lines · Thomas Gleixner

Quoted 7 lines · Daniel Lezcano

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 ?

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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 :)

Quoted 6 lines

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.

Quoted 4 lines

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.

kernel/time/tick-broadcast.c struct cpumask *tick_get_broadcast_oneshot_mask(void) · line 385

Quoted 13 lines · On Thu, 21 Feb 2013 23:01:23 +0100 Daniel Lezcano <[email protected]>…

kernel/time/tick-broadcast.c +4 -0

+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device

*bc, int cpu) +{

kernel/time/tick-broadcast.c +8 -0

+	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?

kernel/time/tick-broadcast.c struct cpumask *tick_get_broadcast_oneshot_mask(void) · line 385

Quoted 13 lines · On Thu, 21 Feb 2013 23:01:23 +0100 Daniel Lezcano <[email protected]>…

kernel/time/tick-broadcast.c +4 -0

+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device

*bc, int cpu) +{

kernel/time/tick-broadcast.c +8 -0

+	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)
}

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 <[email protected]>

Quoted 4 lines · Daniel Lezcano

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.

Quoted 1 line

Signed-off-by Daniel Lezcano <[email protected]>

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 <[email protected]>

Quoted 4 lines · Daniel Lezcano

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.

Quoted 1 line

Signed-off-by Daniel Lezcano <[email protected]>

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 <[email protected]>

Oh, by the way, could send me the patch to set the flag to the timer device ? I will include it to the patchset.