On Thu, Jun 20, 2013 at 4:16 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > Il 20/06/2013 09:39, Stefan Hajnoczi ha scritto: >> qemu_bh_cancel() and qemu_bh_delete() are not modified by this patch. >> >> It seems that calling them from a thread is a little risky because there >> is no guarantee that the BH is no longer invoked after a thread calls >> these functions. >> >> I think that's worth a comment or do you want them to take the lock so >> they become safe? > > Taking the lock wouldn't help. The invoking loop of aio_bh_poll runs > lockless. I think a comment is better. > Yes, will document it. > qemu_bh_cancel is inherently not thread-safe, there's not much you can > do about it. > > qemu_bh_delete is safe as long as you wait for the bottom half to stop > before deleting the containing object. Once we have RCU, deletion of > QOM objects will be RCU-protected. Hence, a simple way could be to put > the first part of aio_bh_poll() within rcu_read_lock/unlock. > >> The other thing I'm unclear on is the ->idle assignment followed >> immediately by a ->scheduled assignment. Without memory barriers >> aio_bh_poll() isn't guaranteed to get an ordered view of these updates: >> it may see an idle BH as a regular scheduled BH because ->idle is still >> 0. > > Right. You need to order ->idle writes before ->scheduled writes, and > add memory barriers, or alternatively use two bits in ->scheduled so > that you can assign both atomically. > I think just shift the position of smp_rmb/wmb in _schedule and _poll, we can acheive this (callbacks will not refer to ->idle)
Regards, Pingfan > Paolo
