Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-08-29 19:40, Avi Kivity wrote:
> On 08/29/2012 10:30 AM, Jan Kiszka wrote:
>> On 2012-08-29 19:23, Avi Kivity wrote:
>>> On 08/28/2012 02:42 AM, Jan Kiszka wrote:
>>>>
>>>> Let's not talk about devices or MMIO dispatching. I think the problem is
>>>> way more generic, and we will face it multiple times in QEMU. 
>>>
>>> The problem exists outside qemu as well.  It is one of the reasons for
>>> the popularity of garbage collection IMO, and the use of reference
>>> counting when GC is not available.
>>>
>>> This pattern is even documented in
>>> Documentation/DocBook/kernel-locking.tmpl:
>>>
>>> @@ -104,12 +114,11 @@
>>>  struct object *cache_find(int id)
>>>  {
>>>          struct object *obj;
>>> -        unsigned long flags;
>>>
>>> -        spin_lock_irqsave(&cache_lock, flags);
>>> +        rcu_read_lock();
>>>          obj = __cache_find(id);
>>>          if (obj)
>>>                  object_get(obj);
>>> -        spin_unlock_irqrestore(&cache_lock, flags);
>>> +        rcu_read_unlock();
>>>   
>>>
>>> Of course that doesn't mean we should use it, but at least it indicates
>>> that it is a standard pattern.  With MemoryRegion the pattern is
>>> changed, since MemoryRegion is a thunk, not the object we're really
>>> dispatching to.
>>
>> We are dispatching according to the memory region (parameters, op
>> handlers, opaques). If we end up in device object is not decided at this
>> level. A memory region describes a dispatchable area - not to confuse
>> with a device that may only partially be able to receive such requests.
> 
> Correct.
> 
> Let's experiment with refcounting MemoryRegion.  We can move the entire
> contents of MemoryRegion to MemoryRegionImpl, add a reference count (to
> MemoryRegionImpl), and change MemoryRegion to contain a pointer to the
> refcounted MemoryRegionImpl:
> 
> struct MemoryRegion {
>     struct MemoryRegionImpl *impl;
> };
> 
> struct MemoryRegionImpl {
>     atomic int refs;
>     ...
> };
> 
> The memory core can then store MemoryRegion copies (with elevated
> refcounts) instead of pointers.  Devices can destroy MemoryRegions at
> any time, the implementation will not go away.  However, what of the
> opaque stored in MemoryRegionImpl?  It becomes a dangling pointer.
> 
> One way out is to add a lock to MemoryRegionImpl.  Dispatch takes the
> lock, examines the ->enabled member, and bails out if it is cleared. 
> The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lock,
> clears ->enabled,  releases the lock, and drops the reference.

That means holding the MemoryRegionImpl lock across the handler call?

> 
> The advantage to this scheme is that all changes are localized to the
> memory core, no need for a full sweep.  It is a little complicated, but
> we may be able to simplify it (or find another one).

May work. We just need to detect if memory region tries to delete itself
from its handler to prevent the deadlock.

> 
>>
>>>
>>>> Besides
>>>> MMIO and PIO dispatching, it will haunt us for file or event handlers,
>>>> any kind of callbacks etc.
>>>>
>>>> Context A                               Context B
>>>> ---------                               ---------
>>>>                                         object = lookup()
>>>> deregister(object)
>>>> modify(object) -> invalid state
>>>> ...                                     use(object)
>>>> modify(object) -> valid state
>>>> register(object)
>>>>
>>>> And with "object" I'm not talking about QOM but any data structure.
>>>>
>>>
>>>
>>> Context B
>>> ---------
>>> rcu_read_lock()
>>> object = lookup()
>>> if (object) {
>>>     ref(object)
>>> }
>>> rcu_read_unlock()
>>>
>>> use(object)
>>>
>>> unref(object)
>>>
>>> (substitute locking scheme to conform to taste and/or dietary
>>> restrictions)   
>>>
>>
>> + wait for refcount(object) == 0 in deregister(object). That's what I'm
>> proposing.
> 
> Consider timer_del() called from a timer callback.  It's often not doable.

This is inherently synchronous already (when working against the same
alarm timer backend). We can detect this in timer_del and skip waiting,
as in the above scenario.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux