Re: [PATCH v3 17/17] x86/sgx: Fix pages in the BLOCKED state ending up to the free pool

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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