On Wed, Sep 25, 2019 at 11:33:03AM -0700, Sean Christopherson wrote:
> 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);
> }
If you want to make it cleaner, it would be better keep
sgx_reclaimer_age() call outside the function in order to keep the call
hierarchy kind of "flat". Aging is kind of core part of the flow works
and I don't want it to be buried too deep.
E.g. inside the sgx_reclaim_pages():
if (sgx_reclaimer_age(epc_page))
sgx_reclaimer_prepare(epc_page, &backing[i]);
sgx_reclaimer_prepare() would do then all the preparation work (also
set SGX_ENCL_PAGE_RECLAIMED).
/Jarkko
