[v3,13/17] x86/sgx: Introduce sgx_can_reclaim()
diff mbox series

Message ID 20190916101803.30726-14-jarkko.sakkinen@linux.intel.com
State New
Headers show
Series
  • Fixes and updates for v23
Related show

Commit Message

Jarkko Sakkinen Sept. 16, 2019, 10:17 a.m. UTC
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@intel.com>
Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
Cc: Serge Ayoun <serge.ayoun@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

Comments

Sean Christopherson Sept. 17, 2019, 11:25 p.m. UTC | #1
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@intel.com>
> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Cc: Serge Ayoun <serge.ayoun@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  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
>
Sean Christopherson Sept. 25, 2019, 6:28 p.m. UTC | #2
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@intel.com>
> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Cc: Serge Ayoun <serge.ayoun@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  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)

Belated thought.  The param can be 'struct sgx_encl_page *encl_page'.  The
caller already has the encl_page, and the function only uses the epc_page
to retrieve the encl_page.
Jarkko Sakkinen Sept. 27, 2019, 3:33 p.m. UTC | #3
On Wed, Sep 25, 2019 at 11:28:27AM -0700, Sean Christopherson wrote:
> 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@intel.com>
> > Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> > Cc: Serge Ayoun <serge.ayoun@intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  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)
> 
> Belated thought.  The param can be 'struct sgx_encl_page *encl_page'.  The
> caller already has the encl_page, and the function only uses the epc_page
> to retrieve the encl_page.

I used sgx_reclaimer_age() for the function when I merged.
Agree that the function name was not right because it is not
a pure query but also changes state.

/Jarkko

Patch
diff mbox series

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;
+			mutex_unlock(&encl_page->encl->lock);
 			continue;
+		}
 
 		kref_put(&encl_page->encl->refcount, sgx_encl_release);