On Mon, Sep 9, 2013 at 12:21 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > > > @@ -601,12 +608,22 @@ static void reset_ram_globals(void) > > last_seen_block = NULL; > > last_sent_block = NULL; > > last_offset = 0; > > - last_version = ram_list.version; > > ram_bulk_stage = true; > > + smp_wmb(); > > + last_version = ram_list.version; > > This barrier is still not needed.
Yes, I agree, because of the calling context. It is brittle though because reset_ram_globals is a static function and may be called differently in the future (or by new code at a different location). The need for a barrier may change and it would be opaque to the internals of the reset function. Also, the globals are writable elsewhere in the file. It needs more organization but I agree that should be a discrete patch. > Can you take a look at my rcu branch? The comments clarify to me why writing to last_mru does not need a write barrier, thank you. > I pushed there the conversion of mru_block to RCU (based on 4.1), and > clarified a bit more the locking conventions. Basically we have: > > - functions called under iothread lock (e.g. qemu_ram_alloc) > > - functions called under RCU or iothread lock (e.g. qemu_get_ram_ptr) > > - functions called under RCU or iothread lock, or while holding a > reference to the MemoryRegion that is looked up again (e.g. > qemu_ram_addr_from_host). > > The difference between the second and third group is simply that the > second will not call rcu_read_lock()/rcu_read_unlock(), the third will. > > Does it make sense? Should we simplify it by always calling > rcu_read_lock()/rcu_read_unlock(), which removes the second group? I think the benefits of simplification and future reliability are greater than the performance cost of the rcu_read_lock. The latter should be something we verify, though. Thank you! Mike
