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

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

On 2013-05-06 12:27, Paolo Bonzini wrote:
> Il 06/05/2013 10:40, Jan Kiszka ha scritto:
> 
>>>
>>> 	[*]  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.
> 
> The problem is that even if I/O for a region is supposed to happen
> within the BQL, lookup can happen outside the BQL.  Lookup will use the
> region even if it is just to discard it:
> 
>            VCPU thread (under BQL)              device thread
>  --------------------------------------------------------------------------------------
>                                                 flatview_ref
>                                                 memory_region_find returns d->mr
>                                                 memory_region_ref(d->mr) /* nop */
>            qdev_free(d)
>              object_unparent(d)
>                unrealize(d)
>                  memory_region_del_subregion(d->mr)
>                    FlatView updated, d->mr not in the new view
> 
>                                                 flatview_unref
>                                                   memory_region_unref(d->mr)
>                                                     object_unref(d)
>                                                       free(d)
>                                                 if (!d->mr->is_ram) {        /* BAD! */
>                                                   memory_region_unref(d->mr) /* nop */
>                                                   return error
>                                                 }
> 
> 
> Here, the memory region is dereferenced *before* we know that it is BQL-free
> (in fact, exactly to ascertain whether it is BQL-free).

Both flatview update and lookup *plus* locking type evaluation (i.e.
memory region dereferencing) always happen under the address space lock.
See Pingfan's patch.

Jan

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