Il 06/05/2013 14:06, Jan Kiszka ha scritto:
> 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)
This doesn't really work. Regions that are known not to disappear (most
importantly, the main RAM region) also support BQL-free operation, but
have no owner right now.
Also, memory_region_find cannot know if it's returning a valid result,
and the callee cannot check it because the region may have disappeared
already when it is returned.
But I really would be surprised if adding an owner everywhere is so
hard... let's try that first, it would solve the problem.
> - 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.
Paolo
