On Sat, Sep 1, 2012 at 4:46 PM, Avi Kivity <a...@redhat.com> wrote: > 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. > 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?
> 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. So I think we can save both object_ref/unref(dev) for memory system. The only problem is that whether we can implement it as synchronous or not, is it possible that we can launch a _del_subregion(mr-X) in mr-X's dispatcher? Regards, pingfan > 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. >
