Re: [Xen-devel] [PATCHv6 2/2] x86/ept: defer the invalidation until the p2m lock is released

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 22/12/15 12:23, George Dunlap wrote:
> On 18/12/15 13:50, David Vrabel wrote:
>> Holding the p2m lock while calling ept_sync_domain() is very expensive
>> since it does a on_selected_cpus() call.  IPIs on many socket machines
>> can be very slows and on_selected_cpus() is serialized.
>>
>> It is safe to defer the invalidate until the p2m lock is released
>> except for two cases:
>>
>> 1. When freeing a page table page (since partial translations may be
>>    cached).
>> 2. When reclaiming a zero page as part of PoD.
>>
>> For these cases, add p2m_tlb_flush_sync() calls which will immediately
>> perform the invalidate before the page is freed or reclaimed.
> 
> There are at least two other places in the PoD code where the "remove ->
> check -> add to cache -> unlock" pattern exist; and it looks to me like
> there are other places where races might occur (e.g.,
> p2m_paging_evict(), which does remove -> scrub -> put -> unlock;
> p2m_altp2m_propagate_change(), which does remove -> put -> unlock).

I've fixed the PoD cases.

Freeing pages back to the domheap is protected by the tlb flush
timestamp mechanism.  See patch #1 in this series which makes use of
this for invalidating the guest-physical mappings (in addition to any
linear and combined mappings that were already flushes).

> Part of me wonders whether, rather than making callers that need it
> remember to do a flush, it might be better to explicitly pass in
> P2M_FLUSH or P2M_CAN_DEFER when calling p2m_set_entry, just to make
> people think about the fact that the p2m change may not actually take
> effect until later.  Any thoughts on that?

It is more efficient to batch flushes as much as possible, so explicit
flushes just before they are needed are better.  I also think it's clear
to have an explicit flush before the operation that actually needs it.

>> +static void ept_flush_and_unlock(struct p2m_domain *p2m, bool_t unlock)
>> +{
>> +    p2m->need_flush = 0;
>> +    if ( unlock )
>> +        mm_write_unlock(&p2m->lock);
>> +    ept_sync_domain_mask(p2m, p2m->domain->domain_dirty_cpumask);
>>  }
> 
> Having a function called "flush_and_unlock", with a boolean as to
> whether to unlock or not, just seems a bit wonky.
> 
> Wouldn't it make more sense to have the hook just named "flush_sync()",
> and move the unlocking out in the generic p2m code (where you already
> have the check for need_flush)?

The ept_sync_domain_mask() call must be outside the lock when unlock is
true or you don't get the performance benefits from this patch.

David

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