Re: [PATCH v9 09/15] x86/sgx: Charge mem_cgroup for per-cgroup reclamation

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


On Fri, 16 Feb 2024 09:15:59 -0600, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:

On 2/5/24 13:06, Haitao Huang wrote:
@@ -414,7 +416,7 @@ static void sgx_reclaim_pages_global(void)
 void sgx_reclaim_direct(void)
 	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
-		sgx_reclaim_pages_global();
+		sgx_reclaim_pages_global(false);

 static int ksgxd(void *p)
@@ -437,7 +439,7 @@ static int ksgxd(void *p)

 		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES))
-			sgx_reclaim_pages_global();
+			sgx_reclaim_pages_global(true);


First, I'm never a fan of random true/false or 0/1 arguments to
functions like this.  You end up having to go look at the called
function to make any sense of it.  You can either do an enum, or some
construct like this:

 		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES)) {
			bool indirect = true;

Yeah, it takes a few more lines but it saves you having to comment the

Does this 'indirect' change any behavior other than whether it does a
search for an mm to find a place to charge the backing storage?


Instead of passing a flag around, why not just pass the mm?

There is no need to pass in mm. We could just check if current->mm == NULL for the need of doing the search in the enclave mm list.

But you had a concern [1] that the purpose was not clear hence suggested current_is_ksgxd().

Would it be OK if we replace current_is_ksgxd() with (current->flags & PF_KTHREAD)? That would express the real intent of checking if calling context is not in a user context.

This refactoring out of 'indirect' or passing the mm around really wants
to be in its own patch anyway.

Looks like I could do:
1) refactoring of 'indirect' value/enum suggested above. This seems the most straightforward without depending on any assumptions of other kernel code. 2) replace current_is_ksgxd() with current->mm == NULL. This assumes kthreads has no mm. 3) replace current_is_ksgxd() with current->flags & PF_KTHREAD. This is direct use of the flag PF_KTHREAD, so it should be better than #2?

Any preference or further thoughts?



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

  Powered by Linux