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

[Haitao Huang]

Hi Tim,
On Fri, 02 Feb 2024 17:45:05 -0600, Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>  
wrote:

> 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

Yes, will do.
Thanks
Haitao