[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] Convert ram_list to RCU DQ V4,2

From: Mike Day
Subject: Re: [Qemu-devel] [PATCH] Convert ram_list to RCU DQ V4,2
Date: Mon, 9 Sep 2013 14:18:31 -0400

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!


reply via email to

[Prev in Thread] Current Thread [Next in Thread]