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

From: David Vrabel <david.vrabel@citrix.com>
To: George Dunlap <george.dunlap@citrix.com>,
	David Vrabel <david.vrabel@citrix.com>,
	xen-devel@lists.xenproject.org
Cc: Kevin Tian <kevin.tian@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>, Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCHv6 2/2] x86/ept: defer the invalidation until the p2m lock is released
Date: Mon, 1 Feb 2016 15:57:07 +0000	[thread overview]
Message-ID: <56AF8053.6000801@citrix.com> (raw)
In-Reply-To: <567940A4.6040900@citrix.com>

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