[PATCH] irq: use per_cpu kstat_irqs

[PATCH] irq: use per_cpu kstat_irqs

[Eric Dumazet]

Use modern per_cpu API to increment {soft|hard}irq counters, and
use per_cpu allocation for (struct irq_desc)->kstats_irq instead of an
array.

This gives better SMP/NUMA locality and saves few instructions per irq.

With small nr_cpuids values (8 for example), kstats_irq was a small
array (less than L1_CACHE_BYTES), potentially source of false sharing.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/irqdesc.h     |    2 +-
 include/linux/kernel_stat.h |   21 ++++++++++-----------
 kernel/irq/irqdesc.c        |   16 +++++++++-------
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 979c68c..6a64c6f 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -57,7 +57,7 @@ struct irq_desc {
 #endif
 
 	struct timer_rand_state *timer_rand_state;
-	unsigned int		*kstat_irqs;
+	unsigned int __percpu	*kstat_irqs;
 	irq_flow_handler_t	handle_irq;
 	struct irqaction	*action;	/* IRQ action list */
 	unsigned int		status;		/* IRQ status */
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index ad54c84..acd97a3 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -31,7 +31,7 @@ struct cpu_usage_stat {
 struct kernel_stat {
 	struct cpu_usage_stat	cpustat;
 #ifndef CONFIG_GENERIC_HARDIRQS
-       unsigned int irqs[NR_IRQS];
+	unsigned int irqs[NR_IRQS];
 #endif
 	unsigned long irqs_sum;
 	unsigned int softirqs[NR_SOFTIRQS];
@@ -46,41 +46,40 @@ DECLARE_PER_CPU(struct kernel_stat, kstat);
 extern unsigned long long nr_context_switches(void);
 
 #ifndef CONFIG_GENERIC_HARDIRQS
-#define kstat_irqs_this_cpu(irq) \
-	(kstat_this_cpu.irqs[irq])
+#define kstat_irqs_this_cpu(irq) __this_cpu_read(kstat.irqs[irq])
 
 struct irq_desc;
 
 static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
 					    struct irq_desc *desc)
 {
-	kstat_this_cpu.irqs[irq]++;
-	kstat_this_cpu.irqs_sum++;
+	__this_cpu_inc(kstat.irqs[irq]);
+	__this_cpu_inc(kstat.irqs_sum);
 }
 
 static inline unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
 {
-       return kstat_cpu(cpu).irqs[irq];
+	return kstat_cpu(cpu).irqs[irq];
 }
 #else
 #include <linux/irq.h>
 extern unsigned int kstat_irqs_cpu(unsigned int irq, int cpu);
 #define kstat_irqs_this_cpu(DESC) \
-	((DESC)->kstat_irqs[smp_processor_id()])
+	__this_cpu_read((DESC)->kstat_irqs)
 #define kstat_incr_irqs_this_cpu(irqno, DESC) do {\
-	((DESC)->kstat_irqs[smp_processor_id()]++);\
-	kstat_this_cpu.irqs_sum++; } while (0)
+	__this_cpu_inc((DESC)->kstat_irqs);\
+	__this_cpu_inc(kstat.irqs_sum); } while (0)
 
 #endif
 
 static inline void kstat_incr_softirqs_this_cpu(unsigned int irq)
 {
-	kstat_this_cpu.softirqs[irq]++;
+	__this_cpu_inc(kstat.softirqs[irq]);
 }
 
 static inline unsigned int kstat_softirqs_cpu(unsigned int irq, int cpu)
 {
-       return kstat_cpu(cpu).softirqs[irq];
+	return kstat_cpu(cpu).softirqs[irq];
 }
 
 /*
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9988d03..7683422 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -72,6 +72,8 @@ static inline int desc_node(struct irq_desc *desc) { return 0; }
 
 static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node)
 {
+	int cpu;
+
 	desc->irq_data.irq = irq;
 	desc->irq_data.chip = &no_irq_chip;
 	desc->irq_data.chip_data = NULL;
@@ -83,7 +85,8 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node)
 	desc->irq_count = 0;
 	desc->irqs_unhandled = 0;
 	desc->name = NULL;
-	memset(desc->kstat_irqs, 0, nr_cpu_ids * sizeof(*(desc->kstat_irqs)));
+	for_each_possible_cpu(cpu)
+			*per_cpu_ptr(desc->kstat_irqs, cpu) = 0;
 	desc_smp_init(desc, node);
 }
 
@@ -133,8 +136,7 @@ static struct irq_desc *alloc_desc(int irq, int node)
 	if (!desc)
 		return NULL;
 	/* allocate based on nr_cpu_ids */
-	desc->kstat_irqs = kzalloc_node(nr_cpu_ids * sizeof(*desc->kstat_irqs),
-					 gfp, node);
+	desc->kstat_irqs = alloc_percpu(unsigned int);
 	if (!desc->kstat_irqs)
 		goto err_desc;
 
@@ -149,7 +151,7 @@ static struct irq_desc *alloc_desc(int irq, int node)
 	return desc;
 
 err_kstat:
-	kfree(desc->kstat_irqs);
+	free_percpu(desc->kstat_irqs);
 err_desc:
 	kfree(desc);
 	return NULL;
@@ -166,7 +168,7 @@ static void free_desc(unsigned int irq)
 	mutex_unlock(&sparse_irq_lock);
 
 	free_masks(desc);
-	kfree(desc->kstat_irqs);
+	free_percpu(desc->kstat_irqs);
 	kfree(desc);
 }
 
@@ -391,7 +393,7 @@ void dynamic_irq_cleanup(unsigned int irq)
 unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
-	return desc ? desc->kstat_irqs[cpu] : 0;
+	return desc ? *per_cpu_ptr(desc->kstat_irqs, cpu) : 0;
 }
 
 #ifdef CONFIG_GENERIC_HARDIRQS
@@ -404,7 +406,7 @@ unsigned int kstat_irqs(unsigned int irq)
 	if (!desc)
 		return 0;
 	for_each_possible_cpu(cpu)
-		sum += desc->kstat_irqs[cpu];
+		sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
 	return sum;
 }
 #endif /* CONFIG_GENERIC_HARDIRQS */

Re: [PATCH] irq: use per_cpu kstat_irqs

[Christoph Lameter]

On Thu, 4 Nov 2010, Eric Dumazet wrote:

>  static inline unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
>  {
> -       return kstat_cpu(cpu).irqs[irq];
> +	return kstat_cpu(cpu).irqs[irq];
>  }

Whitespace fixes?

Looks ok otherwise.

Reviewed-by: Christoph Lameter <cl@linux.com>

Re: [PATCH] irq: use per_cpu kstat_irqs

[Eric Dumazet]

Le jeudi 04 novembre 2010 à 13:05 -0500, Christoph Lameter a écrit :
> On Thu, 4 Nov 2010, Eric Dumazet wrote:
> 
> >  static inline unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
> >  {
> > -       return kstat_cpu(cpu).irqs[irq];
> > +	return kstat_cpu(cpu).irqs[irq];
> >  }
> 
> Whitespace fixes?
> 
> Looks ok otherwise.
> 
> Reviewed-by: Christoph Lameter <cl@linux.com>


Ah yes, some lines were beginning with seven spaces.

Thanks for the review !

Re: [PATCH] irq: use per_cpu kstat_irqs

[Tejun Heo]

On 11/04/2010 06:48 PM, Eric Dumazet wrote:
> Use modern per_cpu API to increment {soft|hard}irq counters, and
> use per_cpu allocation for (struct irq_desc)->kstats_irq instead of an
> array.
> 
> This gives better SMP/NUMA locality and saves few instructions per irq.
> 
> With small nr_cpuids values (8 for example), kstats_irq was a small
> array (less than L1_CACHE_BYTES), potentially source of false sharing.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Christoph Lameter <cl@linux-foundation.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>

Looks good to me although it would be nice to note the white space
fixes in the comment.

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] irq: use per_cpu kstat_irqs

[Eric Dumazet]

Le vendredi 05 novembre 2010 à 09:27 +0100, Tejun Heo a écrit :

> Looks good to me although it would be nice to note the white space
> fixes in the comment.
> 
> Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks !

I'll send a V2 anyway, without the white space fixes,
and with better support of !CONFIG_SPARSE_IRQ 

[PATCH v2] irq: use per_cpu kstat_irqs

[Eric Dumazet]

Use modern per_cpu API to increment {soft|hard}irq counters, and
use per_cpu allocation for (struct irq_desc)->kstats_irq instead of an
array.

This gives better SMP/NUMA locality and saves few instructions per irq.

With small nr_cpuids values (8 for example), kstats_irq was a small
array (less than L1_CACHE_BYTES), potentially source of false sharing.

In the !CONFIG_SPARSE_IRQ case, remove the huge, NUMA/cache unfriendly
kstat_irqs_all[NR_IRQS][NR_CPUS] array.

Note : we still populate kstats_irq for all possible irqs in
early_irq_init(). We probably could use on-demand allocations.
(Code included in alloc_descs()).
Problem is not all IRQS are used with a prior alloc_descs() call.

kstat_irqs_this_cpu() is not used anymore, remove it.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
V2: support for !CONFIG_SPARSE_IRQ
    No white space changes.

Question : Is dynamic percpu allocator ready at this stage for all
arches ? The need is NR_IRQS*4 bytes of percpu memory for 
!CONFIG_SPARSE_IRQ


diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 979c68c..6a64c6f 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -57,7 +57,7 @@ struct irq_desc {
 #endif
 
 	struct timer_rand_state *timer_rand_state;
-	unsigned int		*kstat_irqs;
+	unsigned int __percpu	*kstat_irqs;
 	irq_flow_handler_t	handle_irq;
 	struct irqaction	*action;	/* IRQ action list */
 	unsigned int		status;		/* IRQ status */
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index ad54c84..0cce2db 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -46,16 +46,14 @@ DECLARE_PER_CPU(struct kernel_stat, kstat);
 extern unsigned long long nr_context_switches(void);
 
 #ifndef CONFIG_GENERIC_HARDIRQS
-#define kstat_irqs_this_cpu(irq) \
-	(kstat_this_cpu.irqs[irq])
 
 struct irq_desc;
 
 static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
 					    struct irq_desc *desc)
 {
-	kstat_this_cpu.irqs[irq]++;
-	kstat_this_cpu.irqs_sum++;
+	__this_cpu_inc(kstat.irqs[irq]);
+	__this_cpu_inc(kstat.irqs_sum);
 }
 
 static inline unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
@@ -65,17 +63,18 @@ static inline unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
 #else
 #include <linux/irq.h>
 extern unsigned int kstat_irqs_cpu(unsigned int irq, int cpu);
-#define kstat_irqs_this_cpu(DESC) \
-	((DESC)->kstat_irqs[smp_processor_id()])
-#define kstat_incr_irqs_this_cpu(irqno, DESC) do {\
-	((DESC)->kstat_irqs[smp_processor_id()]++);\
-	kstat_this_cpu.irqs_sum++; } while (0)
+
+#define kstat_incr_irqs_this_cpu(irqno, DESC)		\
+do {							\
+	__this_cpu_inc(*(DESC)->kstat_irqs);		\
+	__this_cpu_inc(kstat.irqs_sum);			\
+} while (0)
 
 #endif
 
 static inline void kstat_incr_softirqs_this_cpu(unsigned int irq)
 {
-	kstat_this_cpu.softirqs[irq]++;
+	__this_cpu_inc(kstat.softirqs[irq]);
 }
 
 static inline unsigned int kstat_softirqs_cpu(unsigned int irq, int cpu)
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9988d03..a789a22 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -72,6 +72,8 @@ static inline int desc_node(struct irq_desc *desc) { return 0; }
 
 static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node)
 {
+	int cpu;
+
 	desc->irq_data.irq = irq;
 	desc->irq_data.chip = &no_irq_chip;
 	desc->irq_data.chip_data = NULL;
@@ -83,7 +85,8 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node)
 	desc->irq_count = 0;
 	desc->irqs_unhandled = 0;
 	desc->name = NULL;
-	memset(desc->kstat_irqs, 0, nr_cpu_ids * sizeof(*(desc->kstat_irqs)));
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(desc->kstat_irqs, cpu) = 0;
 	desc_smp_init(desc, node);
 }
 
@@ -133,8 +136,7 @@ static struct irq_desc *alloc_desc(int irq, int node)
 	if (!desc)
 		return NULL;
 	/* allocate based on nr_cpu_ids */
-	desc->kstat_irqs = kzalloc_node(nr_cpu_ids * sizeof(*desc->kstat_irqs),
-					 gfp, node);
+	desc->kstat_irqs = alloc_percpu(unsigned int);
 	if (!desc->kstat_irqs)
 		goto err_desc;
 
@@ -149,7 +151,7 @@ static struct irq_desc *alloc_desc(int irq, int node)
 	return desc;
 
 err_kstat:
-	kfree(desc->kstat_irqs);
+	free_percpu(desc->kstat_irqs);
 err_desc:
 	kfree(desc);
 	return NULL;
@@ -166,7 +168,7 @@ static void free_desc(unsigned int irq)
 	mutex_unlock(&sparse_irq_lock);
 
 	free_masks(desc);
-	kfree(desc->kstat_irqs);
+	free_percpu(desc->kstat_irqs);
 	kfree(desc);
 }
 
@@ -234,7 +236,6 @@ struct irq_desc irq_desc[NR_IRQS] __cacheline_aligned_in_smp = {
 	}
 };
 
-static unsigned int kstat_irqs_all[NR_IRQS][NR_CPUS];
 int __init early_irq_init(void)
 {
 	int count, i, node = first_online_node;
@@ -250,7 +251,8 @@ int __init early_irq_init(void)
 	for (i = 0; i < count; i++) {
 		desc[i].irq_data.irq = i;
 		desc[i].irq_data.chip = &no_irq_chip;
-		desc[i].kstat_irqs = kstat_irqs_all[i];
+		/* TODO : do this allocation on-demand ... */
+		desc[i].kstat_irqs = alloc_percpu(unsigned int);
 		alloc_masks(desc + i, GFP_KERNEL, node);
 		desc_smp_init(desc + i, node);
 		lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
@@ -275,6 +277,22 @@ static void free_desc(unsigned int irq)
 
 static inline int alloc_descs(unsigned int start, unsigned int cnt, int node)
 {
+#if defined(CONFIG_KSTAT_IRQS_ONDEMAND)
+	struct irq_desc *desc;
+	unsigned int i;
+	
+	for (i = 0; i < cnt; i++) {
+		desc = irq_to_desc(start + i);
+		if (desc && !desc->kstat_irqs) {
+			unsigned int __percpu *stats = alloc_percpu(unsigned int);
+
+			if (!stats)
+				return -1;
+			if (cmpxchg(&desc->kstat_irqs, NULL, stats) != NULL)
+				free_percpu(stats);
+		}
+	}
+#endif
 	return start;
 }
 #endif /* !CONFIG_SPARSE_IRQ */
@@ -391,7 +409,9 @@ void dynamic_irq_cleanup(unsigned int irq)
 unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
-	return desc ? desc->kstat_irqs[cpu] : 0;
+
+	return desc && desc->kstat_irqs ?
+			*per_cpu_ptr(desc->kstat_irqs, cpu) : 0;
 }
 
 #ifdef CONFIG_GENERIC_HARDIRQS
@@ -401,10 +421,10 @@ unsigned int kstat_irqs(unsigned int irq)
 	int cpu;
 	int sum = 0;
 
-	if (!desc)
+	if (!desc || !desc->kstat_irqs)
 		return 0;
 	for_each_possible_cpu(cpu)
-		sum += desc->kstat_irqs[cpu];
+		sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
 	return sum;
 }
 #endif /* CONFIG_GENERIC_HARDIRQS */

Re: [PATCH v2] irq: use per_cpu kstat_irqs

[Tejun Heo]

Hello,

On 11/05/2010 05:06 PM, Eric Dumazet wrote:
> V2: support for !CONFIG_SPARSE_IRQ
>     No white space changes.
> 
> Question : Is dynamic percpu allocator ready at this stage for all
> arches ? The need is NR_IRQS*4 bytes of percpu memory for 
> !CONFIG_SPARSE_IRQ

The earliest is early_irq_init(), right?  If so, percpu allocator is
up by then on all archs.

Thanks.

-- 
tejun

Re: [PATCH v2] irq: use per_cpu kstat_irqs

[Eric Dumazet]

Le vendredi 05 novembre 2010 à 17:45 +0100, Tejun Heo a écrit :
> Hello,
> 
> On 11/05/2010 05:06 PM, Eric Dumazet wrote:
> > V2: support for !CONFIG_SPARSE_IRQ
> >     No white space changes.
> > 
> > Question : Is dynamic percpu allocator ready at this stage for all
> > arches ? The need is NR_IRQS*4 bytes of percpu memory for 
> > !CONFIG_SPARSE_IRQ
> 
> The earliest is early_irq_init(), right?  If so, percpu allocator is
> up by then on all archs.
> 

OK thats fine then.

Thanks for the clarification Tejun.

Re: [PATCH v2] irq: use per_cpu kstat_irqs

[Christoph Lameter]

On Fri, 5 Nov 2010, Eric Dumazet wrote:

>  static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
>  					    struct irq_desc *desc)

desc is not used.

>  {
> -	kstat_this_cpu.irqs[irq]++;
> -	kstat_this_cpu.irqs_sum++;
> +	__this_cpu_inc(kstat.irqs[irq]);
> +	__this_cpu_inc(kstat.irqs_sum);
>  }

Re: [PATCH v2] irq: use per_cpu kstat_irqs

[Eric Dumazet]

Le vendredi 05 novembre 2010 à 11:54 -0500, Christoph Lameter a écrit :
> On Fri, 5 Nov 2010, Eric Dumazet wrote:
> 
> >  static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
> >  					    struct irq_desc *desc)
> 
> desc is not used.
> 
> >  {
> > -	kstat_this_cpu.irqs[irq]++;
> > -	kstat_this_cpu.irqs_sum++;
> > +	__this_cpu_inc(kstat.irqs[irq]);
> > +	__this_cpu_inc(kstat.irqs_sum);
> >  }
> 

Thats right, but it is used in CONFIG_GENERIC_HARDIRQS case.

Prototype of this function is generic, I did not change this.

(I left the macro in CONFIG_GENERIC_HARDIRQS case too because I felt
using a function could break some arches)