Re: [Qemu-devel] [PATCH V3 08/11] qom: introduce reclaimer to release obj in async

[Avi Kivity]

On 09/13/2012 12:59 PM, liu ping fan wrote:
> On Thu, Sep 13, 2012 at 4:45 PM, Avi Kivity <address@hidden> wrote:
>> On 09/13/2012 09:54 AM, liu ping fan wrote:
>>> On Tue, Sep 11, 2012 at 5:37 PM, Avi Kivity <address@hidden> wrote:
>>>> On 09/11/2012 12:32 PM, liu ping fan wrote:
>>>>> On Tue, Sep 11, 2012 at 4:32 PM, Avi Kivity <address@hidden> wrote:
>>>>>> On 09/11/2012 10:51 AM, Liu Ping Fan wrote:
>>>>>>> From: Liu Ping Fan <address@hidden>
>>>>>>>
>>>>>>> DeviceState will be protected by refcnt from disappearing during
>>>>>>> dispatching. But when refcnt comes down to zero, DeviceState may
>>>>>>> be still in use by iohandler, timer etc in main loop, we just delay
>>>>>>> its free untill no reader.
>>>>>>>
>>>>>>
>>>>>> How can this be?  We elevate the refcount while dispatching I/O.  If we
>>>>>> have similar problems with the timer, we need to employ a similar 
>>>>>> solution.
>>>>>>
>>>>> Yes, at the next step, plan to covert iohandler, timer etc to use
>>>>> refcount as memory. Here just a temp solution.
>>>>
>>>> I prefer not to ever introduce it.
>>>>
>>>> What we can do is introduce a sub-region for e1000's mmio that will take
>>>> only the device lock, and let original region use the old dispatch path
>>>> (and also take the device lock).  As we thread the various subsystems
>>>> e1000 uses, we can expand the sub-region until it covers all of e1000's
>>>> functions, then fold it back into the main region.
>>>>
>>> Introducing new sub-region for e1000  seems no help to resolve this
>>> issue. It can not tell whether main-loop still use it or not.
>>
>> What is "it" here? (actually two of them).
>>
> Should expressed as "The sub-region's dispatcher can not tell whether
> main-loop still use e1000 or not"

The sub-region will not use any unthreaded subsystems, so it need not
care about the main loop.

At first, it would only access registers in device state.

But if we go with the plan below, we can drop it.

> 
>> If you do that, you can also take the big lock in the destructor.  If we
> 
> We do not call qemu_del_timer() etc at the destructor, instead, we
> will call it in qdev_unplug_complete() -->e1000::unmap(). And
> e1000::unmap() is the only function definitely called under bql. When
> coming to destructor, the DeviceState has been completely isolated
> from all of the subsystem. So no need to require big lock in
> destructor.

But between unmap() and the destructor, accesses can still occur (an
access that was started before unmap() was called, but was delayed and
is dispatched after it completes).  These accesses will find the timer
deleted, and so must be prepared to check if the timer is there or not.

So we have a choice, either move timer destruction to the destructor, or
add checks in the dispatch code.



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