[PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[Zwane Mwaikambo]

Hi Richard,
	This is an untested patch to remove the custom mutex, however it 
doesn't maintain the same semantics wrt 'retry' and unconditionally 
blocks on contention. A version of this patch is in the 
smp_call_function_on_cpu patch i posted for Alpha so they are mutually 
exclusive.

Index: linux-2.5.60-uml/arch/alpha/kernel/smp.c
===================================================================
RCS file: /build/cvsroot/linux-2.5.60/arch/alpha/kernel/smp.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 smp.c
--- linux-2.5.60-uml/arch/alpha/kernel/smp.c	10 Feb 2003 22:14:47 -0000	1.1.1.1
+++ linux-2.5.60-uml/arch/alpha/kernel/smp.c	14 Feb 2003 11:49:53 -0000
@@ -668,38 +667,7 @@
 };
 
 static struct smp_call_struct *smp_call_function_data;
-
-/* Atomicly drop data into a shared pointer.  The pointer is free if
-   it is initially locked.  If retry, spin until free.  */
-
-static int
-pointer_lock (void *lock, void *data, int retry)
-{
-	void *old, *tmp;
-
-	mb();
- again:
-	/* Compare and swap with zero.  */
-	asm volatile (
-	"1:	ldq_l	%0,%1\n"
-	"	mov	%3,%2\n"
-	"	bne	%0,2f\n"
-	"	stq_c	%2,%1\n"
-	"	beq	%2,1b\n"
-	"2:"
-	: "=&r"(old), "=m"(*(void **)lock), "=&r"(tmp)
-	: "r"(data)
-	: "memory");
-
-	if (old == 0)
-		return 0;
-	if (! retry)
-		return -EBUSY;
-
-	while (*(void **)lock)
-		barrier();
-	goto again;
-}
+static spinlock_t call_lock = SPIN_LOCK_UNLOCKED;
 
 void
 handle_ipi(struct pt_regs *regs)
@@ -829,9 +797,8 @@
 	atomic_set(&data.unstarted_count, num_cpus_to_call);
 	atomic_set(&data.unfinished_count, num_cpus_to_call);
 
-	/* Acquire the smp_call_function_data mutex.  */
-	if (pointer_lock(&smp_call_function_data, &data, retry))
-		return -EBUSY;
+	spin_lock(&call_lock);
+	smp_call_function_data = &data;
 
 	/* Send a message to the requested CPUs.  */
 	send_ipi_message(to_whom, IPI_CALL_FUNC);
@@ -865,7 +832,8 @@
 
 	/* We either got one or timed out -- clear the lock. */
 	mb();
-	smp_call_function_data = 0;
+	smp_call_function_data = NULL;
+	spin_unlock(&call_lock);
 
 	/* 
 	 * If after both the initial and long timeout periods we still don't

Re: [PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[Ivan Kokshaysky]

On Fri, Feb 14, 2003 at 06:51:54AM -0500, Zwane Mwaikambo wrote:
> 	This is an untested patch to remove the custom mutex, however it 
> doesn't maintain the same semantics wrt 'retry' and unconditionally 
> blocks on contention.

Why do you want to remove it? The critical data here is a single pointer
which can be effectively protected without spinlock.

Ivan.

Re: [PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[Zwane Mwaikambo]

On Fri, 14 Feb 2003, Ivan Kokshaysky wrote:

> On Fri, Feb 14, 2003 at 06:51:54AM -0500, Zwane Mwaikambo wrote:
> > 	This is an untested patch to remove the custom mutex, however it 
> > doesn't maintain the same semantics wrt 'retry' and unconditionally 
> > blocks on contention.
> 
> Why do you want to remove it? The critical data here is a single pointer
> which can be effectively protected without spinlock.

Ok the reason being is that the lock not only protects the 
smp_call_function_data pointer but also acts as a lock for that critical 
section. Without it you'll constantly be overwriting the pointer halfway 
through IPI acceptance (or even worse whilst a remote CPU is assigning the 
data members).

Although i'm not denying there isn't a way to do this lockless, i believe 
what some architectures did was poll on the pointer then do an assignment 
which i think is a bit too much effort when using a spinlock is much 
saner. Regardless i'd be interested to hear about alternatives.

Cheers,
	Zwane
-- 
function.linuxpower.ca

Re: [PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[Richard Henderson]

On Fri, Feb 14, 2003 at 12:16:12PM -0500, Zwane Mwaikambo wrote:
> Ok the reason being is that the lock not only protects the 
> smp_call_function_data pointer but also acts as a lock for that critical 
> section. Without it you'll constantly be overwriting the pointer halfway 
> through IPI acceptance (or even worse whilst a remote CPU is assigning the 
> data members).

Really.  Show me the sequence there? 

I happen to like the pointer_lock a lot, and think we should
make more use of it on systems known to have cmpxchg.  It
saves on the number of cache lines that have to get bounced
between processors.


r~

Re: [PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[Zwane Mwaikambo]

On Mon, 17 Feb 2003, Richard Henderson wrote:

> On Fri, Feb 14, 2003 at 12:16:12PM -0500, Zwane Mwaikambo wrote:
> > Ok the reason being is that the lock not only protects the 
> > smp_call_function_data pointer but also acts as a lock for that critical 
> > section. Without it you'll constantly be overwriting the pointer halfway 
> > through IPI acceptance (or even worse whilst a remote CPU is assigning the 
> > data members).
> 
> Really.  Show me the sequence there? 

/* Acquire the smp_call_function_data mutex.  */
if (pointer_lock(&smp_call_function_data, &data, retry))
	return -EBUSY;

say we remove the pointer lock there and do a single atomic assignment

...

if (atomic_read(&data.unstarted_count) > 0) {
	...
}

we got at least one IPI 

/* We either got one or timed out -- clear the lock. */
mb();
smp_call_function_data = 0;

We clear smp_call_function_data

...

cpuX receives the IPI

case IPI_CALL_FUNC:
{
	struct smp_call_struct *data;
	void (*func)(void *info);
	void *info;
	int wait;

	data = smp_call_function_data;
	func = data->func;
	info = data->info;
...

Assigns whatever the pointer happens to be at the time, be it NULL or the 
next incoming message call.

Therefore we'd need a lock to protect both the variable and critical 
section.

> I happen to like the pointer_lock a lot, and think we should
> make more use of it on systems known to have cmpxchg.  It
> saves on the number of cache lines that have to get bounced
> between processors.

I have to agree there, it would save on locked operations per 
'acquisition' which can be a win on a lot of systems.

	Zwane
-- 
function.linuxpower.ca

Re: [PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[Ivan Kokshaysky]

On Mon, Feb 17, 2003 at 03:32:09AM -0500, Zwane Mwaikambo wrote:
> Assigns whatever the pointer happens to be at the time, be it NULL or the 
> next incoming message call.

No, the pointer is guaranteed to be valid.

> Therefore we'd need a lock to protect both the variable and critical 
> section.

But smp_call_function_data pointer itself is exactly such a lock -
other CPUs can't enter the section between 'if (pointer_lock())' and
'smp_call_function_data = 0', so there is no need for extra lock
variable. Additionally, pointer_lock() with retry = 0 acts as spin_trylock.

> > I happen to like the pointer_lock a lot, and think we should
> > make more use of it on systems known to have cmpxchg.  It
> > saves on the number of cache lines that have to get bounced
> > between processors.
> 
> I have to agree there, it would save on locked operations per 
> 'acquisition' which can be a win on a lot of systems.

Here's cmpxchg version for illustration (untested).

Ivan.

--- linux/arch/alpha/kernel/smp.c.orig	Mon Feb 10 21:38:15 2003
+++ linux/arch/alpha/kernel/smp.c	Mon Feb 17 17:05:47 2003
@@ -680,17 +680,7 @@ pointer_lock (void *lock, void *data, in
 	mb();
  again:
 	/* Compare and swap with zero.  */
-	asm volatile (
-	"1:	ldq_l	%0,%1\n"
-	"	mov	%3,%2\n"
-	"	bne	%0,2f\n"
-	"	stq_c	%2,%1\n"
-	"	beq	%2,1b\n"
-	"2:"
-	: "=&r"(old), "=m"(*(void **)lock), "=&r"(tmp)
-	: "r"(data)
-	: "memory");
-
+	old = cmpxchg(lock, 0, data);
 	if (old == 0)
 		return 0;
 	if (! retry)

Re: [PATCH][2.5] Protect smp_call_function_data w/ spinlocks on Alpha

[Zwane Mwaikambo]

On Mon, 17 Feb 2003, Ivan Kokshaysky wrote:

> On Mon, Feb 17, 2003 at 03:32:09AM -0500, Zwane Mwaikambo wrote:
> > Assigns whatever the pointer happens to be at the time, be it NULL or the 
> > next incoming message call.
> 
> No, the pointer is guaranteed to be valid.
> 
> > Therefore we'd need a lock to protect both the variable and critical 
> > section.
> 
> But smp_call_function_data pointer itself is exactly such a lock -
> other CPUs can't enter the section between 'if (pointer_lock())' and
> 'smp_call_function_data = 0', so there is no need for extra lock
> variable. Additionally, pointer_lock() with retry = 0 acts as spin_trylock.

Oh my mistake i thought you were talking about atomic assignment and not 
blocking at that point. I misunderstood what you stated.

	Zwane
-- 
function.linuxpower.ca