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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux