Re: [PATCH] xen-mapcache: Fix the bug when overlapping emulated DMA operations may cause inconsistency in guest memory mappings

[Alexey G <x1917x@gmail.com>]

Stefano,

On Tue, 18 Jul 2017 15:17:25 -0700 (PDT)
Stefano Stabellini <sstabellini@kernel.org> wrote:

> > The patch modifies the behavior in which MapCacheEntry's are added to
> > the list, avoiding duplicates.  
> 
> I take that the idea is to always go through the whole list to check for
> duplicate locked entries, right?
 
That's a short list.

In fact, it's not easy to generate multiple linked entries in this list --
normally, entries will be added, used and then removed immediately by
xen_invalidate_map_cache(). Specific conditions are required to make the
list grow -- like simultaneous DMA operations (of different cache_size)
originating the same address_index or presence of the Option ROM mapping in
the area. 

So normally we deal with just 1-2 entries in the list. Even three entries
are likely to be generated only intentionally and with a bit of luck as it
depends on host's performance/workload a lot. Also, a good cache_size
diversity is required to produce entries in the list but we actually
limited to only few multiplies of MCACHE_BUCKET_SIZE due to the maximum DMA
size limitations of emulated devices.

> Yes, I think this would work, but we should make sure to scan the whole
> list only when lock ==  true. Something like the following:
> 
> -    while (entry && entry->lock && entry->vaddr_base &&
> +    while (entry && (lock || entry->lock) && entry->vaddr_base &&
>              (entry->paddr_index != address_index || entry->size !=
> cache_size || !test_bits(address_offset >> XC_PAGE_SHIFT,
>                   test_bit_size >> XC_PAGE_SHIFT,
>                   entry->valid_mapping))) {
> +        if (!free_entry && !entry->lock) {
> +            free_entry = entry;
> +            free_pentry = pentry;
> +        }
>          pentry = entry;
>          entry = entry->next;
>      }
> 
> Would this work?

This would, but the question is if there will be a benefit. In this way we
avoiding to traverse the rest of the list (few entries, if any) if we asked
for some lock=0 mapping and found such entry before the reuseable lock=n
entry. We win few iterations of quick checks, but on other hand risking to
have to execute xen_remap_bucket() for this entry (with lot of fairly slow
stuff). If there was a reusable entry later in the list -- using it instead
of (possibly) remapping an entry will be faster... so it's pros and cons
here.

We can use locked entry for "non-locked" request as it is protected by the
same (kinda suspicious) rcu_read_lock/rcu_read_unlock mechanism above. The
big question here is whether rcu_read_(un)lock is enough at all
for underneath xen-mapcache usage -- seems like the xen-mapcache-related
code in QEMU expects RCU read lock to work like a plain critical section...
although this needs to be checked.

One possible minor optimization for xen-mapcache would be to reuse larger
mappings for mappings of lesser cache_size. Right now existing code does
checks in the "entry->size == cache_size" manner, while we can use
"entry->size >= cache_size" here. However, we may end up with resident
MapCacheEntries being mapped to a bigger mapping sizes than necessary and
thus might need to add remapping back to the normal size in
xen_invalidate_map_cache_entry_unlocked() when there are no other mappings.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel