Re: [PATCH v6 09/12] x86/sgx: Restructure top-level EPC reclaim function

From: "Huang, Kai" <kai.huang@intel.com>
To: "Mehta, Sohil" <sohil.mehta@intel.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"tj@kernel.org" <tj@kernel.org>,
	"mkoutny@suse.com" <mkoutny@suse.com>,
	"haitao.huang@linux.intel.com" <haitao.huang@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"bp@alien8.de" <bp@alien8.de>
Cc: "mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"Zhang, Bo" <zhanb@microsoft.com>,
	"kristen@linux.intel.com" <kristen@linux.intel.com>,
	"anakrish@microsoft.com" <anakrish@microsoft.com>,
	"sean.j.christopherson@intel.com"
	<sean.j.christopherson@intel.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>,
	"yangjie@microsoft.com" <yangjie@microsoft.com>
Subject: Re: [PATCH v6 09/12] x86/sgx: Restructure top-level EPC reclaim function
Date: Mon, 18 Dec 2023 01:44:56 +0000	[thread overview]
Message-ID: <73ed579be8ad81835df1c309b7c69b491b7f2c8e.camel@intel.com> (raw)
In-Reply-To: <op.2f0eo8r1wjvjmi@hhuan26-mobl.amr.corp.intel.com>

> > 
> > The point is, with or w/o this patch, you can only reclaim 16 EPC pages  
> > in one
> > function call (as you have said you are going to remove  
> > SGX_NR_TO_SCAN_MAX,
> > which is a cipher to both of us).  The only difference I can see is,  
> > with this
> > patch, you can have multiple calls of "isolate" and then call the  
> > "do_reclaim"
> > once.
> > 
> > But what's the good of having the "isolate" if the "do_reclaim" can only  
> > reclaim
> > 16 pages anyway?
> > 
> > Back to my last reply, are you afraid of any LRU has less than 16 pages  
> > to
> > "isolate", therefore you need to loop LRUs of descendants to get 16?   
> > Cause I
> > really cannot think of any other reason why you are doing this.
> > 
> > 
> 
> I think I see your point. By capping pages reclaimed per cycle to 16,  
> there is not much difference even if those 16 pages are spread in separate  
> LRUs . The difference is only significant when we ever raise that cap. To  
> preserve the current behavior of ewb loops independent on number of LRUs  
> to loop through for each reclaiming cycle, regardless of the exact value  
> of the page cap, I would still think current approach in the patch is  
> reasonable choice.  What do you think?

To me I won't bother to do that.  Having less than 16 pages in one LRU is
*extremely rare* that should never happen in practice.  It's pointless to make
such code adjustment at this stage.

Let's focus on enabling functionality first.  When you have some real
performance issue that is related to this, we can come back then.

Btw, I think you need to step back even further.  IIUC the whole multiple LRU
thing isn't mandatory in this initial support.

Please (again) take a look at the comments from Dave and Michal:

https://lore.kernel.org/lkml/7a1a5125-9da2-47b6-ba0f-cf24d84df16b@intel.com/#t
https://lore.kernel.org/lkml/yz44wukoic3syy6s4fcrngagurkjhe2hzka6kvxbajdtro3fwu@zd2ilht7wcw3/