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

[Tim Chen]

On Mon, 2024-01-22 at 09:20 -0800, Haitao Huang wrote:
> 
> @@ -1047,29 +1037,38 @@ static struct mem_cgroup *sgx_encl_get_mem_cgroup(struct sgx_encl *encl)
>   * @encl:	an enclave pointer
>   * @page_index:	enclave page index
>   * @backing:	data for accessing backing storage for the page
> + * @indirect:	in ksgxd or EPC cgroup work queue context
> + *
> + * Create a backing page for loading data back into an EPC page with ELDU. This function takes
> + * a reference on a new backing page which must be dropped with a corresponding call to
> + * sgx_encl_put_backing().
>   *
> - * When called from ksgxd, sets the active memcg from one of the
> - * mms in the enclave's mm_list prior to any backing page allocation,
> - * in order to ensure that shmem page allocations are charged to the
> - * enclave.  Create a backing page for loading data back into an EPC page with
> - * ELDU.  This function takes a reference on a new backing page which
> - * must be dropped with a corresponding call to sgx_encl_put_backing().
> + * When @indirect is true, sets the active memcg from one of the mms in the enclave's mm_list
> + * prior to any backing page allocation, in order to ensure that shmem page allocations are
> + * charged to the enclave.
>   *
>   * Return:
>   *   0 on success,
>   *   -errno otherwise.
>   */
>  int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
> -			   struct sgx_backing *backing)
> +			   struct sgx_backing *backing, bool indirect)
>  {
> -	struct mem_cgroup *encl_memcg = sgx_encl_get_mem_cgroup(encl);
> -	struct mem_cgroup *memcg = set_active_memcg(encl_memcg);
> +	struct mem_cgroup *encl_memcg;
> +	struct mem_cgroup *memcg;
>  	int ret;
>  
> +	if (indirect) {
> +		encl_memcg = sgx_encl_get_mem_cgroup(encl);
> +		memcg = set_active_memcg(encl_memcg);
> +	}
> +
>  	ret = __sgx_encl_get_backing(encl, page_index, backing);
>  
> -	set_active_memcg(memcg);
> -	mem_cgroup_put(encl_memcg);
> +	if (indirect) {
> +		set_active_memcg(memcg);
> +		mem_cgroup_put(encl_memcg);
> +	}
>  


You can reduce the number of if statements to make the logic
simpler.  Something like

	if (!indirect)
		return __sgx_encl_get_backing(encl, page_index, backing);

	encl_memcg = sgx_encl_get_mem_cgroup(encl);
	memcg = set_active_memcg(encl_memcg);
	ret = __sgx_encl_get_backing(encl, page_index, backing);
	set_active_memcg(memcg);
	mem_cgroup_put(encl_memcg);

>  	return ret;

Tim