Message-ID: <op.2d0ltsxxwjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date:   Mon, 06 Nov 2023 19:16:30 -0600
From:   "Haitao Huang" <haitao.huang@...ux.intel.com>
To:     "mingo@...hat.com" <mingo@...hat.com>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "mkoutny@...e.com" <mkoutny@...e.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "tj@...nel.org" <tj@...nel.org>,
        "Mehta, Sohil" <sohil.mehta@...el.com>,
        "bp@...en8.de" <bp@...en8.de>, "Huang, Kai" <kai.huang@...el.com>
Cc:     "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Zhang, Bo" <zhanb@...rosoft.com>,
        "kristen@...ux.intel.com" <kristen@...ux.intel.com>,
        "anakrish@...rosoft.com" <anakrish@...rosoft.com>,
        "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
        "Li, Zhiquan1" <zhiquan1.li@...el.com>,
        "yangjie@...rosoft.com" <yangjie@...rosoft.com>
Subject: Re: [PATCH v6 04/12] x86/sgx: Implement basic EPC misc cgroup
 functionality

On Mon, 06 Nov 2023 16:18:30 -0600, Huang, Kai <kai.huang@...el.com> 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).

Note this patch does not do per-cgroup reclamation. If we had stopped with  
this patch without next patches, cgroups will only block allocation by  
returning invalid epc_cg pointer (-ENOMEM) from sgx_epc_cg_try_charge().  
Reclamation only happens when cgroup is not full but system level free  
count is below threshold.

>> You can see it
>> later in changes in patch 10/12. For now I just make a comment there but
>> no real changes. Cgroup reclamation will be done as part of _try_charge
>> call.
>>
>> > >  		cond_resched();
>> > >  	}
>> > >
>> > > +	if (!IS_ERR(page)) {
>> > > +		WARN_ON_ONCE(page->epc_cg);
>> > > +		page->epc_cg = epc_cg;
>> > > +	} else {
>> > > +		sgx_epc_cgroup_uncharge(epc_cg);
>> > > +	}
>> > > +
>> > >  	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
>> > >  		wake_up(&ksgxd_waitq);
>> > >
>> > > @@ -604,6 +622,11 @@ void sgx_free_epc_page(struct sgx_epc_page  
>> *page)
>> > >  	struct sgx_epc_section *section =  
>> &sgx_epc_sections[page->section];
>> > >  	struct sgx_numa_node *node = section->node;
>> > >
>> > > +	if (page->epc_cg) {
>> > > +		sgx_epc_cgroup_uncharge(page->epc_cg);
>> > > +		page->epc_cg = NULL;
>> > > +	}
>> > > +
>> > >  	spin_lock(&node->lock);
>> > >
>> > >  	page->owner = NULL;
>> > > @@ -643,6 +666,7 @@ static bool __init sgx_setup_epc_section(u64
>> > > phys_addr, u64 size,
>> > >  		section->pages[i].flags = 0;
>> > >  		section->pages[i].owner = NULL;
>> > >  		section->pages[i].poison = 0;
>> > > +		section->pages[i].epc_cg = NULL;
>> > >  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>> > >  	}
>> > >
>> > > @@ -787,6 +811,7 @@ static void __init  
>> arch_update_sysfs_visibility(int
>> > > nid) {}
>> > >  static bool __init sgx_page_cache_init(void)
>> > >  {
>> > >  	u32 eax, ebx, ecx, edx, type;
>> > > +	u64 capacity = 0;
>> > >  	u64 pa, size;
>> > >  	int nid;
>> > >  	int i;
>> > > @@ -837,6 +862,7 @@ static bool __init sgx_page_cache_init(void)
>> > >
>> > >  		sgx_epc_sections[i].node =  &sgx_numa_nodes[nid];
>> > >  		sgx_numa_nodes[nid].size += size;
>> > > +		capacity += size;
>> > >
>> > >  		sgx_nr_epc_sections++;
>> > >  	}
>> > > @@ -846,6 +872,8 @@ static bool __init sgx_page_cache_init(void)
>> > >  		return false;
>> > >  	}
>> > >
>> > > +	misc_cg_set_capacity(MISC_CG_RES_SGX_EPC, capacity);
>
> Hmm.. I think this is why MISC_CG_RES_SGX_EPC is needed when
> !CONFIG_CGROUP_SGX_EPC.

right, that was it :-)

>
>> > > +
>> > >  	return true;
>> > >  }
>> >
>> > I would separate setting up capacity as a separate patch.
>>
>> I thought about that, but again it was only 3-4 lines all in this  
>> function
>> and it's also necessary part of basic setup for misc controller...
>
> Fine.  Anyway it depends on what things you want to do on this patch.  
> It's fine
> to include the capacity if this patch is some "structure" patch that  
> shows the
> high level flow of how EPC cgroup works.

Ok, I'll keep this way for now unless any objections.

Thanks
Haitao

Powered by blists - more mailing lists