[Spice-devel] [spice-gtk 2/3] usbredir: Use atomic for UsbDeviceManager::event_thread_run

[Hans de Goede]

Hi,

On 30-06-16 09:13, Christophe Fergeau wrote:
> On Wed, Jun 29, 2016 at 07:20:16PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 29-06-16 17:42, Christophe Fergeau wrote:
>>> This variable is accessed from 2 different threads (main thread and USB
>>> event thread), so some care must be taken to read/write it.
>>
>> The event-thread only reads it, so I believe there is no need for this.
>
> https://en.wikipedia.org/wiki/Memory_ordering#Runtime_memory_ordering
> says that on most architectures stores can be reordered after loads

Right, the "bible" on this is:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/memory-barriers.txt

As this mostly is a kernel problem, as such I'm not aware of any
userspace code dealing with this, and the while(run) syntax is quite
normal.

Given that libusb_handle_events takes multiple locks it will act
as a full barrier and you do not really need atomics.

 > I'd
> read this as: main thread sets event_thread_run to FALSE, calls
> libusb_hotplug_deregister_callback(), usb event thread returns from
> libusb_handle_events(), reads event_thread_run, but reads the old value
> (TRUE) rather than the FALSE causing it to exit. I agree that it's
> likely that either libusb_hotplug_deregister_callback() and/or
> libusb_handle_events() are going to trigger some barrier, but it feels
> safer to me to handle the concurrency ourselves.

It certainly cannot hurt but it is not necessary.

 >> event_thread_run is a bool, you should make it a gint, probably even volatile.
 >
 > I've changed it to gint in v2, I don't know about the volatile, are
 > there cases where not having it as a volatile is going to cause issues?
 > Code using GAtomic in glib does not seem to be using volatile.

If you use atomic accesses you definitely do not need to use volatile.

Volatile is needed for something like this (typical cs example):

int event_ready;

thread_run_function()
{
	while (true) {
		while (!event_ready) {}
		handle_event();
	}
}

Which the compile will typically optimize too:

thread_run_function()
{
	while (true) {
		if (!event_ready)
			while (true) {}
		handle_event();
	}
}

At which point the thread will just hang
if event_ready is false once, marking
event_ready volatile fixes this as it tells
the compiler to not optimize away any
event_ready accesses (be it reads or writes).

Note that this example is a bit microcontroller
oriented under a modern os one would typically
do e.g.:

thread_run_function()
{
	while (true) {
		while (!event_ready)
			msleep(10);
		handle_event();
	}
}

To relax the cpu, and the compile does cannot know
if the msleep() call does or does not touch the
event_ready global, so it will not do the troublesome
optimization, assuming that msleep() may modify it.


Another textbook example is:

u8 *iomem;

iomem[5] = 7;
iomem[5] = 8;

Which the compiler will optimize to:

iomem[5] = 8;

Which when talking to actually memory mapped io
which you're trying to run through various internal
states is bad, again volatile fixes this.

Regards,

Hans