[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jan Kiszka
Subject: Re: [Qemu-devel] [PATCH v7 0/7] push mmio dispatch out of big lock
Date: Mon, 06 May 2013 16:05:56 +0200
User-agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv: Gecko/20080226 SUSE/ Thunderbird/ Mnenhy/

On 2013-05-06 15:09, Paolo Bonzini wrote:
> 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.

Those few are much easier to convert than a full set of PCI and other
hot-pluggable device, that's my point.

> 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.

Again, we hold the address space lock while checking the conditions. If
a region does not supports BQL-free mode and BQL is not held, we have an
error and return NULL (or bail out with a runtime error).

> But I really would be surprised if adding an owner everywhere is so
> hard...  let's try that first, it would solve the problem.

If we can avoid it, that would only help the process. If we can't, ok.


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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]