Re: [PATCH for_v23 v3 09/12] x86/sgx: Split second half of sgx_free_page() to a separate helper

[Sean Christopherson <sean.j.christopherson@xxxxxxxxx>]

On Fri, Oct 18, 2019 at 01:06:55PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 11:37:42AM -0700, Sean Christopherson wrote:
> > Move the post-reclaim half of sgx_free_page() to a standalone helper so
> > that it can be used in flows where the page is known to be
> > non-reclaimable.
> 
> The call sites wher it is known to be reclaimable should handle the
> error instead of creating call site specific versions of the function.

What if we completely split the function(s)?  The existing callers of
sgx_free_page() stay as is, there is one and only one "free_page()", we
don't take sgx_active_page_list_lock in most flows, and the one case
where failure is acceptable gets to do its thing.  I think this'd make
both of us happy.  E.g.:

/**
 * sgx_unmark_page_reclaimable() - Unmark a page as reclaimable
 * @page:	EPC page
 *
 * Clear the reclaimable flag from the page and remove the page from the active
 * page list.
 *
 * Return:
 *   0 on success,
 *   -EBUSY if a reclaim is in progress
 */
int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
{
	/*
	 * Remove the page from the active list if necessary.  If the page
	 * is actively being reclaimed, i.e. RECLAIMABLE is set but the
	 * page isn't on the active list, return -EBUSY as we can't free
	 * the page at this time since it is "owned" by the reclaimer.
	 */
	spin_lock(&sgx_active_page_list_lock);
	if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) {
		if (list_empty(&page->list)) {
			spin_unlock(&sgx_active_page_list_lock);
			return -EBUSY;
		}
		list_del(&page->list);
		page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
	}
	spin_unlock(&sgx_active_page_list_lock);

	return 0;
}

/**
 * sgx_free_page() - Free an EPC page
 * @page:	pointer a previously allocated EPC page
 *
 * EREMOVE an EPC page and insert it back to the list of free pages. The page
 * must not be reclaimable.
 */
void sgx_free_page(struct sgx_epc_page *page)
{
	struct sgx_epc_section *section;
	int ret;

	/*
	 * Don't take sgx_active_page_list_lock when asserting the page isn't
	 * reclaimable, missing a WARN in the very rare case is preferable to
	 * unnecessarily taking a global lock in the common case.
	 */
	WARN_ON_ONCE(page->desc & SGX_EPC_PAGE_RECLAIMABLE);

	ret = __eremove(sgx_epc_addr(page));
	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
		return;

	section = sgx_epc_section(page);

	spin_lock(&section->lock);
	list_add_tail(&page->list, &section->page_list);
	section->free_cnt++;
	spin_unlock(&section->lock);
}