On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
>
> Adjust and expose the top-level reclaim function as
> sgx_reclaim_epc_pages() for use by the upcoming EPC cgroup, which will
> initiate reclaim to enforce the max limit.
>
> Make these adjustments to the function signature.
>
> 1) To take a parameter that specifies the number of pages to scan for
> reclaiming. Define a max value of 32, but scan 16 in the case for the
> global reclaimer (ksgxd). The EPC cgroup will use it to specify a
> desired number of pages to be reclaimed up to the max value of 32.
>
> 2) To take a flag to force reclaiming a page regardless of its age. The
> EPC cgroup will use the flag to enforce its limits by draining the
> reclaimable lists before resorting to other measures, e.g. forcefully
> kill enclaves.
>
> 3) Return the number of reclaimed pages. The EPC cgroup will use the
> result to track reclaiming progress and escalate to a more forceful
> reclaiming mode, e.g., calling this function with the flag to ignore age
> of pages.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Co-developed-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> Co-developed-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> V4:
> - Combined the 3 patches that made the individual changes to the
> function signature.
> - Removed 'high' limit in commit message.
> ---
> arch/x86/kernel/cpu/sgx/main.c | 31 +++++++++++++++++++++----------
> arch/x86/kernel/cpu/sgx/sgx.h | 1 +
> 2 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 3b875ab4dcd0..4e1a3e038db5 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -18,6 +18,11 @@
> #include "encl.h"
> #include "encls.h"
>
> +/*
> + * Maximum number of pages to scan for reclaiming.
> + */
> +#define SGX_NR_TO_SCAN_MAX 32
> +
> struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
> static int sgx_nr_epc_sections;
> static struct task_struct *ksgxd_tsk;
> @@ -279,7 +284,11 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
> mutex_unlock(&encl->lock);
> }
>
> -/*
> +/**
> + * sgx_reclaim_epc_pages() - Reclaim EPC pages from the consumers
> + * @nr_to_scan: Number of EPC pages to scan for reclaim
> + * @ignore_age: Reclaim a page even if it is young
> + *
> * Take a fixed number of pages from the head of the active page pool and
> * reclaim them to the enclave's private shmem files. Skip the pages, which have
> * been accessed since the last scan. Move those pages to the tail of active
> @@ -292,15 +301,14 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
> * problematic as it would increase the lock contention too much, which would
> * halt forward progress.
> */
> -static void sgx_reclaim_pages(void)
> +size_t sgx_reclaim_epc_pages(size_t nr_to_scan, bool ignore_age)
'size_t' looks odd. Any reason to use it?
Given you only scan 32 at maximum, seems 'int' is good enough?
> {
> - struct sgx_backing backing[SGX_NR_TO_SCAN];
> + struct sgx_backing backing[SGX_NR_TO_SCAN_MAX];
> struct sgx_epc_page *epc_page, *tmp;
> struct sgx_encl_page *encl_page;
> pgoff_t page_index;
> LIST_HEAD(iso);
> - int ret;
> - int i;
> + size_t ret, i;
>
> spin_lock(&sgx_global_lru.lock);
> for (i = 0; i < SGX_NR_TO_SCAN; i++) {
The function comment says
* @nr_to_scan: Number of EPC pages to scan for reclaim
But I don't see it is even used, if my eye isn't deceiving me?
> @@ -326,13 +334,14 @@ static void sgx_reclaim_pages(void)
> spin_unlock(&sgx_global_lru.lock);
>
> if (list_empty(&iso))
> - return;
> + return 0;
>
> i = 0;
> list_for_each_entry_safe(epc_page, tmp, &iso, list) {
> encl_page = epc_page->encl_page;
>
> - if (!sgx_reclaimer_age(epc_page))
> + if (i == SGX_NR_TO_SCAN_MAX ||
i == nr_to_scan?
And should we have a
if (nr_to_scan < SGX_NR_TO_SCAN_MAX)
return 0;
at the very beginning of this function?
> + (!ignore_age && !sgx_reclaimer_age(epc_page)))
> goto skip;
>
> page_index = PFN_DOWN(encl_page->desc - encl_page->encl->base);
> @@ -371,6 +380,8 @@ static void sgx_reclaim_pages(void)
>
> sgx_free_epc_page(epc_page);
> }
> +
> + return i;
> }
>
I found this function a little bit odd, given the mixing of 'nr_to_scan',
SGX_NR_TO_SCAN and SGX_NR_TO_SCAN_MAX.
>From the changelog:
1) To take a parameter that specifies the number of pages to scan for
reclaiming. Define a max value of 32, but scan 16 in the case for the
global reclaimer (ksgxd).
It appears we want to make this function to scan @nr_to_scan for cgroup, but
still want to scan a fixed value for ksgxd, which is SGX_NR_TO_SCAN. And
@nr_to_scan can be larger than SGX_NR_TO_SCAN but smaller than
SGX_NR_TO_SCAN_MAX.
Putting behind the mystery of why above is needed, to achieve it, is it more
clear if we do below?
int __sgx_reclaim_epc_pages(int nr_to_scan, bool ignore_age)
{
struct sgx_backing backing[SGX_NR_TO_SCAN_MAX];
...
if (nr_to_scan > SGX_NR_TO_SCAN_MAX)
return 0;
for (i = 0; i < nr_to_scan; i++) {
...
}
return reclaimed;
}
/* This is for ksgxd() */
int sgx_reclaim_epc_page(void)
{
return __sgx_reclaim_epc_pages(SGX_NR_TO_SCAN, false);
}
EPC cgroup calls __sgx_reclaim_epc_pages() directly, or introduce another
wrapper.
