On Mon, Sep 3, 2012 at 6:16 PM, Avi Kivity <a...@redhat.com> wrote: > On 09/03/2012 01:06 PM, liu ping fan wrote: >> On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity <a...@redhat.com> wrote: >>> On 09/03/2012 10:44 AM, liu ping fan wrote: >>>>>> >>>>> >>>>> 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. >>>>> >>>> I think you mean if using MemoryRegionImpl, then at this level, we had >>>> better not touch the refcnt of container_of(mr) and should face the >>>> mr->impl->refcnt. Right? >>> >>> I did not understand the second part, sorry. >>> >> My understanding of "Replacing the opaque with container_of(mr) >> doesn't help, since we can't refcount mr, only >> mr->impl." is that although Object_ref(container_of(mr)) can help us >> to protect it from disappearing. But apparently it is not right place >> to do it it in memory core. Do I catch you meaning? > > We cannot call container_of(mr) in the memory core, because the > parameters to container_of() are not known. But that is not the main issue. > > Something we can do is make MemoryRegionOps::object() take a mr > parameter instead of opaque. MemoryRegionOps::object() then uses > container_of() to derive the object to ref. > > (works with MemoryRegionOps::ref()/MemoryRegionOps::unref() too; Jan, > this decouples Object from MemoryRegion at the cost of extra boilerplate > in client code, but it may be worthwhile as a temporary measure while we > gain more experience with this) > Exactly catch your meaning, thanks. >> >>>>> 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) >>>>> >>>> I think "object_ref(dev)" just another style to push >>>> MemoryRegionOps::object() to device level. And I think we can bypass >>>> it. The caller (unplug, pci-reconfig ) of >>>> memory_region_del_subregion() ensure the @dev is valid. >>>> If the implied flush is implemented in synchronize, _del_subregion() >>>> will guarantee no reader for @dev->mr and @dev exist any longer. >>> >>> The above code has a deadlock. memory_region_del_subregion() may be >>> called under the device lock (since it may be the result of mmio to the >>> device), and if the flush uses synchronized_rcu(), it will wait forever >>> for the read-side critical section to complete. >>> >> But if _del_subregion() just wait for mr-X quiescent period, while >> calling in mr-Y's read side critical section, then we can avoid >> deadlock. I saw in pci-mapping, we delete mr-X in mr-Y read side. > > Look at cirrus-vga.c, there are many calls to the memory API there. > While I doubt that we have one region disabling itself (or a subregion > of itself), that's all code that would be run under the same device > lock, and so would deadlock. > Oh, yes, quiescent time will never come since reader wait for the lock! Got it, thanks.
pingfan > > -- > error compiling committee.c: too many arguments to function
