Received-SPF: pass (google.com: domain of linux-kernel+bounces-69369-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes
To: jarkko@kernel.org, dave.hansen@linux.intel.com, tj@kernel.org,
 mkoutny@suse.com, linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org,
 x86@kernel.org, cgroups@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
 bp@alien8.de, hpa@zytor.com, sohil.mehta@intel.com,
 tim.c.chen@linux.intel.com, "Dave Hansen" <dave.hansen@intel.com>
Cc: zhiquan1.li@intel.com, kristen@linux.intel.com, seanjc@google.com,
 zhanb@microsoft.com, anakrish@microsoft.com, mikko.ylinen@linux.intel.com,
 yangjie@microsoft.com, chrisyan@microsoft.com
Subject: Re: [PATCH v9 09/15] x86/sgx: Charge mem_cgroup for per-cgroup
 reclamation
References: <20240205210638.157741-1-haitao.huang@linux.intel.com>
 <20240205210638.157741-10-haitao.huang@linux.intel.com>
 <a5bd910b-3148-47ec-9280-561cfe6c16df@intel.com>
Date: Fri, 16 Feb 2024 15:38:32 -0600
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.2i87qioawjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <a5bd910b-3148-47ec-9280-561cfe6c16df@intel.com>
User-Agent: Opera Mail/1.0 (Win32)

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/