Messages in this thread | | | Subject | Re: [PATCH v9 09/15] x86/sgx: Charge mem_cgroup for per-cgroup reclamation | Date | Mon, 12 Feb 2024 21:21:47 -0600 | From | "Haitao Huang" <> |
| |
On Mon, 12 Feb 2024 13:46:06 -0600, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> On Mon Feb 5, 2024 at 11:06 PM EET, Haitao Huang wrote: >> Enclave Page Cache(EPC) memory can be swapped out to regular system > > "Enclave Page Cache (EPC)" > ~ > Will fix.
[...] >> int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long >> page_index, >> - struct sgx_backing *backing) >> + struct sgx_backing *backing, bool indirect) > > Boolean parameters should be avoided when possible because they confuse > in the call sites. > >> { >> - struct mem_cgroup *encl_memcg = sgx_encl_get_mem_cgroup(encl); >> - struct mem_cgroup *memcg = set_active_memcg(encl_memcg); >> + struct mem_cgroup *encl_memcg; >> + struct mem_cgroup *memcg; >> int ret; >> >> - ret = __sgx_encl_get_backing(encl, page_index, backing); >> + if (!indirect) >> + return __sgx_encl_get_backing(encl, page_index, backing); > > If a call is either in heead or tail of the code block, then > obviously better option is to make __sgx_encl_get_backing() > as non-static sgx_encl_get_backing() and call it in those > call sites that would call this with "false". > > I.e. you need a new patch where this preparation is done. >
This would actually require more intrusive changes to the call stack for global and cgroup reclaim:
{sgx_epc_cgroup_reclaim_pages(),sgx_reclaim_pages_global()}->sgx_reclaim_pages()[->sgx_reclaimer_write()]->sgx_encl_alloc_backing()
We need make two versions of each of those functions. It'd be especially complicated in refactoring sgx_reclaim_pages() for two versions.
I now double checked the history of current_is_ksgxd()[1], it seemed the intent was to replace "current->mm == NULL" criteria so it is more obvious we are running in ksgxd.
@Dave, @Kristen,
Can we restore the original criteria like below so it works for the cgroup work-queues?
bool current_is_ksgxd(void) { return current == ksgxd_tsk; }
--->
bool current_is_kthread(void) { return current->mm == NULL; }
I'm not experienced in this area and not sure how reliable it is to use current->mm == NULL for kthread and work-queues. But it would eliminate the need for the boolean parameter.
Would appreciate the input.
Haitao
[1]https://lore.kernel.org/linux-sgx/9c269c70-35fe-a1a4-34c9-b1e62ab3bb3b@intel.com/
| |