On 1/26/22 11:17, Kristen Carlson Accardi wrote:
> If backing pages are not able to be allocated during
> sgx_reclaim_pages(), return an error code to the caller.
> sgx_reclaim_pages() can be called from the reclaimer thread,
> or when adding pages via an ioctl. When it is called from the
ioctl() is a function name. Please add parenthesis to make that clear,
just like any other function name.
> kernel thread, it's safe to ignore the return value, however,
> calls from the ioctls should forward the error.
>
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> ---
> arch/x86/kernel/cpu/sgx/main.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index c4030fb608c6..0e95f69ebcb7 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -377,17 +377,18 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
> * problematic as it would increase the lock contention too much, which would
> * halt forward progress.
> */
> -static void sgx_reclaim_pages(void)
> +static int sgx_reclaim_pages(void)
> {
> struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> struct sgx_backing backing[SGX_NR_TO_SCAN];
> struct sgx_epc_section *section;
> struct sgx_encl_page *encl_page;
> + int pages_being_reclaimed = 0;
> struct sgx_epc_page *epc_page;
> struct sgx_numa_node *node;
> pgoff_t page_index;
> int cnt = 0;
> - int ret;
> + int ret = 0;
> int i;
>
> spin_lock(&sgx_reclaimer_lock);
> @@ -422,6 +423,8 @@ static void sgx_reclaim_pages(void)
> if (ret)
> goto skip;
>
> + pages_being_reclaimed++;
I think we can do better on the naming there. Yes, this is number of
pages which have 'SGX_ENCL_PAGE_BEING_RECLAIMED' set, but what does that
*mean*? What are the implications?
For instance, 'backing_pages_allocated' would imply that there are
future backing pages to use (or clean up).
> mutex_lock(&encl_page->encl->lock);
> encl_page->desc |= SGX_ENCL_PAGE_BEING_RECLAIMED;
> mutex_unlock(&encl_page->encl->lock);
> @@ -437,6 +440,9 @@ static void sgx_reclaim_pages(void)
> chunk[i] = NULL;
> }
>
> + if (!pages_being_reclaimed)
> + return ret;
I think this needs a comment. It will return the error from the *last*
failure of sgx_encl_get_backing(). That's fine, I guess, but it's a bit
weird because there could have been 100 errors and the first 99 errors
are ignored.
> for (i = 0; i < cnt; i++) {
> epc_page = chunk[i];
> if (epc_page)
> @@ -463,6 +469,7 @@ static void sgx_reclaim_pages(void)
> spin_unlock(&node->lock);
> atomic_long_inc(&sgx_nr_free_pages);
> }
> + return ret;
> }
Let's say cnt=2. The first run through the loop does
sgx_encl_get_backing() and increments pages_being_reclaimed. The second
run through the loop hits an error, sets ret=-ESOMETHING. The loop
terminates.
if (!pages_being_reclaimed) <-- false
return ret;
... keep running
Then, we get to the bottom of the function. One page was reclaimed.
Success! But, ret=-ESOMETHING. sgx_reclaim_pages() will return an error.
Right?
I think this is structured wrong. In the end, we want to know whether
sgx_reclaim_pages() made any progress. Let's have it return *that*.
How many pages did it successfully reclaim?
That has some really nice properties, especially if we wait until the
last possible moment to manipulate the count. Perhaps:
static int sgx_reclaim_pages(void)
{
...
int nr_pages_reclaimed = 0;
...
// The last loop:
for (i = 0; i < cnt; i++) {
epc_page = chunk[i];
if (!epc_page)
continue;
...
atomic_long_inc(&sgx_nr_free_pages);
nr_pages_reclaimed++
}
return nr_pages_reclaimed;
}
That makes it *blatantly* obvious what the function returns since the
only manipulation of 'nr_pages_reclaimed' is right next to the return.
It also makes the function resilient to any new points of failure.
Let's say that we want to check for sgx_reclaimer_block() failures.
With this patch's approach, we have to add a new check and return for
"pages_being_reclaimed", or even an entirely new counter.
With the approach I'm suggesting, it "just works".
> static bool sgx_should_reclaim(unsigned long watermark)
> @@ -636,6 +643,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
> struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> {
> struct sgx_epc_page *page;
> + int ret;
>
> for ( ; ; ) {
> page = __sgx_alloc_epc_page();
> @@ -657,7 +665,11 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> break;
> }
>
> - sgx_reclaim_pages();
> + ret = sgx_reclaim_pages();
> + if (ret) {
> + page = ERR_PTR(-ENOMEM);
> + break;
> + }
> cond_resched();
> }
So, we go to the trouble of plumbing a real error code out of
sgx_reclaim_pages(), but then throw it away here. Why?
