Received-SPF: pass (google.com: domain of linux-kernel+bounces-62906-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes
To: 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, tim.c.chen@linux.intel.com, "Jarkko
 Sakkinen" <jarkko@kernel.org>
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, chrisyan@microsoft.com
Subject: Re: [PATCH v9 09/15] x86/sgx: Charge mem_cgroup for per-cgroup
 reclamation
References: <20240205210638.157741-1-haitao.huang@linux.intel.com>
 <20240205210638.157741-10-haitao.huang@linux.intel.com>
 <CZ3CXPOLZEI8.C6AJ9W0W07A9@kernel.org>
Date: Mon, 12 Feb 2024 21:21:47 -0600
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.2i18ylk5wjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <CZ3CXPOLZEI8.C6AJ9W0W07A9@kernel.org>
User-Agent: Opera Mail/1.0 (Win32)

On Mon, 12 Feb 2024 13:46:06 -0600, Jarkko Sakkinen <jarkko@kernel.org>  
wrote:

> On Mon Feb 5, 2024 at 11:06 PM EET, Haitao Huang wrote:
>> Enclave Page Cache(EPC) memory can be swapped out to regular system
>
> "Enclave Page Cache (EPC)"
>                    ~
>
Will fix.

[...]
>>  int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long  
>> page_index,
>> -			   struct sgx_backing *backing)
>> +			   struct sgx_backing *backing, bool indirect)
>
> Boolean parameters should be avoided when possible because they confuse
> in the call sites.
>
>>  {
>> -	struct mem_cgroup *encl_memcg = sgx_encl_get_mem_cgroup(encl);
>> -	struct mem_cgroup *memcg = set_active_memcg(encl_memcg);
>> +	struct mem_cgroup *encl_memcg;
>> +	struct mem_cgroup *memcg;
>>  	int ret;
>>
>> -	ret = __sgx_encl_get_backing(encl, page_index, backing);
>> +	if (!indirect)
>> +		return  __sgx_encl_get_backing(encl, page_index, backing);
>
> If a call is either in heead or tail of the code block, then
> obviously better option is to make __sgx_encl_get_backing()
> as non-static sgx_encl_get_backing() and call it in those
> call sites that would call this with "false".
>
> I.e. you need a new patch where this preparation is done.
>

This would actually require more intrusive changes to the call stack for  
global and cgroup reclaim:

{sgx_epc_cgroup_reclaim_pages(),sgx_reclaim_pages_global()}->sgx_reclaim_pages()[->sgx_reclaimer_write()]->sgx_encl_alloc_backing()

We need make two versions of each of those functions.
It'd be especially complicated in refactoring sgx_reclaim_pages() for two  
versions.

I now double checked the history of current_is_ksgxd()[1], it seemed the  
intent was to replace "current->mm == NULL" criteria so it is more obvious  
we are running in ksgxd.

@Dave, @Kristen,

Can we restore the original criteria like below so it works for the cgroup  
work-queues?

bool current_is_ksgxd(void)
{
	return current == ksgxd_tsk;
}

--->

bool current_is_kthread(void)
{
	return current->mm == NULL;	
}

I'm not experienced in this area and not sure how reliable it is to use  
current->mm == NULL for kthread and work-queues. But it would eliminate  
the need for the boolean parameter.

Would appreciate the input.

Haitao

[1]https://lore.kernel.org/linux-sgx/9c269c70-35fe-a1a4-34c9-b1e62ab3bb3b@intel.com/