On Mon, 06 Nov 2023 19:16:30 -0600, Haitao Huang
<haitao.huang@xxxxxxxxxxxxxxx> wrote:
On Mon, 06 Nov 2023 16:18:30 -0600, Huang, Kai <kai.huang@xxxxxxxxx>
wrote:
> > +/**
> > + * sgx_epc_cgroup_try_charge() - hierarchically try to charge a
single
> > EPC page
> > + *
> > + * Returns EPC cgroup or NULL on success, -errno on failure.
> > + */
> > +struct sgx_epc_cgroup *sgx_epc_cgroup_try_charge(void)
> > +{
> > + struct sgx_epc_cgroup *epc_cg;
> > + int ret;
> > +
> > + if (sgx_epc_cgroup_disabled())
> > + return NULL;
> > +
> > + epc_cg = sgx_epc_cgroup_from_misc_cg(get_current_misc_cg());
> > + ret = misc_cg_try_charge(MISC_CG_RES_SGX_EPC, epc_cg->cg,
PAGE_SIZE);
> > +
> > + if (!ret) {
> > + /* No epc_cg returned, release ref from get_current_misc_cg() */
> > + put_misc_cg(epc_cg->cg);
> > + return ERR_PTR(-ENOMEM);
>
> misc_cg_try_charge() returns 0 when successfully charged, no?
Right. I really made some mess in rebasing :-(
>
> > + }
> > +
> > + /* Ref released in sgx_epc_cgroup_uncharge() */
> > + return epc_cg;
> > +}
>
> IMHO the above _try_charge() returning a pointer of EPC cgroup is a
> little bit
> odd, because it doesn't match the existing misc_cg_try_charge() which
> returns
> whether the charge is successful or not. sev_misc_cg_try_charge()
> matches
> misc_cg_try_charge() too.
>
> I think it's better to split "getting EPC cgroup" part out as a
separate
> helper,
> and make this _try_charge() match existing pattern:
>
> struct sgx_epc_cgroup *sgx_get_current_epc_cg(void)
> {
> if (sgx_epc_cgroup_disabled())
> return NULL;
>
> return sgx_epc_cgroup_from_misc_cg(get_current_misc_cg());
> }
>
> int sgx_epc_cgroup_try_charge(struct sgx_epc_cgroup *epc_cg)
> {
> if (!epc_cg)
> return -EINVAL;
>
> return misc_cg_try_charge(epc_cg->cg);
> }
>
> Having sgx_get_current_epc_cg() also makes the caller easier to read,
> because we
> can immediately know we are going to charge the *current* EPC cgroup,
> but not
> some cgroup hidden within sgx_epc_cgroup_try_charge().
>
Actually, unlike other misc controllers, we need charge and get the
epc_cg
reference at the same time.
Can you elaborate?
And in practice you always call sgx_epc_cgroup_try_charge() right after
sgx_get_current_epc_cg() anyway. The only difference is the whole
thing is done
in one function or in separate functions.
[...]
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.
> > 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?
>
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().
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).
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. The free count enforcement is near the
end of this method after should_reclaim() check.
Haitao
