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
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.
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
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~
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
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)
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