[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <op.2ijirxjlwjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date: Fri, 02 Feb 2024 18:39:23 -0600
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: jarkko@...nel.org, dave.hansen@...ux.intel.com, tj@...nel.org,
mkoutny@...e.com, linux-kernel@...r.kernel.org, linux-sgx@...r.kernel.org,
x86@...nel.org, cgroups@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
bp@...en8.de, hpa@...or.com, sohil.mehta@...el.com, "Tim Chen"
<tim.c.chen@...ux.intel.com>
Cc: zhiquan1.li@...el.com, kristen@...ux.intel.com, seanjc@...gle.com,
zhanb@...rosoft.com, anakrish@...rosoft.com, mikko.ylinen@...ux.intel.com,
yangjie@...rosoft.com
Subject: Re: [PATCH v7 09/15] x86/sgx: Charge mem_cgroup for per-cgroup
reclamation
Hi Tim,
On Fri, 02 Feb 2024 17:45:05 -0600, Tim Chen <tim.c.chen@...ux.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
Powered by blists - more mailing lists