Received-SPF: pass (google.com: domain of linux-kernel+bounces-149332-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <8a9ae08a-c813-442d-9fc3-031a4c984700@intel.com>
Date: Thu, 18 Apr 2024 11:51:28 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v12 08/14] x86/sgx: Add basic EPC reclamation flow for
 cgroup
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>,
	<tim.c.chen@linux.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>
References: <20240416032011.58578-1-haitao.huang@linux.intel.com>
 <20240416032011.58578-9-haitao.huang@linux.intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240416032011.58578-9-haitao.huang@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T2pDbnVoWlBhR3g5Zk1ETWVPaCthNk0yTFBFckZGK24xUnNKeFVsQmg3bWJC?=
 =?utf-8?B?dCtMdGx1NTM3YmlZVDN2eXR3T1pjZU1PdVpGd3RObXR5bU52UXIwMEtDQk13?=
 =?utf-8?B?Z3JEdGtiRlowaW55Qk0wS0pEQTNDOXlyYlNEYlVQYytoMEQ2dkFIMDEzOFdP?=
 =?utf-8?B?YVlRaEdSL3l3d2tVVkdZcTJ0TlM4eDlSVVYvMEhuZXpSWmN6ZUxZYWpRUTJD?=
 =?utf-8?B?RTZhVTJ5VWNrc2tuTjNaQS9rTXZ3ZkgrNmJwNmF2QTcxS2JZTVZFSnYrcGsz?=
 =?utf-8?B?MUFnblRsNlFYbk4vUmlId2tPbTFjZVZrNlBMdmErc0oxZzNWWDI4c2p2Y0Na?=
 =?utf-8?B?bDdldk9XTjVQOWpmNE9KSlUvKzI2SUZmanFNbksvSnB6eW0rdS9IT3dHWjhu?=
 =?utf-8?B?SFN2cWYyekpTckFaeE95OEFWZzBQeVk2QUs5Qm5VVDJPL3JZOGY2Q1NqcFdG?=
 =?utf-8?B?bzVpcjl2RldRNnpCMG9wZ3ZOM2Zoc0JoN2doMzd0YU1xTFBUNC96Mi8yTWl6?=
 =?utf-8?B?RHBXUDBzMTBKWW9jamN4bTNIajAyVTVtdk05RDVvSEUzRFdYSk5NaWZ5d3JZ?=
 =?utf-8?B?WkpOVnE3UDU5RXpFemZqTXA1QVJhZXpYR1dlSzBNN1JwOHI4R0ZCWWlyUzZE?=
 =?utf-8?B?aWZJdjhjNkd4dnZjc0Z1MngzczM0T1NCUzJvTGJuUWgwc3ArNlR1MkZNWUdm?=
 =?utf-8?B?Z2lPU3VlelZhV0xDWE5VbVplTEdFaW9nb3E2aHhqMXMxWnZQZm5lWTZ6bGJN?=
 =?utf-8?B?MjBJVnljY0hYeGxKKzFJMjBuY2dlajNIOVFjRU1penVjc29xSGdPRmkrbzVi?=
 =?utf-8?B?Q3Fkc28rZ29uUm5oNkVhdHBacFNyNUF2bXdoMmdKNmtZNUFRMk5jak9wT1VF?=
 =?utf-8?B?Y0RlN1RORFF0Q3BKNllVR2ZPOEM1c3dhaUVacjlmYy93WFVPVmUwZGdCWE5C?=
 =?utf-8?B?Ylp5ZkFMWmxXYkYyZlhXeU9sK2NnYkJQZTduaWp5aWpVenVIem5FaTMxUjJB?=
 =?utf-8?B?U3E5d3VpN2lBZlpicU9Ud1Q0d2tPZXp0dVE0S1hIVlNiYTBzQ0R4RGx4RS9w?=
 =?utf-8?B?eHkxZjBLbjQ3N3Y0emN3UmpyRi9JQlVaQlBkWU05RTR4cElra0V0ckgrY0VV?=
 =?utf-8?B?SURhaTAzM3hINXhNRmZqR2Nia1J5S3FWU2V6b2ViM1B2K1crcW4zUkszL0d5?=
 =?utf-8?B?Y3d1Mk5UcUhEZXlqOVlkR2dtS3JGRjM5U1FlUXpOZHZEUTRpNW1qQWY4WFZR?=
 =?utf-8?B?azV5RHNoZU44RjFHcDkwSUEyaWhJNXRKWk1HMGx2b2lSUlRHTlpyNDlLcndD?=
 =?utf-8?B?bnpia2VBeVo4Y0dvdW1wMzNYM21pQVBLalUrbVVvOG9qNTUwYnM3RjJnWmRs?=
 =?utf-8?B?T29NTmFBTHNYbEJVcGhzRE42aWhjOU9hbkhCRlpZaXZKNDNQZXJaa0x6ZmNm?=
 =?utf-8?B?VGJ6TjVzQ3JyV0xXTU5odlBTQnJIY2ZmQXlDbUpnU0tlcDhSQ3M3Vjh3eTBn?=
 =?utf-8?B?MEVhZHdwMzNpTmNReCsrMUMwdGpqVllCUTIzZEVhZEx4TFNJTnhpYkYzMjNI?=
 =?utf-8?B?YnpRVXUxRVVmTWpQd1g2ZWhwL3B3eERTODk2MmpTUHpKRmI3Yy9lSDhBaW1n?=
 =?utf-8?B?ZnVFWEVpc2ltZXhPL3VyTkFPaHpuLzBrcTh6NzZpM0tNUE12Z1JQUnVFNjc5?=
 =?utf-8?B?SGFpWUtPRWpSWjJvWHptWWE3VzFPRnJQRGVBZnozcmc5VC9BL0xFVFB4eFY0?=
 =?utf-8?B?ZTVvTWkzZmlUTTVFWnI2cVRpdmxEc2lUYkdIU3hkanltRDg3QWJzZmtrSEFn?=
 =?utf-8?B?S1UyTHFZSGE4SGdGVGJ4NVVuaWVuQTlMeDYxMXoyT1YvbzgxdTgwQlNaRDhl?=
 =?utf-8?B?QWxRT21iVGpOVW1jNnNPeERrd01ZQjRkYUphODRIOGVQMitrVVh3UWxwaHEz?=
 =?utf-8?B?Sm9HN0ZLZGprd3FqVEhMRTBYMnRXUmg1WkVtb1dpd0pXVlhUbXQ4eC93dWFQ?=
 =?utf-8?B?ZFRSaUxxMUtlTisyOCtKa1dTYzhKM3VNdXNhYXhrZUtSdDQ3WkJ2amsyTDJi?=
 =?utf-8?B?ZXpFTVVtZW83Q0l5dUdDdi84Z0pkMXdnMUxTKzdIb2VnZUpIdHpXUm5FNUYz?=
 =?utf-8?Q?d2TTwASJTbsEBJvluW4UxcSM8?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a4f55cb4-09cf-42df-93bf-08dc5f39537a
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2024 23:51:39.9776
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Hhfr38oNHWrGN9FbtGCgqq4F6fN8k1Pu2cMPOnKaTQNJeiFxvjq+bu8UOxV6pBWkkCbHY49/e8n1uIit39UI5Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5268
X-OriginatorOrg: intel.com


On 16/04/2024 3:20 pm, Haitao Huang wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> Currently in the EPC page allocation, the kernel simply fails the
> allocation when the current EPC cgroup fails to charge due to its usage
> reaching limit.  This is not ideal. When that happens, a better way is
> to reclaim EPC page(s) from the current EPC cgroup (and/or its
> descendants) to reduce its usage so the new allocation can succeed.
> 
> Add the basic building blocks to support per-cgroup reclamation.
> 
> Currently the kernel only has one place to reclaim EPC pages: the global
> EPC LRU list.  To support the "per-cgroup" EPC reclaim, maintain an LRU
> list for each EPC cgroup, and introduce a "cgroup" variant function to
> reclaim EPC pages from a given EPC cgroup and its descendants.
> 
> Currently the kernel does the global EPC reclaim in sgx_reclaim_page().
> It always tries to reclaim EPC pages in batch of SGX_NR_TO_SCAN (16)
> pages.  Specifically, it always "scans", or "isolates" SGX_NR_TO_SCAN
> pages from the global LRU, and then tries to reclaim these pages at once
> for better performance.
> 
> Implement the "cgroup" variant EPC reclaim in a similar way, but keep
> the implementation simple: 1) change sgx_reclaim_pages() to take an LRU
> as input, and return the pages that are "scanned" and attempted for
> reclamation (but not necessarily reclaimed successfully); 2) loop the
> given EPC cgroup and its descendants and do the new sgx_reclaim_pages()
> until SGX_NR_TO_SCAN pages are "scanned".
> 
> This implementation, encapsulated in sgx_cgroup_reclaim_pages(), always
> tries to reclaim SGX_NR_TO_SCAN pages from the LRU of the given EPC
> cgroup, and only moves to its descendants when there's no enough
> reclaimable EPC pages to "scan" in its LRU.  It should be enough for
> most cases.
> 
> Note, this simple implementation doesn't _exactly_ mimic the current
> global EPC reclaim (which always tries to do the actual reclaim in batch
> of SGX_NR_TO_SCAN pages): when LRUs have less than SGX_NR_TO_SCAN
> reclaimable pages, the actual reclaim of EPC pages will be split into
> smaller batches _across_ multiple LRUs with each being smaller than
> SGX_NR_TO_SCAN pages.
> 
> A more precise way to mimic the current global EPC reclaim would be to
> have a new function to only "scan" (or "isolate") SGX_NR_TO_SCAN pages
> _across_ the given EPC cgroup _AND_ its descendants, and then do the
> actual reclaim in one batch.  But this is unnecessarily complicated at
> this stage.
> 
> Alternatively, the current sgx_reclaim_pages() could be changed to
> return the actual "reclaimed" pages, but not "scanned" pages. However,
> the reclamation is a lengthy process, forcing a successful reclamation
> of predetermined number of pages may block the caller for too long. And
> that may not be acceptable in some synchronous contexts, e.g., in
> serving an ioctl().
> 
> With this building block in place, add synchronous reclamation support
> in sgx_cgroup_try_charge(): trigger a call to
> sgx_cgroup_reclaim_pages() if the cgroup reaches its limit and the
> caller allows synchronous reclaim as indicated by s newly added
> parameter.
> 
> A later patch will add support for asynchronous reclamation reusing
> sgx_cgroup_reclaim_pages().
> 
> Note all reclaimable EPC pages are still tracked in the global LRU thus
> no per-cgroup reclamation is actually active at the moment. Per-cgroup
> tracking and reclamation will be turned on in the end after all
> necessary infrastructure is in place.

Nit:

"all necessary infrastructures are in place", or, "all necessary 
building blocks are in place".

?

> 
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---

Reviewed-by: Kai Huang <kai.huang@intel.com>

More nitpickings below:

[...]

> -static inline int sgx_cgroup_try_charge(struct sgx_cgroup *sgx_cg)
> +static inline int sgx_cgroup_try_charge(struct sgx_cgroup *sgx_cg, enum sgx_reclaim reclaim)

Let's still wrap the text on 80-character basis.

I guess most people are more used to that.

[...]

> -		epc_page = list_first_entry_or_null(&sgx_global_lru.reclaimable,
> -						    struct sgx_epc_page, list);
> +		epc_page = list_first_entry_or_null(&lru->reclaimable, struct sgx_epc_page, list);

Ditto.