Re: [PATCH v8 04/15] x86/sgx: Implement basic EPC misc cgroup functionality

From: Haitao Huang
Date: Tue Jan 30 2024 - 20:59:47 EST


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