Re: [Qemu-devel] [PATCH v5 07/18] qemu-thread: add simple test-and-set spinlock

[Emilio G. Cota]

On Tue, May 17, 2016 at 20:13:24 +0300, Sergey Fedorov wrote:
> On 14/05/16 06:34, Emilio G. Cota wrote:
(snip)
> > +static inline void qemu_spin_lock(QemuSpin *spin)
> > +{
> > +    while (atomic_test_and_set_acquire(&spin->value)) {
> 
> From gcc-4.8 info page, node "__atomic Builtins", description of
> __atomic_test_and_set():
> 
>     It should be only used for operands of type 'bool' or 'char'.

Yes I'm aware of that. The way I interpret it is that if you're
storing something other than something ~= bool, you might
be in trouble, since it might get cleared.
We use 'int' but effectively store here a bool, so we're safe.

As to why we're using int, see
  http://thread.gmane.org/gmane.comp.emulators.qemu/405812/focus=405965

> > +        while (atomic_read(&spin->value)) {
> > +            cpu_relax();
> > +        }
> > +    }
> Looks like relaxed atomic access can be a subject to various
> optimisations according to
> https://gcc.gnu.org/wiki/Atomic/GCCMM/AtomicSync#Relaxed.

The important thing here is that the read actually happens
on every iteration; this is achieved with atomic_read().
Barriers etc. do not matter here because once we exit
the loop, the try to acquire the lock -- and if we succeed,
we then emit the right barrier.

> > +static inline bool qemu_spin_locked(QemuSpin *spin)
> > +{
> > +    return atomic_read_acquire(&spin->value);
> 
> Why not just atomic_read()?

I think atomic_read() is better, yes. I'll change it. I went
with the fence because I wanted to have at least a caller
of atomic_read_acquire :P

I also hesitated between calling it _locked or _is_locked;
I used _locked for consistency with qemu_mutex_iothread_locked,
although I think _is_locked is a bit clearer:
qemu_spin_locked(foo)
   is a little too similar to
qemu_spin_lock(foo).

Thanks,

                Emilio