Re: [Qemu-devel] [PATCH 16/16] cpus-common: lock-free fast path for cpu_exec_start/end

[Paolo Bonzini <pbonzini@redhat.com>]

----- Original Message -----
> From: "Richard Henderson" <rth@twiddle.net>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> Cc: "serge fdrv" <serge.fdrv@gmail.com>, cota@braap.org, "alex bennee" <alex.bennee@linaro.org>, "sergey fedorov"
> <sergey.fedorov@linaro.org>
> Sent: Friday, September 23, 2016 8:23:46 PM
> Subject: Re: [Qemu-devel] [PATCH 16/16] cpus-common: lock-free fast path for cpu_exec_start/end
> 
> On 09/23/2016 12:31 AM, Paolo Bonzini wrote:
> > +        if (atomic_read(&other_cpu->running)) {
> ...
> > +    atomic_set(&cpu->running, true);
> ...
> > +            cpu->running = false;
> ...
> > +            cpu->running = true;
> 
> Inconsistent use of atomics.  I don't see that the cpu_list_lock protects the
> last two lines in any way.

It does:

        qemu_mutex_lock(&qemu_cpu_list_lock);
        if (!cpu->has_waiter) {
            /* Not counted in pending_cpus, let the exclusive item
             * run.  Since we have the lock, just set cpu->running to true
             * while holding it; no need to check pending_cpus again.
             */
            cpu->running = false;
            exclusive_idle();
            /* Now pending_cpus is zero.  */
            cpu->running = true;
        } else {
            /* Counted in pending_cpus, go ahead and release the
             * waiter at cpu_exec_end.
             */
        }
        qemu_mutex_unlock(&qemu_cpu_list_lock);

but I can change it anyway to atomic_set.

Paolo