Re: [PATCH] x86/sgx: Fix free page accounting
From: Reinette Chatre
Date: Mon Nov 08 2021 - 14:20:09 EST
Hi Greg,
On 11/5/2021 12:10 AM, Greg KH wrote:
On Thu, Nov 04, 2021 at 01:57:31PM -0700, Reinette Chatre wrote:
Hi Greg,
On 11/4/2021 11:54 AM, Greg KH wrote:
On Thu, Nov 04, 2021 at 11:28:54AM -0700, Reinette Chatre wrote:
The SGX driver maintains a single global free page counter,
sgx_nr_free_pages, that reflects the number of free pages available
across all NUMA nodes. Correspondingly, a list of free pages is
associated with each NUMA node and sgx_nr_free_pages is updated
every time a page is added or removed from any of the free page
lists. The main usage of sgx_nr_free_pages is by the reclaimer
that will run when the total free pages go below a watermark to
ensure that there are always some free pages available to, for
example, support efficient page faults.
With sgx_nr_free_pages accessed and modified from a few places
it is essential to ensure that these accesses are done safely but
this is not the case. sgx_nr_free_pages is sometimes accessed
without any protection and when it is protected it is done
inconsistently with any one of the spin locks associated with the
individual NUMA nodes.
The consequence of sgx_nr_free_pages not being protected is that
its value may not accurately reflect the actual number of free
pages on the system, impacting the availability of free pages in
support of many flows. The problematic scenario is when the
reclaimer never runs because it believes there to be sufficient
free pages while any attempt to allocate a page fails because there
are no free pages available. The worst scenario observed was a
user space hang because of repeated page faults caused by
no free pages ever made available.
Change the global free page counter to an atomic type that
ensures simultaneous updates are done safely. While doing so, move
the updating of the variable outside of the spin lock critical
section to which it does not belong.
Cc: stable@xxxxxxxxxxxxxxx
Fixes: 901ddbb9ecf5 ("x86/sgx: Add a basic NUMA allocation scheme to sgx_alloc_epc_page()")
Suggested-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
---
arch/x86/kernel/cpu/sgx/main.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 63d3de02bbcc..8558d7d5f3e7 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -28,8 +28,7 @@ static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
static LIST_HEAD(sgx_active_page_list);
static DEFINE_SPINLOCK(sgx_reclaimer_lock);
-/* The free page list lock protected variables prepend the lock. */
-static unsigned long sgx_nr_free_pages;
+atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
/* Nodes with one or more EPC sections. */
static nodemask_t sgx_numa_mask;
@@ -403,14 +402,15 @@ static void sgx_reclaim_pages(void)
spin_lock(&node->lock);
list_add_tail(&epc_page->list, &node->free_page_list);
- sgx_nr_free_pages++;
spin_unlock(&node->lock);
+ atomic_long_inc(&sgx_nr_free_pages);
}
}
static bool sgx_should_reclaim(unsigned long watermark)
{
- return sgx_nr_free_pages < watermark && !list_empty(&sgx_active_page_list);
+ return atomic_long_read(&sgx_nr_free_pages) < watermark &&
+ !list_empty(&sgx_active_page_list);
...
The value changes were happening safely, it was just the reading of the
value that was not. You have not changed the fact that the value can
change right after reading given that there was not going to be a
problem with reading a stale value before.
I am testing on a two socket system and I am seeing that the value of
sgx_nr_free_pages does not accurately reflect the actual free pages.
It does not look to me like the value is updated safely as it is updated
with inconsistent protection on a system like this. There is a spin lock
associated with each NUMA node and which lock is used to update the variable
depends on which NUMA node memory is being modified - it is not always
protected with the same lock:
spin_lock(&node->lock);
sgx_nr_free_pages++;
spin_unlock(&node->lock);
Ah, I missed that the original code was using a different lock for every
call place, while now you are just using a single lock (the atomic value
itself.) That makes more sense, sorry for the noise.
But isn't this going to cause more thrashing and slow things down as you
are hitting the "global" lock for this variable for every allocation?
Or is this not in the hot path?
I do see this as being in the hot path as it is in the page fault
handling flow. A global lock does seem to be required since it is a
single free page count that directs the reclaimer and that counter needs
to be updated safely.
I obtained access to another two socket system where I can test this
issue. Since this system also has two NUMA nodes it updates the global
counter unsafely but on this system I am not encountering the user space
hang and can thus test how much things are being slowed down by this fix.
Interesting is that without the fix the test is actually slightly
_slower_ than with the fix. I am speculating now that the issue is
indeed also encountered on this system also but not noticed because the
global counter can correct itself after some time and not get stuck as
on the other system from which I sent the long traces.
Here are four runs without the fix:
real 0m47.024s 0m47.433s 0m47.547s 0m47.569s
user 0m7.204s 0m7.292s 0m7.265s 0m7.388s
sys 0m39.806s 0m40.129s 0m40.271s 0m40.168s
Here are four runs with the fix:
real 0m46.893s 0m47.328s 0m46.786s 0m46.635s
user 0m7.351s 0m7.252s 0m7.130s 0m7.170s
sys 0m39.528s 0m40.063s 0m39.642s 0m39.452s
Here is the perf top trace before this patch is applied showing how user
space is stuck hitting page faults over and over:
PerfTop: 4569 irqs/sec kernel:25.0% exact: 100.0% lost: 0/0 drop:
0/0 [4000Hz cycles], (all, 224 CPUs)
<ascii art that line-wrapped snipped>
Sorry about that.
With this patch the test is able to complete and the tracing shows that the
reclaimer is getting a chance to run. Previously the system was spending
almost all its time in page faults but now the time is split between the
page faults and the reclaimer.
PerfTop: 7432 irqs/sec kernel:81.5% exact: 100.0% lost: 0/0 drop:
0/0 [4000Hz cycles], (all, 224 CPUs)
Ok, that's better, you need the reclaim in order to make forward
progress.
Thanks for the detailed explaination, no objection from me, sorry for
the misunderstanding.
Thank you very much for taking a look. It is much appreciated.
Reinette