[parisc-linux] [PATCH] Remove spinlock_t from cpu_data (and thus from processor.h)

[parisc-linux] [PATCH] Remove spinlock_t from cpu_data (and thus from processor.h)

[Kyle McMartin]

This should enable us to clean up our header files nicely.

Signed-off-by: Kyle McMartin <kyle@parisc-linux.org>

---
diff --git a/arch/parisc/kernel/processor.c b/arch/parisc/kernel/processor.c
index fb81e56..0af1fad 100644
--- a/arch/parisc/kernel/processor.c
+++ b/arch/parisc/kernel/processor.c
@@ -153,8 +153,6 @@ #endif
 	p->cpuid = cpuid;	/* save CPU id */
 	p->txn_addr = txn_addr;	/* save CPU IRQ address */
 #ifdef CONFIG_SMP
-	spin_lock_init(&p->lock);
-
 	/*
 	** FIXME: review if any other initialization is clobbered
 	**	for boot_cpu by the above memset().
diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 4a23a97..5301600 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -76,6 +76,9 @@ cpumask_t cpu_possible_map __read_mostly
 EXPORT_SYMBOL(cpu_online_map);
 EXPORT_SYMBOL(cpu_possible_map);
 
+spinlock_t fencepost[NR_CPUS] = {
+	[0 ... NR_CPUS-1] = SPIN_LOCK_UNLOCKED
+};
 
 struct smp_call_struct {
 	void (*func) (void *info);
@@ -167,10 +170,11 @@ ipi_interrupt(int irq, void *dev_id) 
 	mb();	/* Order interrupt and bit testing. */
 
 	for (;;) {
-		spin_lock_irqsave(&(p->lock),flags);
+		spinlock_t *lock = &fencepost[this_cpu];
+		spin_lock_irqsave(lock, flags);
 		ops = p->pending_ipi;
 		p->pending_ipi = 0;
-		spin_unlock_irqrestore(&(p->lock),flags);
+		spin_unlock_irqrestore(lock, flags);
 
 		mb(); /* Order bit clearing and data access. */
 
@@ -275,12 +279,13 @@ static inline void
 ipi_send(int cpu, enum ipi_message_type op)
 {
 	struct cpuinfo_parisc *p = &cpu_data[cpu];
+	spinlock_t *lock = &fencepost[cpu];
 	unsigned long flags;
 
-	spin_lock_irqsave(&(p->lock),flags);
+	spin_lock_irqsave(lock, flags);
 	p->pending_ipi |= 1 << op;
 	gsc_writel(IPI_IRQ - CPU_IRQ_BASE, cpu_data[cpu].hpa);
-	spin_unlock_irqrestore(&(p->lock),flags);
+	spin_unlock_irqrestore(lock, flags);
 }
 
 
diff --git a/include/asm-parisc/processor.h b/include/asm-parisc/processor.h
index fd7866d..435afe5 100644
--- a/include/asm-parisc/processor.h
+++ b/include/asm-parisc/processor.h
@@ -87,7 +87,6 @@ struct cpuinfo_parisc {
 	unsigned long hpa;          /* Host Physical address */
 	unsigned long txn_addr;     /* MMIO addr of EIR or id_eid */
 #ifdef CONFIG_SMP
-	spinlock_t lock;            /* synchronization for ipi's */
 	unsigned long pending_ipi;  /* bitmap of type ipi_message_type */
 	unsigned long ipi_count;    /* number ipi Interrupts */
 #endif
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] Remove spinlock_t from cpu_data (and thus from processor.h)

[Grant Grundler]

On Sat, Nov 25, 2006 at 11:53:44AM -0500, Kyle McMartin wrote:
> This should enable us to clean up our header files nicely.
...

Yes, but...

> +spinlock_t fencepost[NR_CPUS] = {
> +	[0 ... NR_CPUS-1] = SPIN_LOCK_UNLOCKED
> +};

Unless each CPU gets it's own cacheline, I believe this is
going to ping-pong across CPU caches like hell. Thus
the "per-CPU" initialization. If the fencepost[] locks
are seldom used, then it doesn't matter and we could just use
one cacheline.

but I think you are on the right track to unraveling our
header dependencies a bit more...

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] Remove spinlock_t from cpu_data (and thus from processor.h)

[Kyle McMartin]

On Sun, Nov 26, 2006 at 12:33:06AM -0700, Grant Grundler wrote:
> 

diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 4a23a97..5b6bc6e 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -76,6 +76,7 @@ cpumask_t cpu_possible_map __read_mostly
 EXPORT_SYMBOL(cpu_online_map);
 EXPORT_SYMBOL(cpu_possible_map);
 
+DEFINE_PER_CPU(spinlock_t, ipi_lock) = SPIN_LOCK_UNLOCKED;
 
 struct smp_call_struct {
 	void (*func) (void *info);
@@ -167,10 +168,11 @@ ipi_interrupt(int irq, void *dev_id) 
 	mb();	/* Order interrupt and bit testing. */
 
 	for (;;) {
-		spin_lock_irqsave(&(p->lock),flags);
+		spinlock_t *lock = &per_cpu(ipi_lock, this_cpu);
+		spin_lock_irqsave(lock, flags);
 		ops = p->pending_ipi;
 		p->pending_ipi = 0;
-		spin_unlock_irqrestore(&(p->lock),flags);
+		spin_unlock_irqrestore(lock, flags);
 
 		mb(); /* Order bit clearing and data access. */
 
@@ -275,12 +277,13 @@ static inline void
 ipi_send(int cpu, enum ipi_message_type op)
 {
 	struct cpuinfo_parisc *p = &cpu_data[cpu];
+	spinlock_t *lock = &per_cpu(ipi_lock, cpu);
 	unsigned long flags;
 
-	spin_lock_irqsave(&(p->lock),flags);
+	spin_lock_irqsave(lock, flags);
 	p->pending_ipi |= 1 << op;
 	gsc_writel(IPI_IRQ - CPU_IRQ_BASE, cpu_data[cpu].hpa);
-	spin_unlock_irqrestore(&(p->lock),flags);
+	spin_unlock_irqrestore(lock, flags);
 }
 
 
diff --git a/include/asm-parisc/processor.h b/include/asm-parisc/processor.h
index fd7866d..435afe5 100644
--- a/include/asm-parisc/processor.h
+++ b/include/asm-parisc/processor.h
@@ -87,7 +87,6 @@ struct cpuinfo_parisc {
 	unsigned long hpa;          /* Host Physical address */
 	unsigned long txn_addr;     /* MMIO addr of EIR or id_eid */
 #ifdef CONFIG_SMP
-	spinlock_t lock;            /* synchronization for ipi's */
 	unsigned long pending_ipi;  /* bitmap of type ipi_message_type */
 	unsigned long ipi_count;    /* number ipi Interrupts */
 #endif
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] Remove spinlock_t from cpu_data (and thus from processor.h)

[Matthew Wilcox]

On Sat, Nov 25, 2006 at 11:53:44AM -0500, Kyle McMartin wrote:
> This should enable us to clean up our header files nicely.

Thanks for looking at this.

My first thought was "Surely there's a better way to do this, maybe
DEFINE_PER_CPU".

Then I looked at what you'd had to touch.  Essentially, we're
multiplexing 6 different ops onto one interrupt.  This would make sense
if we were short of interrupts, but on the vast majority of systems, we
aren't.

We define NOP, RESCHEDULE, CALL_FUNC, CPU_START, CPU_STOP and CPU_TEST.
CPU_TEST is never sent, down to 5.  CPU_NOP and CPU_RESCHEDULE are the
same other than debug statements, down to 4.  So by taking 3 additional
CPU interrupts, we can eliminate the spinlock entirely.

We could do away with RESCHEDULE if we pass a nop function to CALL_FUNC.
I don't know if we would suffer a performance hit from doing that
though.

We could also only enable CPU_STOP if CPU_HOTPLUG was set.  And
fold CPU_START/CPU_STOP into the CALL_FUNC bucket.  Or deregister
CPU_START after the CPU's come up.

In any case, I think this is a whole lot preferable to the spinlock that
we're currently using.  I have some other ideas (like using set_bit()
and xchg() which would take the atomic_hash_spinlock), but I think
redoing our IPI support in the way I've outlined above is cleaner.
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] Remove spinlock_t from cpu_data (and thus from processor.h)

[Grant Grundler]

On Sun, Nov 26, 2006 at 01:37:42PM -0500, Kyle McMartin wrote:
> diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
> index 4a23a97..5b6bc6e 100644
> --- a/arch/parisc/kernel/smp.c
> +++ b/arch/parisc/kernel/smp.c
> @@ -76,6 +76,7 @@ cpumask_t cpu_possible_map __read_mostly
>  EXPORT_SYMBOL(cpu_online_map);
>  EXPORT_SYMBOL(cpu_possible_map);
>  
> +DEFINE_PER_CPU(spinlock_t, ipi_lock) = SPIN_LOCK_UNLOCKED;

If this works - this is the right way to do it.
You rock!

thanks!
grant

>  
>  struct smp_call_struct {
>  	void (*func) (void *info);
> @@ -167,10 +168,11 @@ ipi_interrupt(int irq, void *dev_id) 
>  	mb();	/* Order interrupt and bit testing. */
>  
>  	for (;;) {
> -		spin_lock_irqsave(&(p->lock),flags);
> +		spinlock_t *lock = &per_cpu(ipi_lock, this_cpu);
> +		spin_lock_irqsave(lock, flags);
>  		ops = p->pending_ipi;
>  		p->pending_ipi = 0;
> -		spin_unlock_irqrestore(&(p->lock),flags);
> +		spin_unlock_irqrestore(lock, flags);
>  
>  		mb(); /* Order bit clearing and data access. */
>  
> @@ -275,12 +277,13 @@ static inline void
>  ipi_send(int cpu, enum ipi_message_type op)
>  {
>  	struct cpuinfo_parisc *p = &cpu_data[cpu];
> +	spinlock_t *lock = &per_cpu(ipi_lock, cpu);
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&(p->lock),flags);
> +	spin_lock_irqsave(lock, flags);
>  	p->pending_ipi |= 1 << op;
>  	gsc_writel(IPI_IRQ - CPU_IRQ_BASE, cpu_data[cpu].hpa);
> -	spin_unlock_irqrestore(&(p->lock),flags);
> +	spin_unlock_irqrestore(lock, flags);
>  }
>  
>  
> diff --git a/include/asm-parisc/processor.h b/include/asm-parisc/processor.h
> index fd7866d..435afe5 100644
> --- a/include/asm-parisc/processor.h
> +++ b/include/asm-parisc/processor.h
> @@ -87,7 +87,6 @@ struct cpuinfo_parisc {
>  	unsigned long hpa;          /* Host Physical address */
>  	unsigned long txn_addr;     /* MMIO addr of EIR or id_eid */
>  #ifdef CONFIG_SMP
> -	spinlock_t lock;            /* synchronization for ipi's */
>  	unsigned long pending_ipi;  /* bitmap of type ipi_message_type */
>  	unsigned long ipi_count;    /* number ipi Interrupts */
>  #endif
> _______________________________________________
> parisc-linux mailing list
> parisc-linux@lists.parisc-linux.org
> http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux