Skip to contentSkip to conversation

x86: sparse_irq need spin_lock in alloc

PATCH In review Yinghai Lu 3 participants 4 messages Aug 21, 2008

Subsystems (2)
x86 IRQ
lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

PATCH 1/1 x86: sparse_irq need spin_lock in alloc

acording to Suresh Siddha, we should have spin_lock around it

Signed-off-by Yinghai Lu <[email protected]>
---
 arch/x86/kernel/io_apic.c |    6 ++++++
 kernel/irq/handle.c       |    7 +++++++
 2 files changed, 13 insertions(+)
kernel/irq/handle.c +7 -0
Index: linux-2.6/kernel/irq/handle.c
===================================================================
@@ -166,6 +166,9 @@ struct irq_desc *irq_to_desc(unsigned in
 	}
 	return NULL;
 }
+
+static DEFINE_SPINLOCK(sparse_irq_lock);
+
 struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 {
 	struct irq_desc *desc, *desc_pri;

@@ -182,6 +185,7 @@ struct irq_desc *irq_to_desc_alloc(unsig
 		count++;
 	}
 
+	spin_lock(&sparse_irq_lock);
 	/*
 	 *  we run out of pre-allocate ones, allocate more
 	 */

@@ -223,6 +227,9 @@ struct irq_desc *irq_to_desc_alloc(unsig
 	else
 		sparse_irqs = desc;
 	desc->irq = irq;
+
+	spin_unlock(&sparse_irq_lock);
+
 	printk(KERN_DEBUG "found new irq_desc for irq %d\n", desc->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{
arch/x86/kernel/io_apic.c +6 -0
Index: linux-2.6/arch/x86/kernel/io_apic.c
===================================================================
@@ -210,6 +210,8 @@ static struct irq_cfg *irq_cfg(unsigned
 	return NULL;
 }
 
+static DEFINE_SPINLOCK(irq_cfg_lock);
+
 static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
 {
 	struct irq_cfg *cfg, *cfg_pri;

@@ -226,6 +228,7 @@ static struct irq_cfg *irq_cfg_alloc(uns
 		count++;
 	}
 
+	spin_lock(&irq_cfg_lock);
 	if (!irq_cfgx_free) {
 		unsigned long phys;
 		unsigned long total_bytes;

@@ -263,6 +266,9 @@ static struct irq_cfg *irq_cfg_alloc(uns
 	else
 		irq_cfgx = cfg;
 	cfg->irq = irq;
+
+	spin_unlock(&irq_cfg_lock);
+
 	printk(KERN_DEBUG "found new irq_cfg for irq %d\n", cfg->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{
#1 lore raw
Quoted 63 lines · On Wed, 20 Aug 2008 20:46:25 -0700 Yinghai Lu <[email protected]> wrote:

acording to Suresh Siddha, we should have spin_lock around it

Signed-off-by Yinghai Lu <[email protected]>
---
 arch/x86/kernel/io_apic.c |    6 ++++++
 kernel/irq/handle.c       |    7 +++++++
 2 files changed, 13 insertions(+)

Index: linux-2.6/kernel/irq/handle.c
===================================================================

kernel/irq/handle.c +7 -0
@@ -166,6 +166,9 @@ struct irq_desc *irq_to_desc(unsigned in
 	}
 	return NULL;
 }
+
+static DEFINE_SPINLOCK(sparse_irq_lock);
+
 struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 {
 	struct irq_desc *desc, *desc_pri;

@@ -182,6 +185,7 @@ struct irq_desc *irq_to_desc_alloc(unsig
 		count++;
 	}
 
+	spin_lock(&sparse_irq_lock);
 	/*
 	 *  we run out of pre-allocate ones, allocate more
 	 */

@@ -223,6 +227,9 @@ struct irq_desc *irq_to_desc_alloc(unsig
 	else
 		sparse_irqs = desc;
 	desc->irq = irq;
+
+	spin_unlock(&sparse_irq_lock);
+
 	printk(KERN_DEBUG "found new irq_desc for irq %d\n", desc->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{

Index: linux-2.6/arch/x86/kernel/io_apic.c
===================================================================

arch/x86/kernel/io_apic.c +3 -0
@@ -210,6 +210,8 @@ static struct irq_cfg *irq_cfg(unsigned
 	return NULL;
 }
 
+static DEFINE_SPINLOCK(irq_cfg_lock);
+
 static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
 {
 	struct irq_cfg *cfg, *cfg_pri;

@@ -226,6 +228,7 @@ static struct irq_cfg *irq_cfg_alloc(uns
 		count++;
 	}
 
+	spin_lock(&irq_cfg_lock);
 	if (!irq_cfgx_free) {
 		unsigned long phys;
 		unsigned long total_bytes;

arch/x86/kernel/io_apic.c static struct irq_cfg *irq_cfg_alloc(uns · line 274

Quoted 9 lines
arch/x86/kernel/io_apic.c +3 -0
@@ -263,6 +266,9 @@ static struct irq_cfg *irq_cfg_alloc(uns
 	else
 		irq_cfgx = cfg;
 	cfg->irq = irq;
+
+	spin_unlock(&irq_cfg_lock);
+
 	printk(KERN_DEBUG "found new irq_cfg for irq %d\n", cfg->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{

Each of these locks can be made local to the function in which they are used (and hence they should be made local).

It would be nice to add a comment explaining what they are protecting, unless that is obvious (I didn't look).

#2 lore raw
Quoted 5 lines · Andrew Morton

Each of these locks can be made local to the function in which they are used (and hence they should be made local).

It would be nice to add a comment explaining what they are protecting, unless that is obvious (I didn't look).

ok - i moved the locks next to the data structure they protect (the free list head), and added a small exlanation as well - as per the commit below.

Ingo

--------------------->

Quoted 1 line

From 6feb0ac69551df02b49d11794ee77d3ad47aaeb3 Mon Sep 17 00:00:00 2001

From: Yinghai Lu <[email protected]>
Date: Wed, 20 Aug 2008 20:46:25 -0700
Subject: [PATCH] x86: sparse_irq needs spin_lock in allocations

Suresh Siddha noticed that we should have a spinlock around it.

Signed-off-by Yinghai Lu <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/io_apic.c |   10 ++++++++++
 kernel/irq/handle.c       |   10 ++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)
arch/x86/kernel/io_apic.c +10 -0
index 34c74cf..d1370ce 100644
@@ -146,6 +146,12 @@ static void init_one_irq_cfg(struct irq_cfg *cfg)
 }
 
 static struct irq_cfg *irq_cfgx;
+
+/*
+ * Protect the irq_cfgx_free freelist:
+ */
+static DEFINE_SPINLOCK(irq_cfg_lock);
+
 #ifdef CONFIG_HAVE_SPARSE_IRQ
 static struct irq_cfg *irq_cfgx_free;
 #endif

@@ -226,6 +232,7 @@ static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
 		count++;
 	}
 
+	spin_lock(&irq_cfg_lock);
 	if (!irq_cfgx_free) {
 		unsigned long phys;
 		unsigned long total_bytes;

@@ -263,6 +270,9 @@ static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
 	else
 		irq_cfgx = cfg;
 	cfg->irq = irq;
+
+	spin_unlock(&irq_cfg_lock);
+
 	printk(KERN_DEBUG "found new irq_cfg for irq %d\n", cfg->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{
kernel/irq/handle.c +10 -0
index 24c83a3..2f02e14 100644
@@ -107,6 +107,11 @@ static void init_kstat_irqs(struct irq_desc *desc, int nr_desc, int nr)
 	}
 }
 
+/*
+ * Protect the sparse_irqs_free freelist:
+ */
+static DEFINE_SPINLOCK(sparse_irq_lock);
+
 #ifdef CONFIG_HAVE_SPARSE_IRQ
 static struct irq_desc *sparse_irqs_free;
 struct irq_desc *sparse_irqs;

@@ -166,6 +171,7 @@ struct irq_desc *irq_to_desc(unsigned int irq)
 	}
 	return NULL;
 }
+
 struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 {
 	struct irq_desc *desc, *desc_pri;

@@ -182,6 +188,7 @@ struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 		count++;
 	}
 
+	spin_lock(&sparse_irq_lock);
 	/*
 	 *  we run out of pre-allocate ones, allocate more
 	 */

@@ -223,6 +230,9 @@ struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 	else
 		sparse_irqs = desc;
 	desc->irq = irq;
+
+	spin_unlock(&sparse_irq_lock);
+
 	printk(KERN_DEBUG "found new irq_desc for irq %d\n", desc->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{
#3 lore raw
Quoted 11 lines · Ingo Molnar
Quoted 5 lines · Andrew Morton

Each of these locks can be made local to the function in which they are used (and hence they should be made local).

It would be nice to add a comment explaining what they are protecting, unless that is obvious (I didn't look).

ok - i moved the locks next to the data structure they protect (the free list head), and added a small exlanation as well - as per the commit below.

they also need to be irqsafe locks, as they might be taken inside irq-safe locks. Updated patch below.

Ingo

Quoted 1 line

From a532e19680ada3b8579b81e67e76d3ebd19c340f Mon Sep 17 00:00:00 2001

From: Yinghai Lu <[email protected]>
Date: Wed, 20 Aug 2008 20:46:25 -0700
Subject: [PATCH] x86: sparse_irq needs spin_lock in allocations

Suresh Siddha noticed that we should have a spinlock around it.

Signed-off-by Yinghai Lu <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/io_apic.c |   13 ++++++++++++-
 kernel/irq/handle.c       |   13 ++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)
arch/x86/kernel/io_apic.c +12 -1
index 34c74cf..7ca5566 100644
@@ -146,6 +146,12 @@ static void init_one_irq_cfg(struct irq_cfg *cfg)
 }
 
 static struct irq_cfg *irq_cfgx;
+
+/*
+ * Protect the irq_cfgx_free freelist:
+ */
+static DEFINE_SPINLOCK(irq_cfg_lock);
+
 #ifdef CONFIG_HAVE_SPARSE_IRQ
 static struct irq_cfg *irq_cfgx_free;
 #endif

@@ -213,8 +219,9 @@ static struct irq_cfg *irq_cfg(unsigned int irq)
 static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
 {
 	struct irq_cfg *cfg, *cfg_pri;
-	int i;
+	unsigned long flags;
 	int count = 0;
+	int i;
 
 	cfg_pri = cfg = irq_cfgx;
 	while (cfg) {

@@ -226,6 +233,7 @@ static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
 		count++;
 	}
 
+	spin_lock_irqsave(&irq_cfg_lock, flags);
 	if (!irq_cfgx_free) {
 		unsigned long phys;
 		unsigned long total_bytes;

@@ -263,6 +271,9 @@ static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
 	else
 		irq_cfgx = cfg;
 	cfg->irq = irq;
+
+	spin_unlock_irqrestore(&irq_cfg_lock, flags);
+
 	printk(KERN_DEBUG "found new irq_cfg for irq %d\n", cfg->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{
kernel/irq/handle.c +12 -1
index 24c83a3..d638a91 100644
@@ -107,6 +107,11 @@ static void init_kstat_irqs(struct irq_desc *desc, int nr_desc, int nr)
 	}
 }
 
+/*
+ * Protect the sparse_irqs_free freelist:
+ */
+static DEFINE_SPINLOCK(sparse_irq_lock);
+
 #ifdef CONFIG_HAVE_SPARSE_IRQ
 static struct irq_desc *sparse_irqs_free;
 struct irq_desc *sparse_irqs;

@@ -166,11 +171,13 @@ struct irq_desc *irq_to_desc(unsigned int irq)
 	}
 	return NULL;
 }
+
 struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 {
 	struct irq_desc *desc, *desc_pri;
-	int i;
+	unsigned long flags;
 	int count = 0;
+	int i;
 
 	desc_pri = desc = sparse_irqs;
 	while (desc) {

@@ -182,6 +189,7 @@ struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 		count++;
 	}
 
+	spin_lock_irqsave(&sparse_irq_lock, flags);
 	/*
 	 *  we run out of pre-allocate ones, allocate more
 	 */

@@ -223,6 +231,9 @@ struct irq_desc *irq_to_desc_alloc(unsigned int irq)
 	else
 		sparse_irqs = desc;
 	desc->irq = irq;
+
+	spin_unlock_irqrestore(&sparse_irq_lock, flags);
+
 	printk(KERN_DEBUG "found new irq_desc for irq %d\n", desc->irq);
 #ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
 	{