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

[Dave Hansen <dave.hansen@intel.com>]

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);