Received-SPF: pass (google.com: domain of linux-kernel+bounces-69485-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>
 <op.2i87qioawjvjmi@hhuan26-mobl.amr.corp.intel.com>
 <0ab99d42-c3fe-4415-b993-57fb1fec21a4@intel.com>
Date: Fri, 16 Feb 2024 17:33:07 -0600
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.2i9c1hr5wjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <0ab99d42-c3fe-4415-b993-57fb1fec21a4@intel.com>
User-Agent: Opera Mail/1.0 (Win32)

On Fri, 16 Feb 2024 15:55:10 -0600, Dave Hansen <dave.hansen@intel.com>  
wrote:

> On 2/16/24 13:38, Haitao Huang wrote:
>> On Fri, 16 Feb 2024 09:15:59 -0600, Dave Hansen <dave.hansen@intel.com>
>> wrote:
> ...
>>> 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().
>
> Right, because there was only one possible way that mm could be NULL but
> it wasn't obvious from the code what that way was.
>
>> 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.
>
> No, I think that focuses on the symptom and not on the fundamental  
> problem.
>
> The fundamental problem is that you need an mm in order to charge your
> allocations to the right group.  Indirect reclaim means you are not in a
> context which is connected to the mm that should be charged while direct
> reclaim is.
>
>>> 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?
>
> Pass around a:
>
> 	struct mm_struct *charge_mm
>
> Then, at the bottom do:
>
> 	/*
> 	 * Backing RAM allocations need to be charged to some mm and
> 	 * associated cgroup.  If this context does not have an mm to
> 	 * charge, search the enclave's mm_list to find some mm
> 	 * associated with this enclave.
> 	 */
> 	if (!charge_mm)
> 		... do slow mm lookup
> 	else
> 		return mm_to_cgroup_whatever(charge_mm);
>
> Then just comment the call sites where the initial charge_mm comes in:
>
> 	
> 	/* Indirect SGX reclaim, no mm to charge, so NULL: */
> 	foo(..., NULL);
>
>
> 	/* Direct SGX reclaim, charge current mm for allocations: */
> 	foo(..., current->mm);
>
>
Okay. got it now.

Thank you very much!

Haitao