Received-SPF: pass (google.com: domain of linux-kernel+bounces-50792-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::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, "Tim Chen"
 <tim.c.chen@linux.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
Subject: Re: [PATCH v7 09/15] x86/sgx: Charge mem_cgroup for per-cgroup
 reclamation
References: <20240122172048.11953-1-haitao.huang@linux.intel.com>
 <20240122172048.11953-10-haitao.huang@linux.intel.com>
 <ca25d3462944dcea553665d7c85903cdd6a846bc.camel@linux.intel.com>
Date: Fri, 02 Feb 2024 18:39: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.2ijirxjlwjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <ca25d3462944dcea553665d7c85903cdd6a846bc.camel@linux.intel.com>
User-Agent: Opera Mail/1.0 (Win32)

Hi Tim,
On Fri, 02 Feb 2024 17:45:05 -0600, Tim Chen <tim.c.chen@linux.intel.com>  
wrote:

> On Mon, 2024-01-22 at 09:20 -0800, Haitao Huang wrote:
>>
>> @@ -1047,29 +1037,38 @@ static struct mem_cgroup  
>> *sgx_encl_get_mem_cgroup(struct sgx_encl *encl)
>>   * @encl:	an enclave pointer
>>   * @page_index:	enclave page index
>>   * @backing:	data for accessing backing storage for the page
>> + * @indirect:	in ksgxd or EPC cgroup work queue context
>> + *
>> + * Create a backing page for loading data back into an EPC page with  
>> ELDU. This function takes
>> + * a reference on a new backing page which must be dropped with a  
>> corresponding call to
>> + * sgx_encl_put_backing().
>>   *
>> - * When called from ksgxd, sets the active memcg from one of the
>> - * mms in the enclave's mm_list prior to any backing page allocation,
>> - * in order to ensure that shmem page allocations are charged to the
>> - * enclave.  Create a backing page for loading data back into an EPC  
>> page with
>> - * ELDU.  This function takes a reference on a new backing page which
>> - * must be dropped with a corresponding call to sgx_encl_put_backing().
>> + * When @indirect is true, sets the active memcg from one of the mms  
>> in the enclave's mm_list
>> + * prior to any backing page allocation, in order to ensure that shmem  
>> page allocations are
>> + * charged to the enclave.
>>   *
>>   * Return:
>>   *   0 on success,
>>   *   -errno otherwise.
>>   */
>>  int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long  
>> page_index,
>> -			   struct sgx_backing *backing)
>> +			   struct sgx_backing *backing, bool indirect)
>>  {
>> -	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;
>>
>> +	if (indirect) {
>> +		encl_memcg = sgx_encl_get_mem_cgroup(encl);
>> +		memcg = set_active_memcg(encl_memcg);
>> +	}
>> +
>>  	ret = __sgx_encl_get_backing(encl, page_index, backing);
>>
>> -	set_active_memcg(memcg);
>> -	mem_cgroup_put(encl_memcg);
>> +	if (indirect) {
>> +		set_active_memcg(memcg);
>> +		mem_cgroup_put(encl_memcg);
>> +	}
>>
>
>
> You can reduce the number of if statements to make the logic
> simpler.  Something like
>
> 	if (!indirect)
> 		return __sgx_encl_get_backing(encl, page_index, backing);
>
> 	encl_memcg = sgx_encl_get_mem_cgroup(encl);
> 	memcg = set_active_memcg(encl_memcg);
> 	ret = __sgx_encl_get_backing(encl, page_index, backing);
> 	set_active_memcg(memcg);
> 	mem_cgroup_put(encl_memcg);
>
>>  	return ret;
>
> Tim
>

Yes, will do.
Thanks
Haitao