diff mbox series

[v3,12/17] x86/sgx: Open code sgx_reclaimer_get() and sgx_reclaimer_put()

Message ID 20190916101803.30726-13-jarkko.sakkinen@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Fixes and updates for v23 | expand

Commit Message

Jarkko Sakkinen Sept. 16, 2019, 10:17 a.m. UTC 
Open code sgx_reclaimer_get() and sgx_reclaimer_put(). They are legacy
from the callback interface. Wrapping a function call inside a function
does not make sense.

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 | 47 ++++++++++++-------------------
 1 file changed, 18 insertions(+), 29 deletions(-)

Comments

Sean Christopherson Sept. 17, 2019, 11:07 p.m. UTC | #1 
On Mon, Sep 16, 2019 at 01:17:58PM +0300, Jarkko Sakkinen wrote:
> Open code sgx_reclaimer_get() and sgx_reclaimer_put(). They are legacy
> from the callback interface. Wrapping a function call inside a function
> does not make sense.

I actually like the functions, IMO they make the loops more readable.
They should be static, and probably static inline, but I doubt either
annotation affects the compiler output.

> 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 | 47 ++++++++++++-------------------
>  1 file changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
> index c2a85db68307..5a4d44dd02d7 100644
> --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> @@ -121,22 +121,6 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
>  	spin_unlock(&sgx_active_page_list_lock);
>  }
>  
> -bool sgx_reclaimer_get(struct sgx_epc_page *epc_page)
> -{
> -	struct sgx_encl_page *encl_page = epc_page->owner;
> -	struct sgx_encl *encl = encl_page->encl;
> -
> -	return kref_get_unless_zero(&encl->refcount) != 0;
> -}
> -
> -void sgx_reclaimer_put(struct sgx_epc_page *epc_page)
> -{
> -	struct sgx_encl_page *encl_page = epc_page->owner;
> -	struct sgx_encl *encl = encl_page->encl;
> -
> -	kref_put(&encl->refcount, sgx_encl_release);
> -}
> -
>  static bool sgx_reclaimer_evict(struct sgx_epc_page *epc_page)
>  {
>  	struct sgx_encl_page *page = epc_page->owner;
> @@ -392,6 +376,7 @@ void sgx_reclaim_pages(void)
>  {
>  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN + 1];
>  	struct sgx_epc_section *section;
> +	struct sgx_encl_page *encl_page;
>  	struct sgx_epc_page *epc_page;
>  	int cnt = 0;
>  	int i;
> @@ -404,8 +389,9 @@ void sgx_reclaim_pages(void)
>  		epc_page = list_first_entry(&sgx_active_page_list,
>  					    struct sgx_epc_page, list);
>  		list_del_init(&epc_page->list);
> +		encl_page = epc_page->owner;
>  
> -		if (sgx_reclaimer_get(epc_page))
> +		if (kref_get_unless_zero(&encl_page->encl->refcount) != 0)
>  			chunk[cnt++] = epc_page;
>  		else
>  			/* The owner is freeing the page. No need to add the
> @@ -417,10 +403,12 @@ void sgx_reclaim_pages(void)
>  
>  	for (i = 0; i < cnt; i++) {
>  		epc_page = chunk[i];
> +		encl_page = epc_page->owner;
> +
>  		if (sgx_reclaimer_evict(epc_page))
>  			continue;
>  
> -		sgx_reclaimer_put(epc_page);
> +		kref_put(&encl_page->encl->refcount, sgx_encl_release);
>  
>  		spin_lock(&sgx_active_page_list_lock);
>  		list_add_tail(&epc_page->list, &sgx_active_page_list);
> @@ -437,19 +425,20 @@ void sgx_reclaim_pages(void)
>  
>  	for (i = 0; i < cnt; i++) {
>  		epc_page = chunk[i];
> -		if (epc_page) {
> -			sgx_reclaimer_write(epc_page);
> -			sgx_reclaimer_put(epc_page);
> -			epc_page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> +		if (!epc_page)
> +			continue;
>  
> -			section = sgx_epc_section(epc_page);
>  
> -			spin_lock(&section->lock);
> -			list_add_tail(&epc_page->list,
> -				      &section->page_list);
> -			section->free_cnt++;
> -			spin_unlock(&section->lock);
> -		}
> +		encl_page = epc_page->owner;
> +		sgx_reclaimer_write(epc_page);
> +		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> +		epc_page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> +
> +		section = sgx_epc_section(epc_page);
> +		spin_lock(&section->lock);
> +		list_add_tail(&epc_page->list, &section->page_list);
> +		section->free_cnt++;
> +		spin_unlock(&section->lock);
>  	}
>  }
>  
> -- 
> 2.20.1
>
Jarkko Sakkinen Sept. 18, 2019, 4:12 a.m. UTC | #2 
On Tue, Sep 17, 2019 at 04:07:40PM -0700, Sean Christopherson wrote:
> On Mon, Sep 16, 2019 at 01:17:58PM +0300, Jarkko Sakkinen wrote:
> > Open code sgx_reclaimer_get() and sgx_reclaimer_put(). They are legacy
> > from the callback interface. Wrapping a function call inside a function
> > does not make sense.
> 
> I actually like the functions, IMO they make the loops more readable.
> They should be static, and probably static inline, but I doubt either
> annotation affects the compiler output.

The way I see it they only add to need to cross reference when you read
the code. Wrapping a single function call makes only sense when you
actually have a semantical need for it.

/Jarkko
Jarkko Sakkinen Sept. 20, 2019, 1:38 p.m. UTC | #3 
On Wed, Sep 18, 2019 at 07:12:25AM +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 17, 2019 at 04:07:40PM -0700, Sean Christopherson wrote:
> > On Mon, Sep 16, 2019 at 01:17:58PM +0300, Jarkko Sakkinen wrote:
> > > Open code sgx_reclaimer_get() and sgx_reclaimer_put(). They are legacy
> > > from the callback interface. Wrapping a function call inside a function
> > > does not make sense.
> > 
> > I actually like the functions, IMO they make the loops more readable.
> > They should be static, and probably static inline, but I doubt either
> > annotation affects the compiler output.
> 
> The way I see it they only add to need to cross reference when you read
> the code. Wrapping a single function call makes only sense when you
> actually have a semantical need for it.

It also incosistent to have the wrappers as in other call sites kref_put()
is used "raw".

If we have a wrapper, it should be used consistently in every possible
call site.

/Jarkko
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
index c2a85db68307..5a4d44dd02d7 100644
--- a/arch/x86/kernel/cpu/sgx/reclaim.c
+++ b/arch/x86/kernel/cpu/sgx/reclaim.c
@@ -121,22 +121,6 @@  void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
 	spin_unlock(&sgx_active_page_list_lock);
 }
 
-bool sgx_reclaimer_get(struct sgx_epc_page *epc_page)
-{
-	struct sgx_encl_page *encl_page = epc_page->owner;
-	struct sgx_encl *encl = encl_page->encl;
-
-	return kref_get_unless_zero(&encl->refcount) != 0;
-}
-
-void sgx_reclaimer_put(struct sgx_epc_page *epc_page)
-{
-	struct sgx_encl_page *encl_page = epc_page->owner;
-	struct sgx_encl *encl = encl_page->encl;
-
-	kref_put(&encl->refcount, sgx_encl_release);
-}
-
 static bool sgx_reclaimer_evict(struct sgx_epc_page *epc_page)
 {
 	struct sgx_encl_page *page = epc_page->owner;
@@ -392,6 +376,7 @@  void sgx_reclaim_pages(void)
 {
 	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN + 1];
 	struct sgx_epc_section *section;
+	struct sgx_encl_page *encl_page;
 	struct sgx_epc_page *epc_page;
 	int cnt = 0;
 	int i;
@@ -404,8 +389,9 @@  void sgx_reclaim_pages(void)
 		epc_page = list_first_entry(&sgx_active_page_list,
 					    struct sgx_epc_page, list);
 		list_del_init(&epc_page->list);
+		encl_page = epc_page->owner;
 
-		if (sgx_reclaimer_get(epc_page))
+		if (kref_get_unless_zero(&encl_page->encl->refcount) != 0)
 			chunk[cnt++] = epc_page;
 		else
 			/* The owner is freeing the page. No need to add the
@@ -417,10 +403,12 @@  void sgx_reclaim_pages(void)
 
 	for (i = 0; i < cnt; i++) {
 		epc_page = chunk[i];
+		encl_page = epc_page->owner;
+
 		if (sgx_reclaimer_evict(epc_page))
 			continue;
 
-		sgx_reclaimer_put(epc_page);
+		kref_put(&encl_page->encl->refcount, sgx_encl_release);
 
 		spin_lock(&sgx_active_page_list_lock);
 		list_add_tail(&epc_page->list, &sgx_active_page_list);
@@ -437,19 +425,20 @@  void sgx_reclaim_pages(void)
 
 	for (i = 0; i < cnt; i++) {
 		epc_page = chunk[i];
-		if (epc_page) {
-			sgx_reclaimer_write(epc_page);
-			sgx_reclaimer_put(epc_page);
-			epc_page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
+		if (!epc_page)
+			continue;
 
-			section = sgx_epc_section(epc_page);
 
-			spin_lock(&section->lock);
-			list_add_tail(&epc_page->list,
-				      &section->page_list);
-			section->free_cnt++;
-			spin_unlock(&section->lock);
-		}
+		encl_page = epc_page->owner;
+		sgx_reclaimer_write(epc_page);
+		kref_put(&encl_page->encl->refcount, sgx_encl_release);
+		epc_page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
+
+		section = sgx_epc_section(epc_page);
+		spin_lock(&section->lock);
+		list_add_tail(&epc_page->list, &section->page_list);
+		section->free_cnt++;
+		spin_unlock(&section->lock);
 	}
 }