Re: [Xen-devel] [PATCH 00/10] [PATCH RFC V2] Paravirtualized ticketlocks

[Jeremy Fitzhardinge <jeremy@goop.org>]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

On 10/06/2011 07:04 AM, Stephan Diestelhorst wrote:
> On Wednesday 28 September 2011, 14:49:56 Linus Torvalds wrote:
>> Which certainly should *work*, but from a conceptual standpoint, isn't
>> it just *much* nicer to say "we actually know *exactly* what the upper
>> bits were".
> Well, we really do NOT want atomicity here. What we really rather want
> is sequentiality: free the lock, make the update visible, and THEN
> check if someone has gone sleeping on it.
>
> Atomicity only conveniently enforces that the three do not happen in a
> different order (with the store becoming visible after the checking
> load).
>
> This does not have to be atomic, since spurious wakeups are not a
> problem, in particular not with the FIFO-ness of ticket locks.
>
> For that the fence, additional atomic etc. would be IMHO much cleaner
> than the crazy overflow logic.

All things being equal I'd prefer lock-xadd just because its easier to
analyze the concurrency for, crazy overflow tests or no.  But if
add+mfence turned out to be a performance win, then that would obviously
tip the scales.

However, it looks like locked xadd is also has better performance:  on
my Sandybridge laptop (2 cores, 4 threads), the add+mfence is 20% slower
than locked xadd, so that pretty much settles it unless you think
there'd be a dramatic difference on an AMD system.

(On Nehalem it was much less dramatic 2% difference, but still in favour
of locked xadd.)

This is with dumb-as-rocks run it in a loop with "time" benchmark, but
the results are not very subtle.

    J

[-- Attachment #2: add-barrier.c --]
[-- Type: text/x-csrc, Size: 285 bytes --]

#include <stdio.h>

struct {
	unsigned char flag;
	unsigned char val;
} l;

int main(int argc, char **argv)
{
	int i;

	for (i = 0; i < 100000000; i++) {
		l.val += 2;
		asm volatile("mfence" : : : "memory");
		if (l.flag)
			break;
		asm volatile("" : : : "memory");
	}

	return 0;
}

[-- Attachment #3: locked-xadd.c --]
[-- Type: text/x-csrc, Size: 422 bytes --]

#include <stdio.h>

union {
	struct {
		unsigned char val;
		unsigned char flag;
	};
	unsigned short lock;
} l = { 0,0 };

int main(int argc, char **argv)
{
	int i;

	for (i = 0; i < 100000000; i++) {
		unsigned short inc = 2;
		if (l.val >= (0x100 - 2))
			inc += -1 << 8;
		asm volatile("lock; xadd %1,%0" : "+m" (l.lock), "+r" (inc) : );
		if (inc & 0x100)
			break;
		asm volatile("" : : : "memory");
	}

	return 0;
}