Re: [Qemu-devel] [PATCH v7 0/7] push mmio dispatch out of big lock

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-05-06 10:07, Paolo Bonzini wrote:
> Il 04/05/2013 12:42, Jan Kiszka ha scritto:
>> On 2013-05-04 11:47, Paolo Bonzini wrote:
>>> Il 03/05/2013 10:04, Jan Kiszka ha scritto:
>>>> We can't change the semantics of opaque as long as old_mmio /
>>>> old_portio are around. But we need a flag anyway to indicate if
>>>> a region is depending on BQL or not. Adding a separate "Object
>>>> *owner" to MemoryRegion can serve both purposes. Then we define
>>>> something like
>>>>
>>>> void memory_region_set_local_locking(MemoryRegion *mr, bool
>>>> local_locking, Object *owner);
>>>>
>>>> to control the property (if local_locking is true, owner must
>>>> be non-NULL, of course). That's quite similar to my old
>>>> prototype here that had
>>>> memory_region_set/clear_global_locking.
>>>
>>> I think setting the owner can be done separately from enabling
>>> local lock.  For example, memory_region_find could also have a
>>> variant that adds a ref to the owner.  It would be very similar
>>> to what Ping Fan is doing in the virtio-dataplane's HostMem data
>>> structure.
> 
>> That's trivial to break up, but I'm not sure if there will be
>> reasonable scenarios where a region requires reference counting
>> without being able to work without the BQL. RAM, e.g., should
>> always work BQL-free (once we have the infrastructure in place).
> 
> I think we need to add an owner to all regions (tedious, but
> doable---perhaps even scriptable).  The current code covers
> address_space_rw, but memory_region_find remains callable only from
> BQL-protected regions.  The caller of memory_region_find needs to be
> able to inspect the MemoryRegion, even if it is just to fail on
> non-RAM regions.  I would like to switch the dataplane code to use
> memory_region_find.
> 
> BTW, have you seen
> http://lwn.net/SubscriberLink/548909/b6fdd846f1232be6/ ? [*]  Perhaps
> we can adopt something like that, it solves the same exact problem
> that we have, and it's a well-known solution from the literature.

That looks like a more handy wrapper around mutex_trylock + recursive locks.

But the problem is not the locking mechanism. The issue is the
(non-existing) roll-back logic in the device models. That's the tedious
work - if we would like to go that way. But I'm still optimistic we can
avoid it. We may need lock state inspection (mutex_is_locked), playing
with this ATM.

> 
> 	[*]  The "subscriber link" mechanism allows an LWN.net
> 	     subscriber to generate a special URL for a
> 	     subscription-only article. That URL can then be given to
> 	     others, who will be able to access the article regardless
> 	     of whether they are subscribed. This feature is made
> 	     available as a service to LWN subscribers, and in the hope
> 	     that they will use it to spread the word about their
> 	     favorite LWN articles.
> 
>> And memory_region_find should likely always increment a reference
>> if the target region has an owner. We should convert its users to
>> properly dereference the region once done with it.
> 
> Yes.  But this is what requires you to have an owner for all regions.

You don't need an owner for regions that are protect by the BQL (the
majority in the foreseeable future). For those regions, reference
counting can remain a nop, internally. But that's nothing their users
should care about.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux