On Mon, Sep 9, 2013 at 12:21 PM, Paolo Bonzini <
address@hidden> 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