Re: [PATCH v9 08/15] x86/sgx: Implement EPC reclamation flows for cgroup

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

 



On Tue, 20 Feb 2024 03:52:39 -0600, Huang, Kai <kai.huang@xxxxxxxxx> wrote:

+/**
+ * sgx_epc_cgroup_reclaim_pages() - walk a cgroup tree and scan LRUs to reclaim pages
+ * @root:	Root of the tree to start walking from.
+ * Return:	Number of pages reclaimed.

Just wondering, do you need to return @cnt given this function is called w/o
checking the return value?

Yes. Will add explicit commenting that we need scan fixed number of pages for attempted reclamation.
+ */
+unsigned int sgx_epc_cgroup_reclaim_pages(struct misc_cg *root)
+{
+	/*
+	 * Attempting to reclaim only a few pages will often fail and is
+	 * inefficient, while reclaiming a huge number of pages can result in
+	 * soft lockups due to holding various locks for an extended duration.
+	 */

Not sure we need this comment, given it's already implied in
sgx_reclaim_pages().  You cannot pass a value > SGX_NR_TO_SCAN anyway.

Will rework on these comments to make them more meaningful.


[other comments/questions addressed in separate email threads]
[...]
+
+/*
+ * Scheduled by sgx_epc_cgroup_try_charge() to reclaim pages from the cgroup
+ * when the cgroup is at/near its maximum capacity
+ */

I don't see this being "scheduled by sgx_epc_cgroup_try_charge()" here. Does it
make more sense to move that code change to this patch for better review?


Right. This comment was left-over when I split the old patch.

+static void sgx_epc_cgroup_reclaim_work_func(struct work_struct *work)
+{
+	struct sgx_epc_cgroup *epc_cg;
+	u64 cur, max;
+
+	epc_cg = container_of(work, struct sgx_epc_cgroup, reclaim_work);
+
+	for (;;) {
+		max = sgx_epc_cgroup_max_pages_to_root(epc_cg);
+
+		/*
+		 * Adjust the limit down by one page, the goal is to free up
+		 * pages for fault allocations, not to simply obey the limit.
+		 * Conditionally decrementing max also means the cur vs. max
+		 * check will correctly handle the case where both are zero.
+		 */
+		if (max)
+			max--;

With the below max -= SGX_NR_TO_SCAN/2 staff, do you still need this one?


Logically still needed for case max <= SGX_NR_TO_SCAN * 2

+
+		/*
+		 * Unless the limit is extremely low, in which case forcing
+		 * reclaim will likely cause thrashing, force the cgroup to
+		 * reclaim at least once if it's operating *near* its maximum
+		 * limit by adjusting @max down by half the min reclaim size.

OK. But why choose "SGX_NO_TO_SCAN * 2" as "extremely low"? E.g, could we
choose SGX_NR_TO_SCAN instead?
IMHO at least we should at least put a comment to mention this.

And maybe you can have a dedicated macro for that in which way I believe the
code would be easier to understand?

Good point. I think the value is kind of arbitrary. We consider enclaves/cgroups of 64K size are very small. If such a cgroup ever reaches the limit, then we don't aggressively reclaim to optimize #PF handling. User might as well just raise the limit if it is not performant.


+		 * This work func is scheduled by sgx_epc_cgroup_try_charge

This has been mentioned in the function comment already.

+		 * when it cannot directly reclaim due to being in an atomic
+		 * context, e.g. EPC allocation in a fault handler.

Why a fault handler is an "atomic context"? Just say when it cannot directly
reclaim.


Sure.

Waiting
+		 * to reclaim until the cgroup is actually at its limit is less
+		 * performant as it means the faulting task is effectively
+		 * blocked until a worker makes its way through the global work
+		 * queue.
+		 */
+		if (max > SGX_NR_TO_SCAN * 2)
+			max -= (SGX_NR_TO_SCAN / 2);
+
+		cur = sgx_epc_cgroup_page_counter_read(epc_cg);
+
+		if (cur <= max || sgx_epc_cgroup_lru_empty(epc_cg->cg))
+			break;
+
+		/* Keep reclaiming until above condition is met. */
+		sgx_epc_cgroup_reclaim_pages(epc_cg->cg);

Also, each loop here calls sgx_epc_cgroup_max_pages_to_root() and
sgx_epc_cgroup_lru_empty(), both loop the given EPC cgroup and descendants. If we still make sgx_reclaim_pages() always scan SGX_NR_TO_SCAN pages, seems we can
reduce the number of loops here?


[We already scan SGX_NR_TO_SCAN pages for the cgroup at the level of sgx_epc_cgroup_reclaim_pages().]

I think you mean that we keep scanning and reclaiming until at least SGX_NR_TO_SCAN pages are reclaimed as your code suggested above. We probably can make that a version for this background thread for optimization. But sgx_epc_cgroup_max_pages_to_root() and sgx_epc_cgroup_lru_empty() are not that bad unless we had very deep and wide cgroup trees. So would you agree we defer this optimization for later?


+	}
+}
+
+/**
+ * sgx_epc_cgroup_try_charge() - try to charge cgroup for a single EPC page
  * @epc_cg:	The EPC cgroup to be charged for the page.
  * Return:
  * * %0 - If successfully charged.
@@ -38,6 +209,7 @@ static void sgx_epc_cgroup_free(struct misc_cg *cg)
 	if (!epc_cg)
 		return;

+	cancel_work_sync(&epc_cg->reclaim_work);
 	kfree(epc_cg);
 }

@@ -50,6 +222,8 @@ const struct misc_res_ops sgx_epc_cgroup_ops = {

static void sgx_epc_misc_init(struct misc_cg *cg, struct sgx_epc_cgroup *epc_cg)
 {
+	sgx_lru_init(&epc_cg->lru);
+	INIT_WORK(&epc_cg->reclaim_work, sgx_epc_cgroup_reclaim_work_func);
 	cg->res[MISC_CG_RES_SGX_EPC].priv = epc_cg;
 	epc_cg->cg = cg;
 }
@@ -69,6 +243,11 @@ static int sgx_epc_cgroup_alloc(struct misc_cg *cg)

 void sgx_epc_cgroup_init(void)
 {
+	sgx_epc_cg_wq = alloc_workqueue("sgx_epc_cg_wq",
+					WQ_UNBOUND | WQ_FREEZABLE,
+					WQ_UNBOUND_MAX_ACTIVE);
+	BUG_ON(!sgx_epc_cg_wq);

You cannot BUG_ON() simply due to unable to allocate a workqueue. You can use some way to mark EPC cgroup as disabled but keep going. Static key is one way
although we cannot re-enable it at runtime.


Okay, I'll disable and print a log.

[...]
[workqueue related discussion in separate email]

Thanks
Haitao




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

  Powered by Linux