Re: [PATCH v40 21/24] x86/sgx: Add a page reclaimer

[Jarkko Sakkinen]

On Sun, Nov 08, 2020 at 11:56:30AM +0800, Hillf Danton wrote:
> On Wed,  4 Nov 2020 16:54:27 Jarkko Sakkinen wrote:
> [...]
> > +/**
> > + * sgx_alloc_epc_page() - Allocate an EPC page
> > + * @owner:	the owner of the EPC page
> > + * @reclaim:	reclaim pages if necessary
> > + *
> > + * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> > + * page is no longer needed it must be released with sgx_free_epc_page(). If
> > + * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> > + * mm's can be locked when @reclaim is set to true.
> > + *
> > + * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> > + * before returning back to the caller.
> > + *
> > + * Return:
> > + *   an EPC page,
> > + *   -errno on error
> > + */
> > +struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> > +{
> > +	struct sgx_epc_page *entry;
> 
> Nit: s/entry/epc_page/
> > +
> > +	for ( ; ; ) {
> > +		entry = __sgx_alloc_epc_page();
> > +		if (!IS_ERR(entry)) {
> > +			entry->owner = owner;
> > +			break;
> > +		}
> > +
> > +		if (list_empty(&sgx_active_page_list))
> > +			return ERR_PTR(-ENOMEM);
> > +
> > +		if (!reclaim) {
> > +			entry = ERR_PTR(-EBUSY);
> > +			break;
> > +		}
> > +
> > +		if (signal_pending(current)) {
> > +			entry = ERR_PTR(-ERESTARTSYS);
> > +			break;
> > +		}
> > +
> > +		sgx_reclaim_pages();
> i
> This is the direct reclaim mode with ksgxswapd that works in
> the background ignored in the entire for loop. But we can go
> with it in parallel, see below, if it tries as hard as it can
> to maitain the watermark in which allocators may have no
> interest.

I think this policy should be left at is and once the code in mainline
further refined. Consider it as a baseline/initial version for
reclaiming code.

> > +		schedule();
> 
> To cut allocator's latency use cond_resched();

Thanks, I'll change this.

> > +	}
> > +
> > +	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
> > +		wake_up(&ksgxswapd_waitq);
> 
> Nit: s/ksgxswapd/sgxd/ as it seems to have nothing to do with swap,
> given sgx itself is clear and good enough.

Yeah, it also handling kexec() situation, i.e. has multitude of
functions.

> > +
> > +	return entry;
> > +}
> 
> struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> {
> 	struct sgx_epc_page *epc_page;
> 
> 	for (;;) {
> 		epc_page = __sgx_alloc_epc_page();
> 
> 		if (!IS_ERR(epc_page)) {
> 			epc_page->owner = owner;
> 			return epc_page;
> 		}
> 
> 		if (signal_pending(current))
> 			return ERR_PTR(-ERESTARTSYS);
> 
> 		if (list_empty(&sgx_active_page_list) || !reclaim)
> 			return ERR_PTR(-ENOMEM);
> 
> 		wake_up(&ksgxswapd_waitq);
> 		cond_resched();
> 	}
> 	return ERR_PTR(-ENOMEM);
> }

/Jarkko