[PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Use an atomic_notifier instead of a raw_notifier for the clockevents
notification.

This avoids a global lock in the idle path, is a scalability
problem. With this patch we don't have a global lock anymore
at least on systems with an always running timer.

Only broadcast_enter/exit actually do use RCU for now.

I kept all the other events under the lock because they are not
on fast paths and it was not fully clear if they are all RCU
safe or not.

Actually an alternative would be to just get rid of the notifier.
As far as I can see it only has a single client anyways. But
right now I kept it.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 kernel/time/clockevents.c |   29 +++++++++++++----------------
 1 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index e4c699d..2ee6bcb 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -25,12 +25,12 @@
 static LIST_HEAD(clockevent_devices);
 static LIST_HEAD(clockevents_released);
 
-/* Notification for clock events */
-static RAW_NOTIFIER_HEAD(clockevents_chain);
-
 /* Protection for the above */
 static DEFINE_RAW_SPINLOCK(clockevents_lock);
 
+/* Notification for clock events */
+static ATOMIC_NOTIFIER_HEAD(clockevents_chain);
+
 /**
  * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
  * @latch:	value to convert
@@ -137,23 +137,12 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
  */
 int clockevents_register_notifier(struct notifier_block *nb)
 {
-	unsigned long flags;
-	int ret;
-
-	raw_spin_lock_irqsave(&clockevents_lock, flags);
-	ret = raw_notifier_chain_register(&clockevents_chain, nb);
-	raw_spin_unlock_irqrestore(&clockevents_lock, flags);
-
-	return ret;
+	return atomic_notifier_chain_register(&clockevents_chain, nb);
 }
 
-/*
- * Notify about a clock event change. Called with clockevents_lock
- * held.
- */
 static void clockevents_do_notify(unsigned long reason, void *dev)
 {
-	raw_notifier_call_chain(&clockevents_chain, reason, dev);
+	atomic_notifier_call_chain(&clockevents_chain, reason, dev);
 }
 
 /*
@@ -308,6 +297,14 @@ void clockevents_notify(unsigned long reason, void *arg)
 	unsigned long flags;
 	int cpu;
 
+	/* For idle path events don't take a lock */
+
+	if (reason == CLOCK_EVT_NOTIFY_BROADCAST_ENTER ||
+	    reason == CLOCK_EVT_NOTIFY_BROADCAST_EXIT) {
+		clockevents_do_notify(reason, arg);
+		return;
+	}
+
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
 	clockevents_do_notify(reason, arg);
 
-- 
1.7.4.4

[PATCH 2/3] broadcast-tick: Move oneshot broadcast mask to per cpu variables v2

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Avoid a global cache line hotspot in the oneshot cpu mask. Maintain
this information in per cpu variables instead.

v2: use DEFINE_PER_CPU_ALIGNED (Eric Dumazet)

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 include/linux/tick.h         |    2 +-
 kernel/time/tick-broadcast.c |   40 ++++++++++++++++++++++++----------------
 kernel/time/timer_list.c     |   11 +++++++++--
 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index b232ccc..3df6e2e 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -90,7 +90,7 @@ extern struct tick_device *tick_get_broadcast_device(void);
 extern struct cpumask *tick_get_broadcast_mask(void);
 
 #  ifdef CONFIG_TICK_ONESHOT
-extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
+extern void tick_get_broadcast_oneshot_mask(struct cpumask *);
 #  endif
 
 # endif /* BROADCAST */
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index c7218d1..54a5977 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -358,15 +358,22 @@ int tick_resume_broadcast(void)
 
 #ifdef CONFIG_TICK_ONESHOT
 
-/* FIXME: use cpumask_var_t. */
-static DECLARE_BITMAP(tick_broadcast_oneshot_mask, NR_CPUS);
+struct broadcast_cpu_state {
+	int need_oneshot;
+};
+static DEFINE_PER_CPU_ALIGNED(struct broadcast_cpu_state, state);
 
 /*
  * Exposed for debugging: see timer_list.c
  */
-struct cpumask *tick_get_broadcast_oneshot_mask(void)
+void tick_get_broadcast_oneshot_mask(struct cpumask *mask)
 {
-	return to_cpumask(tick_broadcast_oneshot_mask);
+	int i;
+
+	for_each_online_cpu (i) {
+		if (per_cpu(state, i).need_oneshot)
+			cpumask_set_cpu(i, mask);
+	}
 }
 
 static int tick_broadcast_set_event(ktime_t expires, int force)
@@ -388,7 +395,7 @@ int tick_resume_broadcast_oneshot(struct clock_event_device *bc)
  */
 void tick_check_oneshot_broadcast(int cpu)
 {
-	if (cpumask_test_cpu(cpu, to_cpumask(tick_broadcast_oneshot_mask))) {
+	if (per_cpu(state, cpu).need_oneshot) {
 		struct tick_device *td = &per_cpu(tick_cpu_device, cpu);
 
 		clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_ONESHOT);
@@ -411,7 +418,9 @@ again:
 	cpumask_clear(to_cpumask(tmpmask));
 	now = ktime_get();
 	/* Find all expired events */
-	for_each_cpu(cpu, tick_get_broadcast_oneshot_mask()) {
+	for_each_online_cpu(cpu) {
+		if (!per_cpu(state, cpu).need_oneshot)
+			continue;
 		td = &per_cpu(tick_cpu_device, cpu);
 		if (td->evtdev->next_event.tv64 <= now.tv64)
 			cpumask_set_cpu(cpu, to_cpumask(tmpmask));
@@ -478,16 +487,15 @@ void tick_broadcast_oneshot_control(unsigned long reason)
 
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 	if (reason == CLOCK_EVT_NOTIFY_BROADCAST_ENTER) {
-		if (!cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) {
-			cpumask_set_cpu(cpu, tick_get_broadcast_oneshot_mask());
+		if (!__get_cpu_var(state).need_oneshot) {
+			__get_cpu_var(state).need_oneshot = 1;
 			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);
 		}
 	} else {
-		if (cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) {
-			cpumask_clear_cpu(cpu,
-					  tick_get_broadcast_oneshot_mask());
+		if (__get_cpu_var(state).need_oneshot) {
+			__get_cpu_var(state).need_oneshot = 0;
 			clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT);
 			if (dev->next_event.tv64 != KTIME_MAX)
 				tick_program_event(dev->next_event, 1);
@@ -503,7 +511,7 @@ void tick_broadcast_oneshot_control(unsigned long reason)
  */
 static void tick_broadcast_clear_oneshot(int cpu)
 {
-	cpumask_clear_cpu(cpu, tick_get_broadcast_oneshot_mask());
+	per_cpu(state, cpu).need_oneshot = 0;
 }
 
 static void tick_broadcast_init_next_event(struct cpumask *mask,
@@ -529,6 +537,7 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc)
 	/* Set it up only once ! */
 	if (bc->event_handler != tick_handle_oneshot_broadcast) {
 		int was_periodic = bc->mode == CLOCK_EVT_MODE_PERIODIC;
+		int i;
 
 		bc->event_handler = tick_handle_oneshot_broadcast;
 		clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT);
@@ -544,9 +553,8 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc)
 		 */
 		cpumask_copy(to_cpumask(tmpmask), tick_get_broadcast_mask());
 		cpumask_clear_cpu(cpu, to_cpumask(tmpmask));
-		cpumask_or(tick_get_broadcast_oneshot_mask(),
-			   tick_get_broadcast_oneshot_mask(),
-			   to_cpumask(tmpmask));
+		for_each_cpu (i, to_cpumask(tmpmask))
+			per_cpu(state, i).need_oneshot = 1;
 
 		if (was_periodic && !cpumask_empty(to_cpumask(tmpmask))) {
 			tick_broadcast_init_next_event(to_cpumask(tmpmask),
@@ -598,7 +606,7 @@ void tick_shutdown_broadcast_oneshot(unsigned int *cpup)
 	 * Clear the broadcast mask flag for the dead cpu, but do not
 	 * stop the broadcast device!
 	 */
-	cpumask_clear_cpu(cpu, tick_get_broadcast_oneshot_mask());
+	per_cpu(state, cpu).need_oneshot = 0;
 
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 3258455..28964ed 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -235,14 +235,21 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu)
 static void timer_list_show_tickdevices(struct seq_file *m)
 {
 	int cpu;
+#ifdef CONFIG_TICK_ONESHOT
+	cpumask_var_t mask;
+#endif
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 	print_tickdevice(m, tick_get_broadcast_device(), -1);
 	SEQ_printf(m, "tick_broadcast_mask: %08lx\n",
 		   cpumask_bits(tick_get_broadcast_mask())[0]);
 #ifdef CONFIG_TICK_ONESHOT
-	SEQ_printf(m, "tick_broadcast_oneshot_mask: %08lx\n",
-		   cpumask_bits(tick_get_broadcast_oneshot_mask())[0]);
+	if (zalloc_cpumask_var(&mask, GFP_KERNEL) == 0) {
+		tick_get_broadcast_oneshot_mask(mask);
+		SEQ_printf(m, "tick_broadcast_oneshot_mask: %08lx\n",
+			   cpumask_bits(mask)[0]);
+		free_cpumask_var(mask);
+	}
 #endif
 	SEQ_printf(m, "\n");
 #endif
-- 
1.7.4.4

[PATCH 3/3] tick-broadcast: push down tick_broadcast_lock

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

For the oneshot case, only take the tick_broadcast_lock when the
global device is actually changing. For the case when the new
event is only setting the wakeup to a later time than it already
is we don't need the lock.

This avoids lock contention for some special cases on systems
that don't have an always running per cpu timer. It's not a full
solution to the scalability problem there unfortunately, just
the first step.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 kernel/time/tick-broadcast.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 54a5977..7e748fb 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -485,23 +485,33 @@ void tick_broadcast_oneshot_control(unsigned long reason)
 
 	bc = tick_broadcast_device.evtdev;
 
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 	if (reason == CLOCK_EVT_NOTIFY_BROADCAST_ENTER) {
 		if (!__get_cpu_var(state).need_oneshot) {
 			__get_cpu_var(state).need_oneshot = 1;
 			clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN);
-			if (dev->next_event.tv64 < bc->next_event.tv64)
+		       
+			/* Only take the lock if the events gets set earlier */
+			if (dev->next_event.tv64 < bc->next_event.tv64) {
+				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 				tick_broadcast_set_event(dev->next_event, 1);
+				raw_spin_unlock_irqrestore(&tick_broadcast_lock, 
+							   flags);		
+			}
 		}
 	} else {
 		if (__get_cpu_var(state).need_oneshot) {
 			__get_cpu_var(state).need_oneshot = 0;
 			clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT);
-			if (dev->next_event.tv64 != KTIME_MAX)
+
+			/* Only take the lock if the event changes */
+			if (dev->next_event.tv64 != KTIME_MAX) {
+				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 				tick_program_event(dev->next_event, 1);
+				raw_spin_unlock_irqrestore(&tick_broadcast_lock, 
+							   flags);
+			}
 		}
 	}
-	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 
 /*
-- 
1.7.4.4

Re: [PATCH 2/3] broadcast-tick: Move oneshot broadcast mask to per cpu variables v2

[Thomas Gleixner]

On Mon, 29 Aug 2011, Andi Kleen wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> Avoid a global cache line hotspot in the oneshot cpu mask. Maintain
> this information in per cpu variables instead.

<SNIP>

> @@ -411,7 +418,9 @@ again:
>  	cpumask_clear(to_cpumask(tmpmask));
>  	now = ktime_get();
>  	/* Find all expired events */
> -	for_each_cpu(cpu, tick_get_broadcast_oneshot_mask()) {
> +	for_each_online_cpu(cpu) {
> +		if (!per_cpu(state, cpu).need_oneshot)
> +			continue;

So we iterate over each online CPU instead. I'm really not convinced
that this is better performing especially if the number of CPUs in
broadcast mode is way smaller than the number of online CPUs.

Also having a separate per cpu variable which is just a boolean marker
is silly. If we really want move that to per cpu storage then the
marker should simply be the expiry time of that CPU so you don't have
to evaluate two per cpu variables, which are in completely different
cachelines. If a CPU is not in that mode that expiry time should
simply read KTIME_MAX.

But again, I'm not convinced that iterating over a large number of
CPUs to find a single one in oneshot mode is a good idea.

Thanks,

	tglx

Re: [PATCH 3/3] tick-broadcast: push down tick_broadcast_lock

[Thomas Gleixner]

On Mon, 29 Aug 2011, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> For the oneshot case, only take the tick_broadcast_lock when the
> global device is actually changing. For the case when the new
> event is only setting the wakeup to a later time than it already
> is we don't need the lock.
> 
> This avoids lock contention for some special cases on systems
> that don't have an always running per cpu timer. It's not a full
> solution to the scalability problem there unfortunately, just
> the first step.

There is no full solution to that problem other than using sane
hardware.

> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  kernel/time/tick-broadcast.c |   18 ++++++++++++++----
>  1 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index 54a5977..7e748fb 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -485,23 +485,33 @@ void tick_broadcast_oneshot_control(unsigned long reason)
>  
>  	bc = tick_broadcast_device.evtdev;
>  
> -	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
>  	if (reason == CLOCK_EVT_NOTIFY_BROADCAST_ENTER) {
>  		if (!__get_cpu_var(state).need_oneshot) {
>  			__get_cpu_var(state).need_oneshot = 1;
>  			clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN);
> -			if (dev->next_event.tv64 < bc->next_event.tv64)
> +		       
> +			/* Only take the lock if the events gets set earlier */
> +			if (dev->next_event.tv64 < bc->next_event.tv64) {

That's racy and broken.

CPU0                                 CPU1

tick_handle_oneshot_broadcast()

 raw_spin_lock(&tick_broadcast_lock);
 bc->next_event = KTIME_MAX;
 for_each_online_cpu() {
	next_event = ...;
 }
 ....                                if (dev->next_event < bc->next_event) {
                                       raw_spin_lock(&tick_broadcast_lock);

 tick_broadcast_set_event(next_event, 0);
   bc->next_event = next_event;

 raw_spin_unlock(&tick_broadcast_lock);
                                       tick_broadcast_set_event(dev->next_event, 1);

So you unconditionally set the broadcast device to dev->next_event of
CPU1 even if the current pending event which was evaluated on CPU0 is
_BEFORE_ the CPU1 event. That can cause stalls and other hard to debug
horror. We've been there before.

Further the unprotected comparison on 32bit is completely bogus.

> +				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
>  				tick_broadcast_set_event(dev->next_event, 1);
> +				raw_spin_unlock_irqrestore(&tick_broadcast_lock, 
> +							   flags);		
> +			}
>  		}
>  	} else {
>  		if (__get_cpu_var(state).need_oneshot) {
>  			__get_cpu_var(state).need_oneshot = 0;
>  			clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT);
> -			if (dev->next_event.tv64 != KTIME_MAX)
> +
> +			/* Only take the lock if the event changes */
> +			if (dev->next_event.tv64 != KTIME_MAX) {
> +				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);

Why would you take the global lock to program the cpu local device?
Just because it happened to be under that lock before?

>  				tick_program_event(dev->next_event, 1);
> +				raw_spin_unlock_irqrestore(&tick_broadcast_lock, 
> +							   flags);

Thanks,

	tglx

Re: [PATCH 3/3] tick-broadcast: push down tick_broadcast_lock

[Andi Kleen]

On Tue, Sep 06, 2011 at 06:19:00PM +0200, Thomas Gleixner wrote:

> 
> There is no full solution to that problem other than using sane
> hardware.

Not convinced.

BTW can you at least merge the first patch for the notifiers. 
This fixes the "fixed hardware" which is currently broken too.

>  raw_spin_lock(&tick_broadcast_lock);
>  bc->next_event = KTIME_MAX;
>  for_each_online_cpu() {
> 	next_event = ...;
>  }
>  ....                                if (dev->next_event < bc->next_event) {
>                                        raw_spin_lock(&tick_broadcast_lock);
> 
>  tick_broadcast_set_event(next_event, 0);
>    bc->next_event = next_event;
> 
>  raw_spin_unlock(&tick_broadcast_lock);
>                                        tick_broadcast_set_event(dev->next_event, 1);
> 
> So you unconditionally set the broadcast device to dev->next_event of
> CPU1 even if the current pending event which was evaluated on CPU0 is
> _BEFORE_ the CPU1 event. That can cause stalls and other hard to debug
> horror. We've been there before.

I don't understand. It only sets it if the new event is earlier.
So it can never be set back.

You seem to say the opposite?

> 
> Further the unprotected comparison on 32bit is completely bogus.

Ok.  Just need a ordered read like i_size_read().

> > -			if (dev->next_event.tv64 != KTIME_MAX)
> > +
> > +			/* Only take the lock if the event changes */
> > +			if (dev->next_event.tv64 != KTIME_MAX) {
> > +				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> 
> Why would you take the global lock to program the cpu local device?
> Just because it happened to be under that lock before?

Yes, I didn't audit that code. But probably it can be dropped
you're right.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 3/3] tick-broadcast: push down tick_broadcast_lock

[Thomas Gleixner]

On Tue, 6 Sep 2011, Andi Kleen wrote:

> On Tue, Sep 06, 2011 at 06:19:00PM +0200, Thomas Gleixner wrote:
> 
> > 
> > There is no full solution to that problem other than using sane
> > hardware.
> 
> Not convinced.
> 
> BTW can you at least merge the first patch for the notifiers. 
> This fixes the "fixed hardware" which is currently broken too.
> 
> >  raw_spin_lock(&tick_broadcast_lock);
> >  bc->next_event = KTIME_MAX;
> >  for_each_online_cpu() {
> > 	next_event = ...;
> >  }
> >  ....                                if (dev->next_event < bc->next_event) {
> >                                        raw_spin_lock(&tick_broadcast_lock);
> > 
> >  tick_broadcast_set_event(next_event, 0);
> >    bc->next_event = next_event;
> > 
> >  raw_spin_unlock(&tick_broadcast_lock);
> >                                        tick_broadcast_set_event(dev->next_event, 1);
> > 
> > So you unconditionally set the broadcast device to dev->next_event of
> > CPU1 even if the current pending event which was evaluated on CPU0 is
> > _BEFORE_ the CPU1 event. That can cause stalls and other hard to debug
> > horror. We've been there before.
> 
> I don't understand. It only sets it if the new event is earlier.
> So it can never be set back.

If you read the above you see that the broadcast handler sets the next
event to KTIME_MAX first. Then it looks for the next event and
programs it to that expiry time.

So when the check on the other CPU happens after CPU0 does
bc->next_event = KTIME_MAX and before CPU0 finished reprogramming,
then CPU1 sees KTIME_MAX and the comparison evaluates true. So CPU1
waits for the lock which is held by CPU0 and then sets the BC device
to CPU1 next event unconditionally, which might be _AFTER_ the already
pending event which was set by CPU0.
 
> You seem to say the opposite?

No, that's racy because you do not hold the lock when doing the
comparision.
 
> > 
> > Further the unprotected comparison on 32bit is completely bogus.
> 
> Ok.  Just need a ordered read like i_size_read().
> 
> > > -			if (dev->next_event.tv64 != KTIME_MAX)
> > > +
> > > +			/* Only take the lock if the event changes */
> > > +			if (dev->next_event.tv64 != KTIME_MAX) {
> > > +				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> > 
> > Why would you take the global lock to program the cpu local device?
> > Just because it happened to be under that lock before?
> 
> Yes, I didn't audit that code. But probably it can be dropped
> you're right.

That's not a question of auditing, it's a question of understanding
the code which you modify.

Thanks,

	tglx

Re: [PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[Thomas Gleixner]

On Mon, 29 Aug 2011, Andi Kleen wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> Use an atomic_notifier instead of a raw_notifier for the clockevents
> notification.
> 
> This avoids a global lock in the idle path, is a scalability
> problem. With this patch we don't have a global lock anymore
> at least on systems with an always running timer.

And why is that code called on a system with an always running local
apic timer at all? The broadcast horror is explicitely only for those
systems with wreckaged hardware.
 
Either the patch makes no sense or the changelog or both.

Thanks,

	tglx

Re: [PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[Andi Kleen]

On Tue, Sep 06, 2011 at 09:23:55PM +0200, Thomas Gleixner wrote:
> On Mon, 29 Aug 2011, Andi Kleen wrote:
> 
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Use an atomic_notifier instead of a raw_notifier for the clockevents
> > notification.
> > 
> > This avoids a global lock in the idle path, is a scalability
> > problem. With this patch we don't have a global lock anymore
> > at least on systems with an always running timer.
> 
> And why is that code called on a system with an always running local
> apic timer at all? The broadcast horror is explicitely only for those
> systems with wreckaged hardware.

Because the notifier is before the check for always running local apic timer.

clockevents_notify ->
clockevents_do_notify ->           
	raw_spin_lock_irqsave(&clockevents_lock, flags);
	clockevents_do_notify(reason, arg);

tick_notifier ->
tick_notify -> 
tick_broadcast_on_off -> 
tick_do_broadcast_on_off

	/* check */
	if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP))
		goto out;

You also get bonus points for a incredible convoluted call chain,
about 50% of it being unnecessary and full of scalability problems.

-Andi

Re: [PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[Thomas Gleixner]

On Tue, 6 Sep 2011, Andi Kleen wrote:

> On Tue, Sep 06, 2011 at 09:23:55PM +0200, Thomas Gleixner wrote:
> > On Mon, 29 Aug 2011, Andi Kleen wrote:
> > 
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > Use an atomic_notifier instead of a raw_notifier for the clockevents
> > > notification.
> > > 
> > > This avoids a global lock in the idle path, is a scalability
> > > problem. With this patch we don't have a global lock anymore
> > > at least on systems with an always running timer.
> > 
> > And why is that code called on a system with an always running local
> > apic timer at all? The broadcast horror is explicitely only for those
> > systems with wreckaged hardware.
> 
> Because the notifier is before the check for always running local apic timer.

The idle code, at least the one in drivers/idle/intel_idle.c, does
_NOT_ call into that code at all when a reliable lapic timer is
available:

        if (!(lapic_timer_reliable_states & (1 << (cstate))))
	   clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);

....

	if (boot_cpu_has(X86_FEATURE_ARAT))     /* Always Reliable APIC Timer */
                lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;

Same applies for drivers/acpi/processor_idle.c

Though according to your changelog something is calling that
clockevents callchain despite of an ALWAYS RUNNING local apic
timer. That means:

Either you are using different code or your machine does not have that
ARAT flag set.

 - If your machine has the ARAT feature, then calling into the
   clockevents code is fcking wrong.

 - If your machine does NOT have the ARAT feature, then removing that
   lock is nice, but has ABSOLUTELY NOTHING TO DO WITH AN ALWAYS
   RUNNING TIMER.

> You also get bonus points for a incredible convoluted call chain,
> about 50% of it being unnecessary and full of scalability problems.

I gladly accept those bonus points for writing correct code, which was
never meant to be scalable on larger systems at all. Assuming that
HPET broadcasting is scalable in any way is just hilarious.

I'm sorry that I'm not able to return the favour of bonus points for
you. There are no bonus points for sloppy patches, sloppy changelogs
and ignorance vs. the problems at hand.

I'm reserving those bonus points for you once you come up with the
patches which remove those 50 percent unnecessary code along with the
scalability problems without breaking the world and some
more. According to your profound analysis above and the other perfect
patches (including the perfect changelogs) which I had the pleasure to
review today, this should be a trivial to solve problem for you.

Thanks,

	tglx

Re: [PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[Andi Kleen]

> The idle code, at least the one in drivers/idle/intel_idle.c, does
> _NOT_ call into that code at all when a reliable lapic timer is
> available:

But the ACPI idle doesn't check that, so it still has a scalability problem.

Hmm ok so could add that check into acpi idle too I guess, but then ACPI
idle doesn't normally know anything about CPUs.

> Either you are using different code or your machine does not have that
> ARAT flag set.

The second test was using ACPI idle.

-Andi

Re: [PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[Thomas Gleixner]

On Tue, 6 Sep 2011, Andi Kleen wrote:

> > The idle code, at least the one in drivers/idle/intel_idle.c, does
> > _NOT_ call into that code at all when a reliable lapic timer is
> > available:
> 
> But the ACPI idle doesn't check that, so it still has a scalability problem.
> 
> Hmm ok so could add that check into acpi idle too I guess, but then ACPI
> idle doesn't normally know anything about CPUs.
> 
> > Either you are using different code or your machine does not have that
> > ARAT flag set.
> 
> The second test was using ACPI idle.

And as I said in that very same mail:

> > Same applies for drivers/acpi/processor_idle.c

lapic_timer_check_state()
{
       if (cpu_has(&cpu_data(pr->id), X86_FEATURE_ARAT))
                return;
....

WTF are you talking about? 

Thanks,

	tglx

[PATCH 1/3] clockevents: Use an atomic RCU notifier for clockevents

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Use an atomic_notifier instead of a raw_notifier for the clockevents
notification.

This avoids a global lock in the idle path, is a scalability
problem. With this patch we don't have a global lock anymore
at least on systems with an always running timer.

Only broadcast_enter/exit actually do use RCU for now.

I kept all the other events under the lock because they are not
on fast paths and it was not fully clear if they are all RCU
safe or not.

Actually an alternative would be to just get rid of the notifier.
As far as I can see it only has a single client anyways. But
right now I kept it.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 kernel/time/clockevents.c |   29 +++++++++++++----------------
 1 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index e4c699d..2ee6bcb 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -25,12 +25,12 @@
 static LIST_HEAD(clockevent_devices);
 static LIST_HEAD(clockevents_released);
 
-/* Notification for clock events */
-static RAW_NOTIFIER_HEAD(clockevents_chain);
-
 /* Protection for the above */
 static DEFINE_RAW_SPINLOCK(clockevents_lock);
 
+/* Notification for clock events */
+static ATOMIC_NOTIFIER_HEAD(clockevents_chain);
+
 /**
  * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
  * @latch:	value to convert
@@ -137,23 +137,12 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
  */
 int clockevents_register_notifier(struct notifier_block *nb)
 {
-	unsigned long flags;
-	int ret;
-
-	raw_spin_lock_irqsave(&clockevents_lock, flags);
-	ret = raw_notifier_chain_register(&clockevents_chain, nb);
-	raw_spin_unlock_irqrestore(&clockevents_lock, flags);
-
-	return ret;
+	return atomic_notifier_chain_register(&clockevents_chain, nb);
 }
 
-/*
- * Notify about a clock event change. Called with clockevents_lock
- * held.
- */
 static void clockevents_do_notify(unsigned long reason, void *dev)
 {
-	raw_notifier_call_chain(&clockevents_chain, reason, dev);
+	atomic_notifier_call_chain(&clockevents_chain, reason, dev);
 }
 
 /*
@@ -308,6 +297,14 @@ void clockevents_notify(unsigned long reason, void *arg)
 	unsigned long flags;
 	int cpu;
 
+	/* For idle path events don't take a lock */
+
+	if (reason == CLOCK_EVT_NOTIFY_BROADCAST_ENTER ||
+	    reason == CLOCK_EVT_NOTIFY_BROADCAST_EXIT) {
+		clockevents_do_notify(reason, arg);
+		return;
+	}
+
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
 	clockevents_do_notify(reason, arg);
 
-- 
1.7.4.4