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

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

On 2013-05-06 13:47, Paolo Bonzini wrote:
> Il 06/05/2013 13:39, Jan Kiszka ha scritto:
>> On 2013-05-06 13:28, Paolo Bonzini wrote:
>>> Il 06/05/2013 13:11, Jan Kiszka ha scritto:
>>>> On 2013-05-06 12:58, Paolo Bonzini wrote:
>>>>> Il 06/05/2013 12:56, Jan Kiszka ha scritto:
>>>>>>> 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.
>>>>>
>>>>> That's true of address_space_rw/map, but I don't think it holds for
>>>>> memory_region_find.
>>>>
>>>> It has to, or it would be broken: Either it is called on a region that
>>>> supports reference counting
>>>
>>> You cannot know that in advance, can you?  The address is decided by the
>>> guest.
>>
>> Need to help me again to get the context: In which case is this a
>> hot-path that we want to keep BQL-free? Current users of
>> memory_region_find appear to be all relatively slow paths, thus are fine
>> with staying under BQL.
> 
> virtio-blk-dataplane is basically redoing memory_region_find with a
> separate data structure, exactly so that it can run outside the BQL
> before we get BQL-free MMIO dispatch.
> 
> I can try to post patches later today that actually use
> memory_region_find instead.

We could define its semantics as follows: return a reference to the
corresponding memory region, provide this is safe. A reference is safe when
 - the region supports BQL-free operation (thus provides an owner to
   apply reference counting on)
 - the caller holds the BQL (check via qemu_mutex_iothread_is_locked()
   - to be implemented)

The latter implies that the BQL is not dropped before returning the
reference, but that's nothing memory_region_find can enforce.

Jan

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