Re: [Qemu-devel] [PATCH] QEMUBH: make AioContext's bh re-entrant

[liu ping fan]

On Sat, Jun 15, 2013 at 11:44 PM, Paolo Bonzini <address@hidden> wrote:
> Il 15/06/2013 07:16, Paolo Bonzini ha scritto:
>> ... I'm not sure that this works yet.  I see two problems:
>> ctx->walking_bh needs to be accessed atomic, and while you are doing the
>> deletions somebody could come up and start a read.  Havoc.
>
> Hmm, are you just trying to protect aio_bh_poll from aio_bh_new, while
> still having only one thread that can do aio_bh_poll (multiple producer,
> single consumer)?
>
Yes :)
> In this case what you have should work and this patch is almost ready
> for commit.  In fact it's actually important to have it for the
> dataplane stuff that Stefan is doing, I think.
>
> But _please document what you are doing_!  It's not the first time that
> I tell you this: locking policy must be _thoroughly_ documented, and
> _not_ figured out by the reviewers.  Without this, I'm not going to give
> my Reviewed-by.
>
Ok, will document it.
> Regarding barriers, there is another write barrier you need to add
> before anything that sets bh->scheduled.  This makes sure any writes
> that are needed by the callback are done before the locations are read
> in the aio_bh_poll thread.  Similarly, you want a matching read barrier
> before calling the callback.
>
Will fix.
> Please pick up the atomics patch from my branch so that you have the
> smp_read_barrier_depends() primitive; and resubmit with documentation
> about the locking policy in the commit message _and_ the header files.
> Also please add a comment before each barrier saying what is ordered
> against what.
>
Ok.
Thanks and regards,
Pingfan

> Thanks,
>
> Paolo
>
>> It's not an easy problem, because even with RCU the bottom half callback
>> should run _outside_ the RCU critical section.  I suggest that you hunt
>> the kernel for something that is trying to do the same.