Leave the initiaization of @cg to a later phase where @sgx_cg is
guaranteed not being NULL, or initialize @cg to NULL here and update later.
> + struct misc_cg *next_cg = NULL;
> +
> + /*
> + * Make sure there are some free pages at both cgroup and global levels.
> + * In both cases, only make one attempt of reclamation to avoid lengthy
> + * block on the caller.
> + */
> + if (sgx_cg && sgx_cgroup_should_reclaim(sgx_cg))
> + next_cg = sgx_cgroup_reclaim_pages(cg, next_cg, current->mm);
I don't quite follow the logic.
First of all, sgx_cgroup_reclaim_pages() isn't called in a loop, so why
not just do:
next_cg = sgx_cgroup_reclaim_pages(cg, NULL, current->mm);
And what is the point of set @next_cg here, since ...
> +
> + if (next_cg != cg)
> + put_misc_cg(next_cg);
> +
> + next_cg = NULL;
... here @next_cg is reset to NULL ?
Looks the only reason is you need to do ...
put_misc_cg(next_cg);
... above?
This piece of code appears repeatedly in this file. Is there any way we
can get rid of it?
> if (sgx_should_reclaim_global(SGX_NR_LOW_PAGES))
> - sgx_reclaim_pages_global(current->mm);
> + next_cg = sgx_reclaim_pages_global(next_cg, current->mm);
And this doesn't seems "global reclaim" at all?
Because it essentially equals to:
next_cg = sgx_reclaim_pages_global(NULL, current->mm);
which always reclaims from the ROOT.
So each call to sgx_reclaim_direct() will always reclaim from the ROOT --
any other LRUs in the hierarchy will unlikely to get any chance to be
reclaimed.
> +
> + if (next_cg != misc_cg_root())
> + put_misc_cg(next_cg);
> +
> + sgx_put_cg(sgx_cg);
> }
>
> static int ksgxd(void *p)
> {
> + struct misc_cg *next_cg = NULL;
> +
> set_freezable();
>
> /*
> @@ -437,11 +489,15 @@ static int ksgxd(void *p)
> kthread_should_stop() ||
> sgx_should_reclaim_global(SGX_NR_HIGH_PAGES));
>
> - if (sgx_should_reclaim_global(SGX_NR_HIGH_PAGES))
> + while (!kthread_should_stop() && sgx_should_reclaim_global(SGX_NR_HIGH_PAGES)) {
> /* Indirect reclaim, no mm to charge, so NULL: */
> - sgx_reclaim_pages_global(NULL);
> + next_cg = sgx_reclaim_pages_global(next_cg, NULL);
> + cond_resched();
Should the 'put_misc_cg(next_cg)' be done within the while() loop but not
below?
> + }
>
> - cond_resched();
> + if (next_cg != misc_cg_root())
> + put_misc_cg(next_cg);
> + next_cg = NULL;
Again, it doesn't seems "global reclaim" here, since you always restart
from the ROOT once the target pages have been reclaimed.
AFAICT it's completely different from the existing global reclaim.
> }
>
> return 0;
> @@ -583,6 +639,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
> */
> struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_reclaim reclaim)
> {
> + struct misc_cg *next_cg = NULL;
> struct sgx_cgroup *sgx_cg;
> struct sgx_epc_page *page;
> int ret;
> @@ -616,10 +673,19 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_reclaim reclaim)
> break;
> }
>
> - sgx_reclaim_pages_global(current->mm);
> + /*
> + * At this point, the usage within this cgroup is under its
> + * limit but there is no physical page left for allocation.
> + * Perform a global reclaim to get some pages released from any
> + * cgroup with reclaimable pages.
> + */
> + next_cg = sgx_reclaim_pages_global(next_cg, current->mm);
> cond_resched();
> }
Ditto IIUC.
