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

2016-09-26 Thread Paolo Bonzini
On 26/09/2016 09:28, Alex Bennée wrote: > > cpu->running is only read under the mutex, but can be written _by the > > owner thread only_ outside the mutex. So writes outside the mutex must > > be atomic, but writes under the mutex don't because: > > > > - no other thread ever writes to cpu->runn

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

2016-09-26 Thread Alex Bennée
Paolo Bonzini writes: > On 24/09/2016 22:43, Richard Henderson wrote: 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); >> >> What I meant is that I don't see that the mutex avoids the need

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

2016-09-26 Thread Paolo Bonzini
On 24/09/2016 22:43, Richard Henderson wrote: >>> 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); > > What I meant is that I don't see that the mutex avoids the need for > atomic_set. Oh, I see. c

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

2016-09-24 Thread Richard Henderson
, 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->runn

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

2016-09-24 Thread Paolo Bonzini
- Original Message - > From: "Richard Henderson" > To: "Paolo Bonzini" , qemu-devel@nongnu.org > Cc: "serge fdrv" , c...@braap.org, "alex bennee" > , "sergey fedorov" > > Sent: Friday, September 23, 2016 8:23:46 PM &

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

2016-09-23 Thread Richard Henderson
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 th

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

2016-09-22 Thread Paolo Bonzini
On 22/09/2016 00:27, Emilio G. Cota wrote: > Another suggestion is to consistently access pending_cpus atomically, > since now we're accessing it with and without the CPU list mutex held. Yeah, that's a bit of a pain in the ass, but it's a good idea. Paolo

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

2016-09-21 Thread Emilio G. Cota
On Mon, Sep 19, 2016 at 14:50:59 +0200, Paolo Bonzini wrote: (snip) > @@ -212,24 +216,75 @@ void end_exclusive(void) > /* Wait for exclusive ops to finish, and begin cpu execution. */ > void cpu_exec_start(CPUState *cpu) > { > -qemu_mutex_lock(&qemu_cpu_list_mutex); > -exclusive_idle();

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

2016-09-21 Thread Emilio G. Cota
On Wed, Sep 21, 2016 at 20:19:18 +0200, Paolo Bonzini wrote: (snip) > No, this is not true. Barriers order stores and loads within a thread > _and_ establish synchronizes-with edges. > > In the example above you are violating causality: > > - cpu0 stores cpu->running before loading pending_cpus

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

2016-09-21 Thread Paolo Bonzini
On 21/09/2016 19:24, Emilio G. Cota wrote: > On Mon, Sep 19, 2016 at 14:50:59 +0200, Paolo Bonzini wrote: >> Set cpu->running without taking the cpu_list lock, only look at it if >> there is a concurrent exclusive section. This requires adding a new >> field to CPUState, which records whether a

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

2016-09-21 Thread Emilio G. Cota
On Mon, Sep 19, 2016 at 14:50:59 +0200, Paolo Bonzini wrote: > Set cpu->running without taking the cpu_list lock, only look at it if > there is a concurrent exclusive section. This requires adding a new > field to CPUState, which records whether a running CPU is being counted > in pending_cpus. W