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

[Paolo Bonzini <pbonzini@redhat.com>]

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

We can hack around it by putting an is_ram field in FlatRange and
MemoryRegionSection, but it is not a solution.  Here is how giving an
owner to all regions fixes it:

           VCPU thread (under BQL)              device thread
 --------------------------------------------------------------------------------------
                                                flatview_ref
                                                memory_region_find returns d->mr
                                                memory_region_ref(d->mr)
                                                  object_ref(d)

           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) /* still alive! */

                                                if (!d->mr->is_ram) {
                                                  memory_region_unref(d->mr)
                                                    object_unref(d)
                                                      free(d)
                                                  return error
                                                }

Paolo