On Sat, 26 Sep 2009, Luck, Tony wrote:
>
> please pull from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux-2.6.git release
>
> This will update the files shown below.
>
> Tony Luck (1):
> [IA64] implement ticket locks for Itanium
Ok, so x86 has obviously done ticket locks for a while now, and I think
it's good that ia64 does too, but I wonder why you made the ticket lock be
a full 64-bit thing?
A plain 32-bit lock with a 16-bit shift should be plenty good enough, and
those spinlocks are embedded into a lot of structures where size might
matter. In fact, on x86, we do a 16-bit lock with a 8-bit shift for the
case where NR_CPU's is less than 256 (but part of that is that we can use
byte ops easily and access the high byte etc - it's not all about just the
size of the spinlock).
Also, your basic spinlock seems to be pessimised a bit:
+ int *p = (int *)&lock->lock, turn, now_serving;
+
+ now_serving = *p;
+ turn = ia64_fetchadd(1, p+1, acq);
+
+ if (turn == now_serving)
+ return;
+
+ do {
+ cpu_relax();
+ } while (ACCESS_ONCE(*p) != turn);
which has a couple of issues, foremost being the one that you force a
regular read first. Now, if the cacheline is already exclusive in your
local caches, that should be fine, but if it's not in your cache, then
you'll first fetch the cacheline for reading and possibly shared, and then
immediately afterwards you'll do that "fetchadd()" thing that turns it
dirty and exclusive.
It's likely _better_ to do the first access with a dirtying access, which
can bring in the cache-line for exclusive ownership immediately. Of
course, that depends on the exact details of ia64 cache coherency
protocol, but in general, I'd have expected
int *p = ((int *)&lock->lock, turn;
turn = ia64_fetchadd(1, p+1, acq);
while (ACCESS_ONCE(*p) != turn)
cpu_relax();
to actually be not only shorter, but better due to cache access patterns
too (ie do the read only _after_ you've done the write).
But I dunno. I'll pull the thing as-is, but I thought I'd point out that
this looks non-optimal.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|