[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] QEMUBH: make AioContext's bh re-entrant
From: |
liu ping fan |
Subject: |
Re: [Qemu-devel] [PATCH] QEMUBH: make AioContext's bh re-entrant |
Date: |
Sun, 16 Jun 2013 18:00:46 +0800 |
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.