On Wed, 27 Sep 2023 05:28:36 -0500, Huang, Kai <kai.huang@xxxxxxxxx> wrote:
On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
Use the lower 3 bits in the flags field of sgx_epc_page struct to
track EPC states in its life cycle and define an enum for possible
states. More state(s) will be added later.
This patch does more than what the changelog claims to do. AFAICT it
does
below:
1) Use the lower 3 bits to track EPC page status
2) Rename SGX_EPC_PAGE_RECLAIMER_TRACKED to SGX_EPC_PAGE_RERCLAIMABLE
3) Introduce a new state SGX_EPC_PAGE_UNRECLAIMABLE
4) Track SECS and VA pages as SGX_EPC_PAGE_UNRECLAIMABLE
The changelog only says 1) IIUC.
I don't quite get why you would view 3) as a separate item from 1).
In my view, 4) is not done as long as there is not separate list to track
it.
Maybe I should make it clear the "states" vs "tracking". States are just
bits in the flags, "tracking" is done using the lists by ksgxd/cgroup. And
this patch is really about "states"
Would that clarify the intention of the patch?
If we really want to do all these in one patch, then the changelog
should at
least mention the justification of all of them.
But I don't see why 3) and 4) need to be done here. Instead, IMHO they
should
be done in a separate patch, and do it after the unreclaimable list is
introduced (or you need to bring that patch forward).
For instance, ...
[snip]
+
+ /* Page is in use but tracked in an unreclaimable LRU list. These are
+ * only reclaimable when the whole enclave is OOM killed or the
enclave
+ * is released, e.g., VA, SECS pages
+ * Becomes NOT_TRACKED after sgx_drop_epc()
+ */
+ SGX_EPC_PAGE_UNRECLAIMABLE = 3,
... We even don't have the unreclaimable LRU list yet. It's odd to have
this
comment here.
Yeah, I should take out the mentioning of the LRUs from definitions of the
states.
Thanks
Haitao
