Received-SPF: pass (google.com: domain of linux-kernel+bounces-50728-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <ca25d3462944dcea553665d7c85903cdd6a846bc.camel@linux.intel.com>
Subject: Re: [PATCH v7 09/15] x86/sgx: Charge mem_cgroup for per-cgroup
 reclamation
From: Tim Chen <tim.c.chen@linux.intel.com>
To: Haitao Huang <haitao.huang@linux.intel.com>, 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
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
Date: Fri, 02 Feb 2024 15:45:05 -0800
In-Reply-To: <20240122172048.11953-10-haitao.huang@linux.intel.com>
References: <20240122172048.11953-1-haitao.huang@linux.intel.com>
	 <20240122172048.11953-10-haitao.huang@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-01-22 at 09:20 -0800, Haitao Huang wrote:
>=20
> @@ -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 ELD=
U. This function takes
> + * a reference on a new backing page which must be dropped with a corres=
ponding 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 pag=
e 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 p=
age allocations are
> + * charged to the enclave.
>   *
>   * Return:
>   *   0 on success,
>   *   -errno otherwise.
>   */
>  int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_ind=
ex,
> -			   struct sgx_backing *backing)
> +			   struct sgx_backing *backing, bool indirect)
>  {
> -	struct mem_cgroup *encl_memcg =3D sgx_encl_get_mem_cgroup(encl);
> -	struct mem_cgroup *memcg =3D set_active_memcg(encl_memcg);
> +	struct mem_cgroup *encl_memcg;
> +	struct mem_cgroup *memcg;
>  	int ret;
> =20
> +	if (indirect) {
> +		encl_memcg =3D sgx_encl_get_mem_cgroup(encl);
> +		memcg =3D set_active_memcg(encl_memcg);
> +	}
> +
>  	ret =3D __sgx_encl_get_backing(encl, page_index, backing);
> =20
> -	set_active_memcg(memcg);
> -	mem_cgroup_put(encl_memcg);
> +	if (indirect) {
> +		set_active_memcg(memcg);
> +		mem_cgroup_put(encl_memcg);
> +	}
> =20


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 =3D sgx_encl_get_mem_cgroup(encl);
	memcg =3D set_active_memcg(encl_memcg);
	ret =3D __sgx_encl_get_backing(encl, page_index, backing);
	set_active_memcg(memcg);
	mem_cgroup_put(encl_memcg);

>  	return ret;

Tim