Received-SPF: pass (google.com: domain of linux-kernel+bounces-18449-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes
To: jarkko@kernel.org, dave.hansen@linux.intel.com, tj@kernel.org,
 mkoutny@suse.com, linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org,
 x86@kernel.org, cgroups@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
 bp@alien8.de, hpa@zytor.com, sohil.mehta@intel.com, "Dave Hansen"
 <dave.hansen@intel.com>
Cc: zhiquan1.li@intel.com, kristen@linux.intel.com, seanjc@google.com,
 zhanb@microsoft.com, anakrish@microsoft.com, mikko.ylinen@linux.intel.com,
 yangjie@microsoft.com, "Sean Christopherson" <sean.j.christopherson@intel.com>
Subject: Re: [PATCH v6 07/12] x86/sgx: Introduce EPC page states
References: <20231030182013.40086-1-haitao.huang@linux.intel.com>
 <20231030182013.40086-8-haitao.huang@linux.intel.com>
 <0f3ae753-031f-4f33-b577-8c863b4621a1@intel.com>
Date: Fri, 05 Jan 2024 19:45:23 -0600
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.2g3q5xxfwjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <0f3ae753-031f-4f33-b577-8c863b4621a1@intel.com>
User-Agent: Opera Mail/1.0 (Win32)

On Fri, 05 Jan 2024 11:57:03 -0600, Dave Hansen <dave.hansen@intel.com>  
wrote:

> On 10/30/23 11:20, Haitao Huang wrote:
>> @@ -527,16 +530,13 @@ void sgx_mark_page_reclaimable(struct  
>> sgx_epc_page *page)
>>  int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
>>  {
>>  	spin_lock(&sgx_global_lru.lock);
>> -	if (page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) {
>> -		/* The page is being reclaimed. */
>> -		if (list_empty(&page->list)) {
>> -			spin_unlock(&sgx_global_lru.lock);
>> -			return -EBUSY;
>> -		}
>> -
>> -		list_del(&page->list);
>> -		page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>> +	if (sgx_epc_page_reclaim_in_progress(page->flags)) {
>> +		spin_unlock(&sgx_global_lru.lock);
>> +		return -EBUSY;
>>  	}
>> +
>> +	list_del(&page->list);
>> +	sgx_epc_page_reset_state(page);
>
> I want to know how much if this series is basically line-for-line
> abstraction shifting like:
>
> -	page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> +	sgx_epc_page_reset_state(page);
>
> versus actually adding complexity.  That way, I might be able to offer
> some advice on where this can be pared down.  That's really hard to do
> with the current series.
>
> Please don't just "introduce new page states".  This should have first
> abstracted out the sgx_epc_page_reclaim_in_progress() operation, using
> the list_empty() check as the implementation.
>
> Then, in a separate patch, introduce the concept of the "reclaim in
> progress" flag and finally flip the implementation over.
>
> Ditto for the sgx_epc_page_reset_state() abstraction.  It should have
> been introduced separately as a concept and then have the implementation
> changed.
>
> On in to patch 10 (which is much too big) which introduces the
> sgx_lru_list() abstraction.
>

Sure. I'll try to refactor according to this plan.
Thanks
Haitao