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

[Avi Kivity]

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).

> I think the key point is that original code SYNC eliminate all the
> readers of DeviceState at acpi_piix_eject_slot() by
> dev->unit()/exit(), so each subsystem will no access it in future.
> But now, we can delete the DeviceState async.

But deleting happens when we are guaranteed to have no I/O dispatch.

> Currently, we can just use e1000->unmap() to detach itself from each
> subsystem(Not implemented in this series patches for timer,...) to
> achieve the goal, because their readers are still under the protection
> of big lock, but when they are out of big lock, we need extra effort
> like memory system.

I see what you mean.  So you defer the deletion to a context where the
big lock is held.

But this solves nothing.  The device model accesses the network stack
and timer subsystem without the big lock held.  So you either need to
thread those two subsystems, or take the big lock in the I/O handlers.
If you do that, you can also take the big lock in the destructor.  If we
make the big lock a recursive lock, then the destructor can be invoked
in any context.

To summarize, I propose:
- drop the reclaimer
- make the bql recursive
- take the bql in the e1000 destructor
- take the bql in the e1000 I/O handlers when it accesses the timer or
network subsystems
(rest for a bit)
- thread the timer subsystem
- drop bql from around timer accesses
- thread the network subsystem
- drop bql from e1000 I/O handlers and destructor

does this work?

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