Dear, Ard and maintainers

We are concerning that ReleaseSpinLock() does not have a memory barrier. This is reported to https://bugzilla.tianocore.org/show_bug.cgi?id=3005. We’d like to hear from you whether current implementation needs improvement or not.

The concern comes from 'weak memory ordering' and multi-core. (we are using AARCH64) And the scenario that we’re concerning is like below:

AcquireSpinLock(); // contains ‘dmb sy’ and prevents "a = *b" from moving up (and unnecessarily prevents other things from moving down)

a = *b;

a = a + 1;

*b = a;

ReleaseSpinLock(); // No write barrier here, so "*b = a" can move down. Another core acquires the spinlock and can read stale data

Please let me know if it would be helpful to add MemoryFence like below:

SPIN_LOCK *

EFIAPI

ReleaseSpinLock (

IN OUT SPIN_LOCK *SpinLock

)

{

SPIN_LOCK LockValue;

ASSERT (SpinLock != NULL);

MemoryFence();

LockValue = *SpinLock;

ASSERT (SPIN_LOCK_ACQUIRED == LockValue || SPIN_LOCK_RELEASED == LockValue);

*SpinLock = SPIN_LOCK_RELEASED;

return SpinLock;

}

MemoryFence is implemented with 'dmb', but I just wonder if it is okay to not implement it with 'dsb'.

* Attaching linux documentation describing SMP barrier pairing

https://github.com/torvalds/linux/blob/master/Documentation/memory-barriers.txt


SMP BARRIER PAIRING
	-------------------

	When dealing with CPU-CPU interactions, certain types of memory barrier should
	always be paired. A lack of appropriate pairing is almost certainly an error.

	General barriers pair with each other, though they also pair with most
	other types of barriers, albeit without multicopy atomicity. An acquire
	barrier pairs with a release barrier, but both may also pair with other
	barriers, including of course general barriers. A write barrier pairs
	with a data dependency barrier, a control dependency, an acquire barrier,
	a release barrier, a read barrier, or a general barrier. Similarly a
	read barrier, control dependency, or a data dependency barrier pairs
	with a write barrier, an acquire barrier, a release barrier, or a
	general barrier:

	CPU 1 CPU 2
	=============== ===============
	WRITE_ONCE(a, 1);
	<write barrier>
	WRITE_ONCE(b, 2); x = READ_ONCE(b);
	<read barrier>
	y = READ_ONCE(a);

	Or:

	CPU 1 CPU 2
	=============== ===============================
	a = 1;
	<write barrier>
	WRITE_ONCE(b, &a); x = READ_ONCE(b);
	<data dependency barrier>
	y = *x;

	Or even:

	CPU 1 CPU 2
	=============== ===============================
	r1 = READ_ONCE(y);
	<general barrier>
	WRITE_ONCE(x, 1); if (r2 = READ_ONCE(x)) {
	<implicit control dependency>
	WRITE_ONCE(y, 1);
	}

	assert(r1 == 0 \|\| r2 == 0);

	Basically, the read barrier always has to be there, even though it can be of
	the "weaker" type.

	[!] Note that the stores before the write barrier would normally be expected to
	match the loads after the read barrier or the data dependency barrier, and vice
	versa:

	CPU 1 CPU 2
	=================== ===================
	WRITE_ONCE(a, 1); }---- --->{ v = READ_ONCE(c);
	WRITE_ONCE(b, 2); } \ / { w = READ_ONCE(d);
	<write barrier> \ <read barrier>
	WRITE_ONCE(c, 3); } / \ { x = READ_ONCE(a);
	WRITE_ONCE(d, 4); }---- --->{ y = READ_ONCE(b);

Thanks,

Bin

From: bugzilla-daemon@bugzilla.tianocore.org <bugzilla-daemon@bugzilla.tianocore.org>
Sent: Wednesday, November 4, 2020 10:44 AM
To: Bin, Sung-Uk (Bin) <sunguk-bin@hp.com>
Subject: [Bug 3005] ReleaseSpinLock() requires a barrier at the beginning