On Mon, Sep 16, 2019 at 01:17:59PM +0300, Jarkko Sakkinen wrote:
> Make sgx_reclaim_evict() idempotennt and rename it to sgx_can_reclaim() and
> set SGX_ENCL_PAGE_RECLAIMED in the call site.
>
> Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Cc: Shay Katz-zamir <shay.katz-zamir@xxxxxxxxx>
> Cc: Serge Ayoun <serge.ayoun@xxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> ---
> arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++--------------
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
> index 5a4d44dd02d7..cc3155b61513 100644
> --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> @@ -121,7 +121,19 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
> spin_unlock(&sgx_active_page_list_lock);
> }
>
> -static bool sgx_reclaimer_evict(struct sgx_epc_page *epc_page)
> +/**
> + * sgx_can_reclaim() - Filter out the pages that should not be reclaimed
> + * @epc_page: a candidate EPC page
> + *
> + * Do not reclaim this page if it has been recently accessed by any mm_struct
> + * *and* if the enclave is still alive. No need to take the enclave's lock,
> + * worst case scenario reclaiming pages from a dead enclave is delayed slightly.
> + * A live enclave with a recently accessed page is more common and avoiding lock
> + * contention in that case is a boon to performance.
> + *
> + * Return: true if the page should be reclaimed
> + */
> +static bool sgx_can_reclaim(struct sgx_epc_page *epc_page)
> {
> struct sgx_encl_page *page = epc_page->owner;
> struct sgx_encl *encl = page->encl;
> @@ -147,21 +159,9 @@ static bool sgx_reclaimer_evict(struct sgx_epc_page *epc_page)
>
> srcu_read_unlock(&encl->srcu, idx);
>
> - /*
> - * Do not reclaim this page if it has been recently accessed by any
> - * mm_struct *and* if the enclave is still alive. No need to take
> - * the enclave's lock, worst case scenario reclaiming pages from a
> - * dead enclave is delayed slightly. A live enclave with a recently
> - * accessed page is more common and avoiding lock contention in that
> - * case is a boon to performance.
> - */
> if (!ret && !(atomic_read(&encl->flags) & SGX_ENCL_DEAD))
> return false;
>
> - mutex_lock(&encl->lock);
> - page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> - mutex_unlock(&encl->lock);
> -
> return true;
> }
>
> @@ -405,8 +405,12 @@ void sgx_reclaim_pages(void)
> epc_page = chunk[i];
> encl_page = epc_page->owner;
>
> - if (sgx_reclaimer_evict(epc_page))
> + if (sgx_can_reclaim(epc_page)) {
> + mutex_lock(&encl_page->encl->lock);
> + encl_page->desc |= SGX_ENCL_PAGE_RECLAIMED;
If we move setting SGX_ENCL_PAGE_RECLAIMED here, then we should also
move the clearing (currently in sgx_encl_ewb()) into the loop that
calls sgx_reclaimer_write(). That would also fix the issue of
SGX_ENCL_PAGE_RECLAIMED being left set when the enclave is dead.
> + mutex_unlock(&encl_page->encl->lock);
> continue;
> + }
>
> kref_put(&encl_page->encl->refcount, sgx_encl_release);
>
> --
> 2.20.1
>
