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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 19/06/2013 00:26, mdroth ha scritto:
> On Tue, Jun 18, 2013 at 09:20:26PM +0200, Paolo Bonzini wrote:
>> Il 18/06/2013 17:14, mdroth ha scritto:
>>> Could we possibly simplify this by introducing a recursive mutex that we
>>> could use to protect the whole list loop and hold even during the cb?
>>
>> If it is possible, we should avoid recursive locks.  It makes impossible
>> to establish a lock hierarchy.  For example:
>>
>>> I assume we can't hold the lock during the cb currently since we might
>>> try to reschedule, but if it's a recursive mutex would that simplify
>>> things?
>>
>> If you have two callbacks in two different AioContexts, both of which do
>> bdrv_drain_all(), you get an AB-BA deadlock
> 
> I think I see what you mean. That problem exists regardless of whether we
> introduce a recursive mutex though right?

Without a recursive mutex, you only hold one lock at a time in each thread.

> I guess the main issue is the
> fact that we'd be encouraging sloppy locking practices without
> addressing the root problem?

Yeah.  We're basically standing where the Linux kernel stood 10 years
ago (let's say 2.2 timeframe).  If Linux got this far without recursive
mutexes, we can at least try. :)

> I'm just worried what other subtle problems pop up if we instead rely
> heavily on memory barriers and inevitably forget one here or there, but
> maybe that's just me not having a good understanding of when to use them.

That's true.  I hope that the docs in patch 1 help, and (much) more
thorough docs are available in the Linux kernel.

> But doesn't rcu provide higher-level interfaces for these kinds of things?
> Is it possible to hide any of this behind our list interfaces?

My atomics header file provides higher-level interfaces, but in most
cases barriers are not that harder to use and the docs help converting
one style to the other.

So far I've not used RCU for lists, only for entire data structures.

Paolo