From: "Haitao Huang" <haitao.huang@linux.intel.com>
To: "mingo@redhat.com" <mingo@redhat.com>,
"jarkko@kernel.org" <jarkko@kernel.org>,
"x86@kernel.org" <x86@kernel.org>,
"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
"hpa@zytor.com" <hpa@zytor.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
"mkoutny@suse.com" <mkoutny@suse.com>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"tj@kernel.org" <tj@kernel.org>,
"Mehta, Sohil" <sohil.mehta@intel.com>,
"bp@alien8.de" <bp@alien8.de>, "Huang, Kai" <kai.huang@intel.com>,
"Haitao Huang" <haitao.huang@linux.intel.com>
Cc: "mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
"Christopherson,, Sean" <seanjc@google.com>,
"Zhang, Bo" <zhanb@microsoft.com>,
"kristen@linux.intel.com" <kristen@linux.intel.com>,
"anakrish@microsoft.com" <anakrish@microsoft.com>,
"sean.j.christopherson@intel.com"
<sean.j.christopherson@intel.com>,
"Li, Zhiquan1" <zhiquan1.li@intel.com>,
"yangjie@microsoft.com" <yangjie@microsoft.com>
Subject: Re: [PATCH v6 04/12] x86/sgx: Implement basic EPC misc cgroup functionality
Date: Mon, 06 Nov 2023 20:08:43 -0600 [thread overview]
Message-ID: <op.2d0n8tjtwjvjmi@hhuan26-mobl.amr.corp.intel.com> (raw)
In-Reply-To: <op.2d0ltsxxwjvjmi@hhuan26-mobl.amr.corp.intel.com>
On Mon, 06 Nov 2023 19:16:30 -0600, Haitao Huang
<haitao.huang@linux.intel.com> wrote:
> On Mon, 06 Nov 2023 16:18:30 -0600, Huang, Kai <kai.huang@intel.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).
>
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
next prev parent reply other threads:[~2023-11-07 2:10 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-30 18:20 [PATCH v6 00/12] Add Cgroup support for SGX EPC memory Haitao Huang
2023-10-30 18:20 ` [PATCH v6 01/12] cgroup/misc: Add per resource callbacks for CSS events Haitao Huang
2023-11-15 20:25 ` Jarkko Sakkinen
2024-01-09 3:37 ` Haitao Huang
2024-01-10 19:55 ` Jarkko Sakkinen
2024-01-05 9:45 ` Michal Koutný
2024-01-06 1:42 ` Haitao Huang
2023-10-30 18:20 ` [PATCH v6 02/12] cgroup/misc: Export APIs for SGX driver Haitao Huang
2023-10-30 18:20 ` [PATCH v6 03/12] cgroup/misc: Add SGX EPC resource type Haitao Huang
2023-10-30 18:20 ` [PATCH v6 04/12] x86/sgx: Implement basic EPC misc cgroup functionality Haitao Huang
2023-11-06 12:09 ` Huang, Kai
2023-11-06 18:59 ` Haitao Huang
2023-11-06 22:18 ` Huang, Kai
2023-11-07 1:16 ` Haitao Huang
2023-11-07 2:08 ` Haitao Huang [this message]
2023-11-07 19:07 ` Huang, Kai
2023-11-20 3:16 ` Huang, Kai
2023-11-26 16:01 ` Haitao Huang
2023-11-26 16:32 ` Haitao Huang
2023-11-06 22:23 ` Huang, Kai
2023-11-15 20:48 ` Jarkko Sakkinen
2023-10-30 18:20 ` [PATCH v6 05/12] x86/sgx: Add sgx_epc_lru_list to encapsulate LRU list Haitao Huang
2023-10-30 18:20 ` [PATCH v6 06/12] x86/sgx: Use sgx_epc_lru_list for existing active page list Haitao Huang
2023-10-30 18:20 ` [PATCH v6 07/12] x86/sgx: Introduce EPC page states Haitao Huang
2023-11-15 20:53 ` Jarkko Sakkinen
2024-01-05 17:57 ` Dave Hansen
2024-01-06 1:45 ` Haitao Huang
2023-10-30 18:20 ` [PATCH v6 08/12] x86/sgx: Use a list to track to-be-reclaimed pages Haitao Huang
2023-11-15 20:59 ` Jarkko Sakkinen
2023-10-30 18:20 ` [PATCH v6 09/12] x86/sgx: Restructure top-level EPC reclaim function Haitao Huang
2023-11-20 3:45 ` Huang, Kai
2023-11-26 16:27 ` Haitao Huang
2023-11-27 9:57 ` Huang, Kai
2023-12-12 4:04 ` Haitao Huang
2023-12-13 11:17 ` Huang, Kai
2023-12-15 19:49 ` Haitao Huang
2023-12-18 1:44 ` Huang, Kai
2023-12-18 17:32 ` Mikko Ylinen
2023-12-18 21:24 ` Haitao Huang
2024-01-03 16:37 ` Dave Hansen
2024-01-04 19:11 ` Haitao Huang
2024-01-04 19:19 ` Jarkko Sakkinen
2024-01-04 19:27 ` Dave Hansen
2024-01-04 21:01 ` Haitao Huang
2024-01-05 14:43 ` Mikko Ylinen
2024-01-04 12:38 ` Michal Koutný
2024-01-04 19:20 ` Haitao Huang
2024-01-12 17:07 ` Haitao Huang
2024-01-13 21:04 ` Jarkko Sakkinen
2024-01-13 21:08 ` Jarkko Sakkinen
2023-10-30 18:20 ` [PATCH v6 10/12] x86/sgx: Implement EPC reclamation for cgroup Haitao Huang
2023-11-06 15:58 ` [PATCH] x86/sgx: Charge proper mem_cgroup for usage due to EPC reclamation by cgroups Haitao Huang
2023-11-06 16:10 ` [PATCH v6 10/12] x86/sgx: Implement EPC reclamation for cgroup Haitao Huang
2023-10-30 18:20 ` [PATCH v6 11/12] Docs/x86/sgx: Add description for cgroup support Haitao Huang
2023-10-30 18:20 ` [PATCH v6 12/12] selftests/sgx: Add scripts for EPC cgroup testing Haitao Huang
2023-11-15 21:00 ` Jarkko Sakkinen
2023-11-15 21:22 ` Haitao Huang
2023-11-06 3:26 ` [PATCH v6 00/12] Add Cgroup support for SGX EPC memory Jarkko Sakkinen
2023-11-06 15:48 ` Haitao Huang
2023-11-08 1:00 ` Haitao Huang
2024-01-05 18:29 ` Dave Hansen
2024-01-05 20:13 ` Haitao Huang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=op.2d0n8tjtwjvjmi@hhuan26-mobl.amr.corp.intel.com \
--to=haitao.huang@linux.intel.com \
--cc=anakrish@microsoft.com \
--cc=bp@alien8.de \
--cc=cgroups@vger.kernel.org \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--cc=kai.huang@intel.com \
--cc=kristen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=mikko.ylinen@linux.intel.com \
--cc=mingo@redhat.com \
--cc=mkoutny@suse.com \
--cc=sean.j.christopherson@intel.com \
--cc=seanjc@google.com \
--cc=sohil.mehta@intel.com \
--cc=tglx@linutronix.de \
--cc=tj@kernel.org \
--cc=x86@kernel.org \
--cc=yangjie@microsoft.com \
--cc=zhanb@microsoft.com \
--cc=zhiquan1.li@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).