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

[Sean Christopherson <sean.j.christopherson@xxxxxxxxx>]

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@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 | 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
>