Skip to contentSkip to conversation

don't use ld.bias

PATCH In review Jesse Barnes 5 participants 7 messages Jul 21, 2003 – Jul 25, 2003

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 don't use ld.bias

Patch to fix spin_lock to not use ld.bias since it causes a bunch of bus activity that hurts scalability when a lock is contended.

Thanks,
Jesse

=== include/asm-ia64/spinlock.h 1.13 vs edited ==--- 1.13/include/asm-ia64/spinlock.h Fri May 30 19:59:40 2003
+++ edited/include/asm-ia64/spinlock.h Mon Jul 21 16:54:15 2003
@@ -93,13 +93,16 @@
* Streamlined test_and_set_bit(0, (x)). We use test-and-test-and-set
* rather than a simple xchg to avoid writing the cache-line when
* there is contention.
+ *
+ * Don't use ld.bias since it generates unnecessary bus activity on
+ * multiprocessor systems when the lock is contended.
*/
#define _raw_spin_lock(x) __asm__ __volatile__ ( \
"mov ar.ccv = r0\n" \
"mov r29 = 1\n" \
";;\n" \
"1:\n" \
- "ld4.bias r2 = [%0]\n" \
+ "ld4 r2 = [%0]\n" \
";;\n" \
"cmp4.eq p0,p7 = r0,r2\n" \
"(p7) br.cond.spnt.few 1b \n" \

#1 lore raw
Quoted 1 line

Patch to fix spin_lock to not use ld.bias since it causes a bunch of

bus

Quoted 1 line

activity that hurts scalability when a lock is contended.

It's dangerous to say something without knowing the whole context, but I can't resist asking why not fix the contended lock area instead? Sounds like you have some serious contention.

#2 lore raw
In reply to Jesse Barnes
Quoted 7 lines · Kenneth W
Quoted 1 line

Patch to fix spin_lock to not use ld.bias since it causes a bunch of

bus

Quoted 1 line

activity that hurts scalability when a lock is contended.

It's dangerous to say something without knowing the whole context, but I can't resist asking why not fix the contended lock area instead? Sounds like you have some serious contention.

Maybe I shouldn't have said 'contended'. This causes problems even when a lock is nearly uncontended. I agree that fixing lock contention is a good idea, but as I read this code, it'll only make the very best case (totally uncontended) only a little better than if the ld.bias were just an ld and it makes the average and contended cases _much_ worse.

I can get some data if you're not convinced by reading the code :)

Thanks,
Jesse

#3 lore raw
In reply to Jesse Barnes

It does make sense to not use the ld.bias in the spinlock, it will make more cacheline bouncing in a larger SMP system.
Thanks,
Asit

Quoted 39 lines

-----Original Message-----
From: Jesse Barnes [mailto:[email protected]]
Sent: Tuesday, July 22, 2003 6:12 AM
To: Chen, Kenneth W
Cc: [email protected]; [email protected]
Subject: Re: [PATCH] don't use ld.bias

Quoted 1 line · Kenneth W
Quoted 1 line

Patch to fix spin_lock to not use ld.bias since it causes

a bunch of

Quoted 4 lines

bus

Quoted 1 line

activity that hurts scalability when a lock is contended.

It's dangerous to say something without knowing the whole

context, but I

Quoted 1 line

can't resist asking why not fix the contended lock area

instead? Sounds

Quoted 1 line

like you have some serious contention.

Maybe I shouldn't have said 'contended'. This causes problems even when
a lock is nearly uncontended. I agree that fixing lock contention is a
good idea, but as I read this code, it'll only make the very best case (totally uncontended) only a little better than if the ld.bias were just
an ld and it makes the average and contended cases _much_ worse.

I can get some data if you're not convinced by reading the code :)

Thanks,
Jesse
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
In reply to Jesse Barnes
Quoted 2 lines · Asit K

It does make sense to not use the ld.bias in the spinlock, it will make more cacheline bouncing in a larger SMP system.

it certainly makes a mockery of the comment:

* Streamlined test_and_set_bit(0, (x)). We use test-and-test-and-set
* rather than a simple xchg to avoid writing the cache-line when
* there is contention.

since there's no real advantage (AIUI) to have the cacheline clean-exclusive vs dirty-exclusive.

but why not delete this code? it's under #else /* !NEW_LOCK */ and NEW_LOCK looks to be the better code as well as the default in 2.5.

Signature · 3 lines
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
#5 lore raw
In reply to Jesse Barnes
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · On Mon, 21 Jul 2003 16:55:59 -0700, [email protected] (Jesse Barnes) said:

Jesse> Patch to fix spin_lock to not use ld.bias since it causes a Jesse> bunch of bus activity that hurts scalability when a lock is Jesse> contended.

Yes, using ld.bias really was wrong there. However, instead of applying your patch, I dropped the old lock code (which was effectively dead code) and instead removed the .bias from the ia64_spinlock_contention() routine in head.S.

Thanks,

	--david
#6 lore raw
In reply to Jesse Barnes
Quoted 10 lines · David Mosberger
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · On Mon, 21 Jul 2003 16:55:59 -0700, [email protected] (Jesse Barnes) said:

Jesse> Patch to fix spin_lock to not use ld.bias since it causes a Jesse> bunch of bus activity that hurts scalability when a lock is Jesse> contended.

Yes, using ld.bias really was wrong there. However, instead of applying your patch, I dropped the old lock code (which was effectively dead code) and instead removed the .bias from the ia64_spinlock_contention() routine in head.S.

Even better. Thanks.

Jesse