lkml.org 
[lkml]   [2024]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v9 09/15] x86/sgx: Charge mem_cgroup for per-cgroup reclamation
Date
From
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/

\
 
 \ /
  Last update: 2024-05-27 15:07    [W:0.193 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site