Re: [PATCH] x86/sgx: Fix free page accounting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);


Thank you very much for taking a look.

What prevents the value from changing right after you test this?

You are correct. It is indeed possible for the value to change after this test. This test decides when to reclaim more pages so an absolute accurate value is not required but knowing that the amount of free pages are running low is.

 Why is
an atomic value somehow solving the problem?

During stress testing it was found that page allocation during hot path (for example page fault) is failing because there were no free pages available in any of the free page lists while the global counter contained a value that does not reflect the actual free pages and was higher than the watermark and thus the reclaimer is never run.

Changing it to atomic fixed this issue. I also reverted to how this counter was managed before with a spin lock protected free counter per free list and that also fixes the issue.


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);


In other words, what did you really fix here?  And how did you test it
to verify it did fix things?

To test this I created a stress test that builds on top of the new "oversubscription" test case that we are trying to add to the SGX kselftests:
https://lore.kernel.org/lkml/7715db4882ab9fd52d21de6f62bb3b7e94dc4885.1635447301.git.reinette.chatre@xxxxxxxxx/

In the changed test an enclave is created with as much heap as SGX memory. After that all the pages are accessed, their type is changed, then the enclave is entered to run EACCEPT on each page, after that the pages are removed (EREMOVE).

This test places significant stress on the SGX memory allocation and reclaim subsystems. The troublesome part of the test is when the enclave is entered so that EACCEPT can be run on each page. During this time, because of the oversubscription and previous accesses, there are many page faults. During this time a new page needs to be allocated in the SGX memory into which the page being faulted needs to be decrypted and loaded back into SGX memory. At this point the test hangs.

Below I show the following:
* perf top showing that the test hangs because user space is stuck just encountering page faults * below that I show the code traces explaining why the repeated page faults occur (because reclaimer never runs) * below that shows the perf top traces after this patch is applied showing that the reclaimer now gets a chance to run and the test can complete


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)
------------------------------------------------------------------------------------------------------------------------------------------------------------

    94.34%    68.51%  [vdso]         [.] __vdso_sgx_enter_enclave
            |
            |--68.51%--0x5541f689495641d7
            |          __libc_start_main
            |          main
            |          test_harness_run
            |          __run_test
| wrapper_enclave_unclobbered_vdso_oversubscribed_remove
            |          enclave_unclobbered_vdso_oversubscribed_remove
            |          __vdso_sgx_enter_enclave
            |          |
            |           --68.30%--asm_exc_page_fault
            |
             --7.84%--__vdso_sgx_enter_enclave
                       |
                        --2.58%--asm_exc_page_fault
                                  |
                                   --2.72%--exc_page_fault
                                             |
                                              --3.67%--do_user_addr_fault
                                                        |

--6.96%--handle_mm_fault
                                                                   |

|--4.37%--__handle_mm_fault
| | | --1.65%--__do_fault | | | --2.66%--sgx_vma_fault | | | |--1.93%--xa_load | | | | | --1.85%--xas_load | | | --1.21%--sgx_encl_load_page
                                                                   |

--1.35%--__count_memcg_events

    85.55%     0.17%  [kernel]       [k] asm_exc_page_fault
            |
             --70.81%--asm_exc_page_fault
                       |
                        --2.73%--exc_page_fault
                                  |
                                   --3.71%--do_user_addr_fault
                                             |
                                              --7.00%--handle_mm_fault
                                                        |

|--4.42%--__handle_mm_fault
                                                        |          |
| --1.65%--__do_fault | | | --2.66%--sgx_vma_fault | | | |--1.93%--xa_load | | | | | --1.85%--xas_load | | | --1.21%--sgx_encl_load_page
                                                        |

--1.35%--__count_memcg_events

    16.83%     0.18%  [kernel]       [k] exc_page_fault
            |
             --2.57%--exc_page_fault
                       |
                        --3.69%--do_user_addr_fault
                                  |
                                   --7.00%--handle_mm_fault
                                             |
                                             |--4.42%--__handle_mm_fault
                                             |          |
| --1.65%--__do_fault
                                             |                     |
| --2.66%--sgx_vma_fault | | | |--1.93%--xa_load | | | | | --1.85%--xas_load | | | --1.21%--sgx_encl_load_page
                                             |
                                              --1.35%--__count_memcg_events

exiting.


What happens above is the following flow is encountered:
sgx_vma_fault()
  sgx_encl_load_page()
    sgx_encl_eldu() //page needs to be loaded from swap
      sgx_alloc_epc_page(..., false) // new EPC page is needed

Taking a closer look at sgx_alloc_epc_page:

struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
{
	struct sgx_epc_page *page;

	for ( ; ; ) {
		page = __sgx_alloc_epc_page(); <--- page == NULL because of no free pages
		if (!IS_ERR(page)) {
			page->owner = owner;
			break;
		}

if (list_empty(&sgx_active_page_list)) <--- there are plenty of pages that can be reclaimed
			return ERR_PTR(-ENOMEM);

if (!reclaim) { <--- reclaim is false so EBUSY will be returned but an attempt is made to wake the reclaimer below
			page = ERR_PTR(-EBUSY);
			break;
		}

		if (signal_pending(current)) {
			page = ERR_PTR(-ERESTARTSYS);
			break;
		}

		sgx_reclaim_pages();
		cond_resched();
	}

if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) <-- expected to wake up reclaimer but this is not happening
		wake_up(&ksgxd_waitq);

	return page;
}

Because the above returns EBUSY sgx_vma_fault() will return VM_FAULT_NOPAGE and user space will keep attempting to access the same page and trigger the same fault again because there still are no free pages.


static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
{

        ...
        entry = sgx_encl_load_page(encl, addr);
        if (IS_ERR(entry)) {
                mutex_unlock(&encl->lock);
                if (PTR_ERR(entry) == -EBUSY)
                        return VM_FAULT_NOPAGE;
        }
        ...
}


I added some tracing and it shows that the value of sgx_nr_free_pages was higher than the watermark and thus the reclaimer does not free up pages, yet the allocation of memory keeps failing because there are no more free pages.

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)
------------------------------------------------------------------------------------------------------------------------------------------------------------

49.62% 0.18% test_sgx [.] enclave_unclobbered_vdso_oversubscribed_remove
            |
             --14.59%--enclave_unclobbered_vdso_oversubscribed_remove
                       |
                        --20.19%--__vdso_sgx_enter_enclave
                                  |
                                   --11.23%--asm_exc_page_fault
                                             |
                                              --4.82%--exc_page_fault
                                                        |

--5.04%--do_user_addr_fault
                                                                   |

--5.33%--handle_mm_fault

       |

        --4.98%--__handle_mm_fault

                  |

                   --4.59%--__do_fault

                             |

                              --5.71%--sgx_vma_fault

                                        |

                                        |--16.71%--sgx_encl_load_page

                                        |          |

                                        |           --17.05%--sgx_encl_eldu

                                        |

                                         --5.31%--__mutex_lock.isra.9

                                                   |


--4.92%--mutex_spin_on_owner

    49.46%    14.41%  [vdso]         [.] __vdso_sgx_enter_enclave
            |
            |--5.81%--__vdso_sgx_enter_enclave
            |          |
            |           --4.83%--asm_exc_page_fault
            |                     |
            |                      --4.82%--exc_page_fault
            |                                |
            |                                 --5.04%--do_user_addr_fault
            |                                           |
| --5.33%--handle_mm_fault
            |                                                      |
| --4.98%--__handle_mm_fault | | | --4.59%--__do_fault | | | --5.71%--sgx_vma_fault | | | |--16.71%--sgx_encl_load_page | | | | | --17.05%--sgx_encl_eldu | | | --5.31%--__mutex_lock.isra.9 | | | --4.92%--mutex_spin_on_owner
            |
             --2.10%--0x5541f689495641d7
                       __libc_start_main
                       main
                       test_harness_run
                       __run_test

wrapper_enclave_unclobbered_vdso_oversubscribed_remove
                       |

--8.90%--enclave_unclobbered_vdso_oversubscribed_remove
                                  |
                                   --14.39%--__vdso_sgx_enter_enclave
                                             |
                                              --6.39%--asm_exc_page_fault

    45.60%     0.05%  [kernel]       [k] ksgxd
            |
             --10.27%--ksgxd
                       |
                        --13.34%--sgx_reclaim_pages
                                  |
                                  |--19.74%--sgx_encl_ewb
                                  |          |
                                  |           --18.49%--__sgx_encl_ewb
                                  |
                                   --15.73%--__mutex_lock.isra.9
                                             |
                                              --14.77%--mutex_spin_on_owner

exiting.


Reinette



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux