lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 21 Feb 2024 11:34:27 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "hpa@...or.com" <hpa@...or.com>, "tim.c.chen@...ux.intel.com"
	<tim.c.chen@...ux.intel.com>, "linux-sgx@...r.kernel.org"
	<linux-sgx@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"jarkko@...nel.org" <jarkko@...nel.org>, "cgroups@...r.kernel.org"
	<cgroups@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "mkoutny@...e.com" <mkoutny@...e.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "haitao.huang@...ux.intel.com"
	<haitao.huang@...ux.intel.com>, "Mehta, Sohil" <sohil.mehta@...el.com>,
	"tj@...nel.org" <tj@...nel.org>, "mingo@...hat.com" <mingo@...hat.com>,
	"bp@...en8.de" <bp@...en8.de>
CC: "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "anakrish@...rosoft.com"
	<anakrish@...rosoft.com>, "Zhang, Bo" <zhanb@...rosoft.com>,
	"kristen@...ux.intel.com" <kristen@...ux.intel.com>, "yangjie@...rosoft.com"
	<yangjie@...rosoft.com>, "Li, Zhiquan1" <zhiquan1.li@...el.com>,
	"chrisyan@...rosoft.com" <chrisyan@...rosoft.com>
Subject: Re: [PATCH v9 11/15] x86/sgx: Abstract check for global reclaimable
 pages

On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote:
> From: Kristen Carlson Accardi <kristen@...ux.intel.com>
> 
> To determine if any page available for reclamation at the global level,
> only checking for emptiness of the global LRU is not adequate when pages
> are tracked in multiple LRUs, one per cgroup. For this purpose, create a
> new helper, sgx_can_reclaim(), currently only checks the global LRU,
> later will check emptiness of LRUs of all cgroups when per-cgroup
> tracking is turned on. Replace all the checks of the global LRU,
> list_empty(&sgx_global_lru.reclaimable), with calls to
> sgx_can_reclaim().
> 
> Co-developed-by: Sean Christopherson <sean.j.christopherson@...el.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> Signed-off-by: Kristen Carlson Accardi <kristen@...ux.intel.com>
> Co-developed-by: Haitao Huang <haitao.huang@...ux.intel.com>
> Signed-off-by: Haitao Huang <haitao.huang@...ux.intel.com>
> ---
> v7:
> - Split this out from the big patch, #10 in V6. (Dave, Kai)
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 2279ae967707..6b0c26cac621 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -37,6 +37,11 @@ static inline struct sgx_epc_lru_list *sgx_lru_list(struct sgx_epc_page *epc_pag
>  	return &sgx_global_lru;
>  }
>  
> +static inline bool sgx_can_reclaim(void)
> +{
> +	return !list_empty(&sgx_global_lru.reclaimable);
> +}
> +
>  static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
>  
>  /* Nodes with one or more EPC sections. */
> @@ -398,7 +403,7 @@ unsigned int sgx_reclaim_pages(struct sgx_epc_lru_list *lru, unsigned int *nr_to
>  static bool sgx_should_reclaim(unsigned long watermark)
>  {
>  	return atomic_long_read(&sgx_nr_free_pages) < watermark &&
> -	       !list_empty(&sgx_global_lru.reclaimable);
> +		sgx_can_reclaim();
>  }
>  
>  static void sgx_reclaim_pages_global(bool indirect)
> @@ -601,7 +606,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
>  			break;
>  		}
>  
> -		if (list_empty(&sgx_global_lru.reclaimable)) {
> +		if (!sgx_can_reclaim()) {
>  			page = ERR_PTR(-ENOMEM);
>  			break;
>  		}

Seems a basic elemental change.  Why did you put this patch at almost end of
this series but not at an earlier place?

I think one advantage of putting elemental changes at early place is, if there's
any code change related to these (the code changes sgx_global_lru in this patch)
in any later patch, the updated one can be used.  Otherwise if you do elemental
change at later place, when you replace you have to replace all the places that
were modified in previous patches.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ