On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
>
> In a later patch, when a cgroup has exceeded the max capacity for EPC
> pages, it may need to identify and OOM kill a less active enclave to
> make room for other enclaves within the same group. Such a victim
> enclave would have no active pages other than the unreclaimable Version
> Array (VA) and SECS pages.
>
What does "no active pages" mean?
A "less active enclave" doesn't necessarily mean it has "no active pages"?
> Therefore, the cgroup needs examine its
^
needs to
> unreclaimable page list, and finding an enclave given a SECS page or a
^
find
> VA page. This will require a backpointer from a page to an enclave,
> which is not available for VA pages.
>
> Because struct sgx_epc_page instances of VA pages are not owned by an
> sgx_encl_page instance, mark their owner as sgx_encl: pass the struct
> sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(),
> which will store this value in the owner field of the struct
> sgx_epc_page.
>
IMHO this paragraph is hard to understand and can be more concise:
One VA page can be shared by multiple enclave pages thus cannot be associated
with any 'struct sgx_encl_page' instance. Set the owner of VA page to the
enclave instead.
> In a later patch, VA pages will be placed in an
> unreclaimable queue that can be examined by the cgroup to select the OOM
> killed enclave.
The code to "place the VA page to unreclaimable queue" has been done in earlier
patch ("x86/sgx: Introduce EPC page states"). Just the unreclaimable list isn't
introduced yet. I think you should just introduce it first then you can get rid
of those "in a later patch" staff.
And nit: please use "unreclaimable list" consistently (not queue).
Btw, probably a dumb question:
Theoretically if you only need to find a victim enclave you don't need to put VA
pages to the unreclaimable list, because those VA pages will be freed anyway
when enclave is killed. So keeping VA pages in the list is for accounting all
the pages that the cgroup is having?
