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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 19 Jul 2017, Alexey G wrote:
> Stefano,
> 
> On Tue, 18 Jul 2017 15:17:25 -0700 (PDT)
> Stefano Stabellini <sstabellini@xxxxxxxxxx> 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.

My expectation is that unlocked mappings are much more frequent than
locked mappings. Also, I expect that only very rarely we'll be able to
reuse locked mappings. Over the course of a VM lifetime, it seems to me
that walking the list every time would cost more than it would benefit.

These are only "expectations", I would love to see numbers. Numbers make
for better decisions :-)  Would you be up for gathering some of these
numbers? Such as how many times you get to reuse locked mappings and how
many times we walk items on the list fruitlessly?

Otherwise, would you be up for just testing the modified version of the
patch I sent to verify that solves the bug?



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

Yes, I thought about it, that would be a good improvement to have.

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