Re: [PATCH 2/4] time : set broadcast irq affinity

On Wednesday 27 February 2013 03:47 AM, Daniel Lezcano wrote:

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.

Minor. Can mention the flag name as well here "CLOCK_EVT_FEAT_DYNIRQ"

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 6634652..c256cea 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

Please add some comments about the usage of the flag.

/**

• struct clock_event_device - clock event device descriptor

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 6197ac0..1f7b4f4 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -406,13 +406,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) +{

Better is just make second parameter as cpu_mask rather than CPU cpu number. Its a semantic of affinity hook which you can easily retain.

• if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ))

• return;
  

• if (cpumask_equal(bc->cpumask, cpumask_of(cpu)))

• return;
  

• bc->cpumask = cpumask_of(cpu);

You can avoid the cpumask_of() couple of times above.

• irq_set_affinity(bc->irq, bc->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);

In case you go by cpumask paramater, then above can be just tick_broadcast_set_affinity(bc, cpumask_of(cpu));

• return 0; }

  int tick_resume_broadcast_oneshot(struct clock_event_device *bc)

@@ -441,7 +464,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:

@@ -454,8 +477,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;
  

• }
  

  }

  /*

@@ -478,7 +503,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);

@@ -521,7 +546,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);
  

Since you have embedded the irq_affinity() in above function, the IRQ affinity for bc->irq will remain to the last CPU on which the interrupt fired. In general it should be fine but would be good if you clear it on CLOCK_EVT_NOTIFY_BROADCAST_EXIT. Not a must have though.

Regards, Santosh