qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Avi Kivity
Subject: Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem
Date: Wed, 29 Aug 2012 10:40:50 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0

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(&cache_lock, flags);
> > +        rcu_read_lock();
> >          obj = __cache_find(id);
> >          if (obj)
> >                  object_get(obj);
> > -        spin_unlock_irqrestore(&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.

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.

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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]