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

[liu ping fan]

On Mon, Sep 3, 2012 at 6:16 PM, Avi Kivity <address@hidden> wrote:
> On 09/03/2012 01:06 PM, liu ping fan wrote:
>> On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity <address@hidden> wrote:
>>> On 09/03/2012 10:44 AM, liu ping fan wrote:
>>>>>>
>>>>>
>>>>> If we make the refcount/lock internal to the region, we must remove the
>>>>> opaque, since the region won't protect it.
>>>>>
>>>>> Replacing the opaque with container_of(mr) doesn't help, since we can't
>>>>> refcount mr, only mr->impl.
>>>>>
>>>> I think you mean if using MemoryRegionImpl, then at this level, we had
>>>> better not touch the refcnt of container_of(mr) and should face the
>>>> mr->impl->refcnt. Right?
>>>
>>> I did not understand the second part, sorry.
>>>
>> My understanding of "Replacing the opaque with container_of(mr)
>> doesn't help, since we can't  refcount mr, only
>> mr->impl." is that although Object_ref(container_of(mr)) can help us
>> to protect it from disappearing. But apparently it is not right place
>> to do it it in memory core.   Do I catch you meaning?
>
> We cannot call container_of(mr) in the memory core, because the
> parameters to container_of() are not known.  But that is not the main issue.
>
> Something we can do is make MemoryRegionOps::object() take a mr
> parameter instead of opaque.  MemoryRegionOps::object() then uses
> container_of() to derive the object to ref.
>
> (works with MemoryRegionOps::ref()/MemoryRegionOps::unref() too; Jan,
> this decouples Object from MemoryRegion at the cost of extra boilerplate
> in client code, but it may be worthwhile as a temporary measure while we
> gain more experience with this)
>
Exactly catch your meaning, thanks.
>>
>>>>> We could externalize the refcounting and push it into device code.  This
>>>>> means:
>>>>>
>>>>>    memory_region_init_io(&s->mem, dev)
>>>>>
>>>>>    ...
>>>>>
>>>>>    object_ref(dev)
>>>>>    memory_region_add_subregion(..., &dev->mr)
>>>>>
>>>>>    ...
>>>>>
>>>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>>>    object_unref(dev)
>>>>>
>>>> I think "object_ref(dev)" just another style to push
>>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>>> it.   The caller (unplug, pci-reconfig ) of
>>>> memory_region_del_subregion() ensure the @dev is valid.
>>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>>
>>> The above code has a deadlock.  memory_region_del_subregion() may be
>>> called under the device lock (since it may be the result of mmio to the
>>> device), and if the flush uses synchronized_rcu(), it will wait forever
>>> for the read-side critical section to complete.
>>>
>> But if _del_subregion() just wait for mr-X quiescent period, while
>> calling in mr-Y's read side critical section, then we can avoid
>> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>
> Look at cirrus-vga.c, there are many calls to the memory API there.
> While I doubt that we have one region disabling itself (or a subregion
> of itself), that's all code that would be run under the same device
> lock, and so would deadlock.
>
Oh, yes, quiescent time will never come since reader wait for the
lock! Got it, thanks.

pingfan
>
> --
> error compiling committee.c: too many arguments to function