From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org,
Shay Katz-zamir <shay.katz-zamir@intel.com>,
Serge Ayoun <serge.ayoun@intel.com>
Subject: Re: [PATCH v3 17/17] x86/sgx: Fix pages in the BLOCKED state ending up to the free pool
Date: Wed, 25 Sep 2019 11:33:03 -0700 [thread overview]
Message-ID: <20190925183303.GI31852@linux.intel.com> (raw)
In-Reply-To: <20190925002749.GA29629@linux.intel.com>
On Wed, Sep 25, 2019 at 03:27:49AM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 18, 2019 at 07:21:20AM +0300, Jarkko Sakkinen wrote:
> > > > + goto skip;
> > > >
> > > > + ret = sgx_encl_get_backing(encl_page->encl,
> > > > + SGX_ENCL_PAGE_INDEX(encl_page),
> > > > + &backing[i]);
> > > > + if (ret)
> > > > + goto skip;
> > > > +
> > > > + mutex_lock(&encl_page->encl->lock);
> > > > + encl_page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> > > > + mutex_unlock(&encl_page->encl->lock);
> > > > + continue;
> > > > +
> > > > +skip:
> > >
> > > Eww. The call to sgx_encl_get_backing() makes it rather ugly no matter
> > > what, but this seems slightly less ugly:
> > >
> > > for (i = 0; i < cnt; i++) {
> > > epc_page = chunk[i];
> > > encl_page = epc_page->owner;
> > >
> > > if (!sgx_can_reclaim(chunk[i]) ||
> > > sgx_encl_get_backing(encl_page->encl,
> > > SGX_ENCL_PAGE_INDEX(encl_page),
> > > &backing[i]) {
> > > kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > >
> > > spin_lock(&sgx_active_page_list_lock);
> > > list_add_tail(&epc_page->list, &sgx_active_page_list);
> > > spin_unlock(&sgx_active_page_list_lock);
> > >
> > > chunk[i] = NULL;
> > > continue;
> > > }
> > >
> > > mutex_lock(&encl_page->encl->lock);
> > > encl_page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> > > mutex_unlock(&encl_page->encl->lock);
> > > }
> > >
>
> Well that is one big nested mess where as the version I did is legit use
> of gotos: two conditions that can cause to skip the action. And also
> fairly normal use of gotos with same ideas as with out/err etc. labels
> except now it is used inside a loop..
Yeah, it's the "inside a loop" part that's ugly. I agree the nested code
is also heinous.
What if we add a helper to split out the verbose check? Maybe as below,
or move the entire guts to a separate helper?
static int sgx_prepare_to_reclaim(struct sgx_encl_page *encl_page,
struct sgx_backing *backing)
{
if (!sgx_can_reclaim(encl_page))
return -EBUSY;
return sgx_encl_get_backing(encl_page->encl,
SGX_ENCL_PAGE_INDEX(encl_page), backing);
}
void sgx_reclaim_pages(void)
{
for (i = 0; i < cnt; i++) {
epc_page = chunk[i];
encl_page = epc_page->owner;
if (!sgx_prepare_to_reclaim(encl_page, &backing[i])) {
mutex_lock(&encl_page->encl->lock);
encl_page->desc |= SGX_ENCL_PAGE_RECLAIMED;
mutex_unlock(&encl_page->encl->lock);
else {
kref_put(&encl_page->encl->refcount, sgx_encl_release);
spin_lock(&sgx_active_page_list_lock);
list_add_tail(&epc_page->list, &sgx_active_page_list);
spin_unlock(&sgx_active_page_list_lock);
chunk[i] = NULL;
}
}
next prev parent reply other threads:[~2019-09-25 18:33 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-16 10:17 [PATCH v3 00/17] Fixes and updates for v23 Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 01/17] selftest/x86/sgx: Remove encl_piggy.h Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 02/17] x86/sgx: Clean up internal includes Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 03/17] x86/sgx: Write backing storage only if EWB is successful Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 04/17] x86/sgx: Rename 'j' as 'cnt' in sgx_reclaim_pages() Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 05/17] x86/sgx: Turn encls_failed() as inline function Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 06/17] x86/sgx: Move sgx_einit() to encls.c Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 07/17] x86/sgx: Remove pages in sgx_reclaimer_write() Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 08/17] x86/sgx: Calculate page index " Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 09/17] x86/sgx: Move SGX_ENCL_DEAD check to sgx_reclaimer_write() Jarkko Sakkinen
2019-09-17 23:13 ` Sean Christopherson
2019-09-18 4:15 ` Jarkko Sakkinen
2019-09-17 23:21 ` Sean Christopherson
2019-09-18 4:16 ` Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 10/17] x86/sgx: Free VA slot when the EWB flow fails Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 11/17] x86/sgx: Call sgx_encl_destroy() " Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 12/17] x86/sgx: Open code sgx_reclaimer_get() and sgx_reclaimer_put() Jarkko Sakkinen
2019-09-17 23:07 ` Sean Christopherson
2019-09-18 4:12 ` Jarkko Sakkinen
2019-09-20 13:38 ` Jarkko Sakkinen
2019-09-16 10:17 ` [PATCH v3 13/17] x86/sgx: Introduce sgx_can_reclaim() Jarkko Sakkinen
2019-09-17 23:25 ` Sean Christopherson
2019-09-25 18:28 ` Sean Christopherson
2019-09-27 15:33 ` Jarkko Sakkinen
2019-09-16 10:18 ` [PATCH v3 14/17] x86/sgx: Replace section->free_cnt with a global sgx_nr_free_pages Jarkko Sakkinen
2019-09-17 22:50 ` Sean Christopherson
2019-09-18 4:07 ` Jarkko Sakkinen
2019-09-16 10:18 ` [PATCH v3 15/17] x86/sgx: sgx_vma_access(): Do not return -ECANCELED on invalid TCS pages Jarkko Sakkinen
2019-09-16 10:18 ` [PATCH v3 16/17] x86/sgx: Introduce sgx_encl_get_backing() Jarkko Sakkinen
2019-09-17 23:05 ` Sean Christopherson
2019-09-18 4:10 ` Jarkko Sakkinen
2019-09-16 10:18 ` [PATCH v3 17/17] x86/sgx: Fix pages in the BLOCKED state ending up to the free pool Jarkko Sakkinen
2019-09-17 23:34 ` Sean Christopherson
2019-09-18 4:21 ` Jarkko Sakkinen
2019-09-25 0:27 ` Jarkko Sakkinen
2019-09-25 18:33 ` Sean Christopherson [this message]
2019-09-27 15:39 ` Jarkko Sakkinen
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=20190925183303.GI31852@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=linux-sgx@vger.kernel.org \
--cc=serge.ayoun@intel.com \
--cc=shay.katz-zamir@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).