Re: [PATCH v3 14/17] x86/sgx: Replace section->free_cnt with a global sgx_nr_free_pages

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

On Mon, Sep 16, 2019 at 01:18:00PM +0300, Jarkko Sakkinen wrote:
> Replace section specific counters with a single gloal counter for free
> pages. In effect, remove sgx_calc_free_cnt().
> 
> 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/main.c    | 11 ++++++-----
>  arch/x86/kernel/cpu/sgx/reclaim.c | 19 ++-----------------
>  arch/x86/kernel/cpu/sgx/sgx.h     |  3 +--
>  3 files changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 4c03e5f33414..f37d28023b97 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -14,19 +14,20 @@
>  
>  struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
>  int sgx_nr_epc_sections;
> +unsigned long sgx_nr_free_pages;
>  
>  static struct sgx_epc_page *sgx_section_try_take_page(
>  	struct sgx_epc_section *section)
>  {
>  	struct sgx_epc_page *page;
>  
> -	if (!section->free_cnt)
> +	if (list_empty(&section->page_list))
>  		return NULL;
>  
>  	page = list_first_entry(&section->page_list, struct sgx_epc_page,
>  				list);
>  	list_del_init(&page->list);
> -	section->free_cnt--;
> +	sgx_nr_free_pages--;
>  	return page;
>  }
>  
> @@ -90,7 +91,7 @@ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim)
>  		schedule();
>  	}
>  
> -	if (sgx_calc_free_cnt() < SGX_NR_LOW_PAGES)
> +	if (sgx_nr_free_pages < SGX_NR_LOW_PAGES)
>  		wake_up(&ksgxswapd_waitq);
>  
>  	return entry;
> @@ -136,7 +137,7 @@ int __sgx_free_page(struct sgx_epc_page *page)
>  
>  	spin_lock(&section->lock);
>  	list_add_tail(&page->list, &section->page_list);
> -	section->free_cnt++;
> +	sgx_nr_free_pages++;

This isn't safe when there are multiple EPC sections as each section has
its own spinlock.

>  	spin_unlock(&section->lock);
>  
>  	return 0;
> @@ -202,7 +203,7 @@ static __init int sgx_init_epc_section(u64 addr, u64 size, unsigned long index,
>  			goto out;
>  		page->desc = (addr + (i << PAGE_SHIFT)) | index;
>  		list_add_tail(&page->list, &section->unsanitized_page_list);
> -		section->free_cnt++;
> +		sgx_nr_free_pages++;
>  	}
>  
>  	return 0;
> diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
> index cc3155b61513..2e04a923d8dc 100644
> --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> @@ -64,7 +64,7 @@ static void sgx_sanitize_section(struct sgx_epc_section *section)
>  
>  static inline bool sgx_should_reclaim(void)
>  {
> -	return sgx_calc_free_cnt() < SGX_NR_HIGH_PAGES &&
> +	return sgx_nr_free_pages < SGX_NR_HIGH_PAGES &&
>  	       !list_empty(&sgx_active_page_list);
>  }
>  
> @@ -432,7 +432,6 @@ void sgx_reclaim_pages(void)
>  		if (!epc_page)
>  			continue;
>  
> -
>  		encl_page = epc_page->owner;
>  		sgx_reclaimer_write(epc_page);
>  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> @@ -441,21 +440,7 @@ void sgx_reclaim_pages(void)
>  		section = sgx_epc_section(epc_page);
>  		spin_lock(&section->lock);
>  		list_add_tail(&epc_page->list, &section->page_list);
> -		section->free_cnt++;
> +		sgx_nr_free_pages++;
>  		spin_unlock(&section->lock);
>  	}
>  }
> -
> -unsigned long sgx_calc_free_cnt(void)
> -{
> -	struct sgx_epc_section *section;
> -	unsigned long free_cnt = 0;
> -	int i;
> -
> -	for (i = 0; i < sgx_nr_epc_sections; i++) {
> -		section = &sgx_epc_sections[i];
> -		free_cnt += section->free_cnt;
> -	}
> -
> -	return free_cnt;
> -}
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 9b08690262b5..56d0bde3f4d8 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -30,7 +30,6 @@ struct sgx_epc_section {
>  	void *va;
>  	struct list_head page_list;
>  	struct list_head unsanitized_page_list;
> -	unsigned long free_cnt;
>  	spinlock_t lock;
>  };
>  
> @@ -72,6 +71,7 @@ static inline void *sgx_epc_addr(struct sgx_epc_page *page)
>  #define SGX_NR_HIGH_PAGES	64
>  
>  extern int sgx_nr_epc_sections;
> +extern unsigned long sgx_nr_free_pages;
>  extern struct task_struct *ksgxswapd_tsk;
>  extern struct wait_queue_head(ksgxswapd_waitq);
>  extern struct list_head sgx_active_page_list;
> @@ -79,7 +79,6 @@ extern spinlock_t sgx_active_page_list_lock;
>  
>  int sgx_page_reclaimer_init(void);
>  void sgx_mark_page_reclaimable(struct sgx_epc_page *page);
> -unsigned long sgx_calc_free_cnt(void);
>  void sgx_reclaim_pages(void);
>  
>  struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim);
> -- 
> 2.20.1
>