Messages in this thread Patch in this message | | | Subject | Re: [PATCH v9 09/15] x86/sgx: Charge mem_cgroup for per-cgroup reclamation | Date | Fri, 16 Feb 2024 15:38:32 -0600 | From | "Haitao Huang" <> |
| |
On Fri, 16 Feb 2024 09:15:59 -0600, Dave Hansen <dave.hansen@intel.com> 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) >> sgx_should_reclaim(SGX_NR_HIGH_PAGES)); >> >> if (sgx_should_reclaim(SGX_NR_HIGH_PAGES)) >> - sgx_reclaim_pages_global(); >> + sgx_reclaim_pages_global(true); >> >> cond_resched(); >> } > > 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; > sgx_reclaim_pages_global(indirect); > } > > Yeah, it takes a few more lines but it saves you having to comment the > thing. > > 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?
No.
> 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?
Thanks Haitao [1] https://lore.kernel.org/linux-sgx/9c269c70-35fe-a1a4-34c9-b1e62ab3bb3b@intel.com/
| |