Re: [PATCH v9 10/15] x86/sgx: Add EPC reclamation in cgroup try_charge()

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

 



On Wed, 21 Feb 2024 05:06:02 -0600, Huang, Kai <kai.huang@xxxxxxxxx> wrote:


-int sgx_epc_cgroup_try_charge(struct sgx_epc_cgroup *epc_cg)
+int sgx_epc_cgroup_try_charge(struct sgx_epc_cgroup *epc_cg, bool reclaim)
 {
-	return misc_cg_try_charge(MISC_CG_RES_SGX_EPC, epc_cg->cg, PAGE_SIZE);
+	for (;;) {
+		if (!misc_cg_try_charge(MISC_CG_RES_SGX_EPC, epc_cg->cg,
+					PAGE_SIZE))
+			break;
+
+		if (sgx_epc_cgroup_lru_empty(epc_cg->cg))
+			return -ENOMEM;
+
+		if (signal_pending(current))
+			return -ERESTARTSYS;
+
+		if (!reclaim) {
+			queue_work(sgx_epc_cg_wq, &epc_cg->reclaim_work);
+			return -EBUSY;
+		}
+
+		if (!sgx_epc_cgroup_reclaim_pages(epc_cg->cg, false))
+			/* All pages were too young to reclaim, try again a little later */
+			schedule();
+	}
+
+	return 0;
 }


Seems this code change is 90% similar to the existing code in the
sgx_alloc_epc_page():

	...
	for ( ; ; ) {
                page = __sgx_alloc_epc_page();
                if (!IS_ERR(page)) {
                        page->owner = owner;
                        break;
                }

                if (list_empty(&sgx_active_page_list))
                        return ERR_PTR(-ENOMEM);

                if (!reclaim) {
                        page = ERR_PTR(-EBUSY);
                        break;
                }

                if (signal_pending(current)) {
                        page = ERR_PTR(-ERESTARTSYS);
                        break;
                }

                sgx_reclaim_pages();
                cond_resched();
        }
	...

Is it better to move the logic/code change in try_charge() out to
sgx_alloc_epc_page() to unify them?

IIUC, the logic is quite similar: When you either failed to allocate one page, or failed to charge one page, you try to reclaim EPC page(s) from the current
EPC cgroup, either directly or indirectly.

No?

Only these lines are the same:
                if (!reclaim) {
                        page = ERR_PTR(-EBUSY);
                        break;
                }

                if (signal_pending(current)) {
                        page = ERR_PTR(-ERESTARTSYS);
                        break;
                }

In sgx_alloc_epc_page() we do global reclamation but here we do per-cgroup reclamation. That's why the logic of other lines is different though they look similar due to similar function names. For the global reclamation we need consider case in that cgroup is not enabled. Similarly list_empty(&sgx_active_page_list) would have to be changed to check root cgroup if cgroups enabled otherwise check global LRU. The (!reclaim) case is also different. So I don't see an obvious good way to abstract those to get meaningful savings.

Thanks
Haitao




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

  Powered by Linux