[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen-mapcache: Fix the bug when overlapping emulated DMA operations may cause inconsistency in guest memory mappings
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. 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |