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

[liu ping fan]

On Thu, Aug 30, 2012 at 1:40 AM, Avi Kivity <address@hidden> 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(&amp;cache_lock, flags);
>> > +        rcu_read_lock();
>> >          obj = __cache_find(id);
>> >          if (obj)
>> >                  object_get(obj);
>> > -        spin_unlock_irqrestore(&amp;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.
>
But I think the meaning of the memory region is for dispatching. If no
dispatching associated with mr, why need it exist in the system?  And
could you elaborate that who will be the ref holder of mr?

> 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.
>
Assume we can get the host objectX of opaque, then we can just
object_ref(objectX). So dispatching will not face the hard nut to
handle opaque.

Regards,
pingfan
> 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).
>
>>
>> >
>> >> 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.
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>