[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <op.2id2hcvkwjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date: Tue, 30 Jan 2024 19:59:26 -0600
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: "jarkko@...nel.org" <jarkko@...nel.org>, "dave.hansen@...ux.intel.com"
<dave.hansen@...ux.intel.com>, "tj@...nel.org" <tj@...nel.org>,
"mkoutny@...e.com" <mkoutny@...e.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-sgx@...r.kernel.org"
<linux-sgx@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
"cgroups@...r.kernel.org" <cgroups@...r.kernel.org>, "tglx@...utronix.de"
<tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
<bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>, "Mehta, Sohil"
<sohil.mehta@...el.com>, "Huang, Kai" <kai.huang@...el.com>
Cc: "Li, Zhiquan1" <zhiquan1.li@...el.com>, "kristen@...ux.intel.com"
<kristen@...ux.intel.com>, "seanjc@...gle.com" <seanjc@...gle.com>, "Zhang,
Bo" <zhanb@...rosoft.com>, "anakrish@...rosoft.com" <anakrish@...rosoft.com>,
"mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
"yangjie@...rosoft.com" <yangjie@...rosoft.com>, "chrisyan@...rosoft.com"
<chrisyan@...rosoft.com>
Subject: Re: [PATCH v8 04/15] x86/sgx: Implement basic EPC misc cgroup
functionality
On Tue, 30 Jan 2024 09:22:14 -0600, Huang, Kai <kai.huang@...el.com> wrote:
>> struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) {
>> + struct sgx_epc_cgroup *epc_cg;
>> struct sgx_epc_page *page;
>> + int ret;
>> +
>> + epc_cg = sgx_get_current_epc_cg();
>> + ret = sgx_epc_cgroup_try_charge(epc_cg);
>> + if (ret) {
>> + sgx_put_epc_cg(epc_cg);
>> + return ERR_PTR(ret);
>> + }
>>
>> for ( ; ; ) {
>> page = __sgx_alloc_epc_page();
>> @@ -567,8 +578,10 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
>> *owner, bool reclaim)
>> break;
>> }
>>
>> - if (list_empty(&sgx_active_page_list))
>> - return ERR_PTR(-ENOMEM);
>> + if (list_empty(&sgx_active_page_list)) {
>> + page = ERR_PTR(-ENOMEM);
>> + break;
>> + }
>
> (Sorry for replying from Outlook because I am in travel for Chinese New
> Year.)
>
> Perhaps I am missing something but I don't understand this change.
>
> An empty sgx_active_page_list means you cannot reclaim any page from it,
> so why need to break?
>
This is to avoid any escape for sgx_put_epc_cg(), which is added in this
version.
>>
>> if (!reclaim) {
>> page = ERR_PTR(-EBUSY);
>> @@ -580,10 +593,25 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
>> *owner, bool reclaim)
>> break;
>> }
>>
>> + /*
>> + * Need to do a global reclamation if cgroup was not full but
>> free
>> + * physical pages run out, causing __sgx_alloc_epc_page() to
>> fail.
>> + */
>> sgx_reclaim_pages();
>> cond_resched();
>> }
>
> And why adding this comment, especially in this patch?
>
> I don't see it brings additional clarity because there's only global
> reclaim now, no matter whether cgroup is enabled or not.
>
True there is only global reclamation at the moment. The comment intended
to help clarify why we can get here when try_charge() passes. It at least
took me some thinking to realize that fact so I put it here to help remind
this can happen even for cases that try_charge() passes. (In some earlier
debugging for some concurrent issues, I actually tried mistakenly to
remove this path hoping to narrow down some causes but made situation
worse.)
I can remove if no one thinks this is needed.
Thanks
Haitao
Powered by blists - more mailing lists