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

[Alexey Kardashevskiy]

On 11/09/17 19:37, Paolo Bonzini wrote:
> 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).

The helpers I converted from AddressSpace to AddressSpaceDispatch do use
dispatch structure only and do not use FlatView so it seemed logical.

btw this address_space in MemoryRegionSection - it does not seem to make
much sense in the PhysPageMap::sections array, it only makes sense when
MemoryRegionSection uses as a temporary object when calling listeners. Will
it make sense if we enforce MemoryRegionSection::address_space to be NULL
in the array and not NULL when used temporary?


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

Ok, since this seems better.


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

I am learning as we speak :)


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


Ah, makes sense now, thanks.

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


-- 
Alexey