Re: [Qemu-devel] [RFC PATCH qemu 3/4] memory: Share flat views and dispatch trees between address spaces

[Paolo Bonzini]

On 11/09/2017 11:06, Alexey Kardashevskiy wrote:
> On 11/09/17 17:40, Paolo Bonzini wrote:
>> On 07/09/2017 11:20, Alexey Kardashevskiy wrote:
>>>  
>>>      /* Accessed via RCU.  */
>>>      struct FlatView *current_map;
>>>  
>>>      int ioeventfd_nb;
>>>      struct MemoryRegionIoeventfd *ioeventfds;
>>> -    struct AddressSpaceDispatch *dispatch;
>>> -    struct AddressSpaceDispatch *next_dispatch;
>>> +
>>
>> The rough idea of the patch matches my suggestion indeed.  However, I am
>> not sure why all of the changes in patch 2 are needed.
> 
> For this:
> 
>  struct MemoryRegionSection {
>      MemoryRegion *mr;
> -    AddressSpace *address_space;
> +    AddressSpaceDispatch *dispatch;
> 
> as there are many ASes attached to the same flatview/dispatch.

Ok, this makes sense.  Maybe it should be a flatview rather than an
AddressSpaceDispatch (a FlatView is essentially a list of
MemoryRegionSections; attaching the ASD to the FlatView is more or less
an implementation detail).


> And because of that, there is also:
> 
>  struct IOMMUTLBEntry {
> -    AddressSpace    *target_as;
> +    AddressSpaceDispatch *target_dispatch;
> 
> as the "section" in address_space_get_iotlb_entry() does not have
> address_space any more, even though the only user of it -
> vhost_device_iotlb_miss() - only checks if (iotlb.target_dispatch != NULL).

You could change address_space_do_translate's "as" to AddressSpace **as.
 Then, address_space_get_iotlb_entry can populate the .target_as from
the output value of the argument.

>> In addition, you could change the computation of FlatView's root to
>> resolve 2^64-sized aliases;
> 
> Here we reached the boundary of my english :)
> 
> Roots are given when AS/Flatview is created, and aliases are resolved already.
> 

Currently, you're doing

+        if (view->root == root) {
+            as->current_map = flatview_get(view);
+            break;
+        }

(and by the way the above doesn't resolve aliases; aliases are only
resolved by render_memory_region).

Instead, the FlatViews should be shared at transaction commit time.  At
the beginning of commit, the list of flat views is empty.  As you
process each AddressSpace, you resolve the root alias(*) and search for
the resulting MemoryRegion in the list of FlatViews.  If you find it,
you do flatview_ref and point as->current_map to the FlatView you just
found.  Otherwise, you do generate_memory_topology etc. as in the old code.

(*) something like

        MemoryRegion *mr = as->root;
        MemoryRegion *root_mr;
        while (mr->alias && !mr->alias_offset &&
               int128_ge(mr->size, mr->alias->size)) {
            /* The alias is included in its entirety.  Use it as
             * the "real" root, so that we can share more FlatViews.
             */
            mr = mr->alias;
        }       

        /* We found the "real" root, but maybe we can use a shared empty
         * FlatView instead?
         */
        for (root_mr = mr; root_mr; root_mr = root_mr->alias) {
            if (!root_mr->enabled) {
                /* Use a shared empty FlatView.  */
                return NULL;
            }
        }

        return mr;

Also:

> +static FlatView *flatview_get(FlatView *view)
>  {
> -    FlatView *view;
> -
>      rcu_read_lock();
> -    view = atomic_rcu_read(&as->current_map);
> +    view = atomic_rcu_read(&view);

This is "view = view" so it makes little sense, and then in the caller

+    view = flatview_get(as->current_map);

as->current_map is accessed without atomic_rcu_read.  So
address_space_get_flatview must stay.  Probably this will solve itself
when you do the rest.

Paolo

>> also set it to NULL if the AddressSpace's
>> root is disabled or the alias it resolves to is disabled (and so on
>> recursively until a non-alias is found).  This should remove the need
>> for address_space_root() and the change to pci_init_bus_master.
> 
> 
> 
>