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

[Avi Kivity <avi@redhat.com>]

On 08/30/2012 12:08 AM, Jan Kiszka wrote:
> >>>
> >>> 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?
>
> Where did I say that memory regions should no longer be used for
> dispatching? The point is to keep the clean layer separation between
> memory regions and device objects instead of merging them together.

Believe me, that's exactly how I feel.  I guess no author of a module
wants to see it swallowed by a larger framework and see its design
completely changed.  Modules should be decoupled as much as possible. 
But I don't see a better way yet.

>
> Given
>
>        Object
>        ^    ^
>        |    |
> Region 1    Region 2
>
> you protect the object during dispatch, implicitly (and that is bad)
> requiring that no region must change in that period.

We only protect the object against removal.  After object_ref() has run,
the region may still be disabled.

>  I say what rather
> needs protection are the regions so that Region 2 can pass away and
> maybe reappear independent of Region 1.

Region 2 can go away until the device-supplied dispatch code takes the
device lock.  If the device chooses to use finer-grained locking, it can
allow region 2 (or even region 1) to change during dispatch.  The only
requirement is that region 1 is not destroyed.

>  And: I won't need to know the
> type of that object the regions are referring to in this model. That's
> the difference.

Sorry, I lost the reference.  What model?  Are you referring to my
broken MemoryRegionImpl split?

> >  And
> > could you elaborate that who will be the ref holder of mr?
>
> The memory subsystem while running a memory region handler. If that will
> be a reference counter or a per-region lock like Avi suggested, we still
> need to find out.
>

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.

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)

er, this must be wrong, since it's so simple

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.