On Tue, 30 Jan 2024 09:22:14 -0600, Huang, Kai <kai.huang@xxxxxxxxx> 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
