Re: [PATCH v6 04/12] x86/sgx: Implement basic EPC misc cgroup functionality

From: Haitao Huang
Date: Sun Nov 26 2023 - 11:04:36 EST

On Mon, 20 Nov 2023 11:16:42 +0800, Huang, Kai <kai.huang@xxxxxxxxx> wrote:

> >
>
> That's true. I was thinking no need to have them done in separate calls.
> The caller has to check the return value for epc_cg instance first, then
> check result of try_charge. But there is really only one caller,
> sgx_alloc_epc_page() below, so I don't have strong opinions now.
>
> With them separate, the checks will look like this:
> if (epc_cg = sgx_get_current_epc_cg()) // NULL means cgroup disabled,
> should continue for allocation
> {
> if (ret = sgx_epc_cgroup_try_charge())
> return ret
> }
> // continue...
>
> I can go either way.

Let's keep this aligned with other _try_charge() variants: return 'int' to
indicate whether the charge is done or not.


Fine with me if no objections from maintainers.

>
> >
> > > > > struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> > > > > {
> > > > > struct sgx_epc_page *page;
> > > > > + struct sgx_epc_cgroup *epc_cg;
> > > > > +
> > > > > + epc_cg = sgx_epc_cgroup_try_charge();
> > > > > + if (IS_ERR(epc_cg))
> > > > > + return ERR_CAST(epc_cg);
> > > > >
> > > > > for ( ; ; ) {
> > > > > page = __sgx_alloc_epc_page();
> > > > > @@ -580,10 +587,21 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
> > > > > *owner, bool reclaim)
> > > > > break;
> > > > > }
> > > > >
> > > > > + /*
> > > > > + * Need to do a global reclamation if cgroup was not full but
> > > free
> > > > > + * physical pages run out, causing __sgx_alloc_epc_page() to
> > > fail.
> > > > > + */
> > > > > sgx_reclaim_pages();
> > > >
> > > > What's the final behaviour? IIUC it should be reclaiming from the
> > > > *current* EPC
> > > > cgroup? If so shouldn't we just pass the @epc_cg to it here?
> > > >
> > > > I think we can make this patch as "structure" patch w/o actually
> > > having
> > > > EPC
> > > > cgroup enabled, i.e., sgx_get_current_epc_cg() always return NULL.
> > > >
> > > > And we can have one patch to change sgx_reclaim_pages() to take the
> > > > 'struct
> > > > sgx_epc_lru_list *' as argument:
> > > >
> > > > void sgx_reclaim_pages_lru(struct sgx_epc_lru_list * lru)
> > > > {
> > > > ...
> > > > }
> > > >
> > > > Then here we can have something like:
> > > >
> > > > void sgx_reclaim_pages(struct sgx_epc_cg *epc_cg)
> > > > {
> > > > struct sgx_epc_lru_list *lru = epc_cg ? &epc_cg->lru :
> > > > &sgx_global_lru;
> > > >
> > > > sgx_reclaim_pages_lru(lru);
> > > > }
> > > >
> > > > Makes sense?
> > > >

The reason we 'isolate' first then do real 'reclaim' is that the actual reclaim is expensive and especially for eblock, etrack, etc.

> > >
> > > This is purely global reclamation. No cgroup involved.
> >
> > Again why? Here you are allocating one EPC page for enclave in a
> > particular EPC
> > cgroup. When that fails, shouldn't you try only to reclaim from the
> > *current*
> > EPC cgroup? Or at least you should try to reclaim from the *current*
> > EPC cgroup
> > first?
> >
>
> Later sgx_epc_cg_try_charge will take a 'reclaim' flag, if true, cgroup
> reclaims synchronously, otherwise in background and returns -EBUSY in
> that case. This function also returns if no valid epc_cg pointer
> returned.
>
> All reclamation for *current* cgroup is done in sgx_epc_cg_try_charge().

This is fine, but I believe my question above is about where to reclaim when
"allocation" fails, but not "try charge" fails.

I mean "will be done" :-) Currently no reclaim in try_charge.

And for "reclaim for current cgroup when charge fails", I don't think its even
necessary in this initial implementation of EPC cgroup. You can just fail the
allocation when charge fails (reaching the limit). Trying to reclaim when limit
is hit can be done later.


Yes. It is done later.

Please see Dave and Michal's replies here:

https://lore.kernel.org/lkml/7a1a5125-9da2-47b6-ba0f-cf24d84df16b@xxxxxxxxx/#t
https://lore.kernel.org/lkml/yz44wukoic3syy6s4fcrngagurkjhe2hzka6kvxbajdtro3fwu@zd2ilht7wcw3/

>
> So, by reaching to this point, a valid epc_cg pointer was returned,
> that means allocation is allowed for the cgroup (it has reclaimed if
> necessary, and its usage is not above limit after charging).

I found memory cgroup uses different logic -- allocation first and then charge:

For instance:

static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
{
......

folio = vma_alloc_zeroed_movable_folio(vma, vmf->address);
if (!folio)
goto oom;
if (mem_cgroup_charge(folio, vma->vm_mm, GFP_KERNEL))
goto oom_free_page;

...... }

Why EPC needs to "charge first" and "then allocate"?


EPC allocation can involve reclaiming which is more expensive than regular RAM reclamation. Also misc only has max hard limit.
Thanks
Haitao
>
> But the system level free count may be low (e.g., limits of all cgroups
> may add up to be more than capacity). so we need to do a global
> reclamation here, which may involve reclaiming a few pages (from current
> or other groups) so the system can be at a performant state with minimal
> free count. (current behavior of ksgxd).
>
I should have sticked to the orignial comment added in code. Actually
__sgx_alloc_epc_page() can fail if system runs out of EPC. That's the
really reason for global reclaim.

I don't see how this can work. With EPC cgroup likely all EPC pages will go to
the individual LRU of each cgroup, and the sgx_global_lru will basically empty.
How can you reclaim from the sgx_global_lru?

Currently, nothing in cgroup LRU, all EPCs pages are in global list.