On Fri, Nov 11, 2022 at 10:35:11AM -0800, Kristen Carlson Accardi wrote:
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
>
> Keep track of whether the EPC page is in the middle of being reclaimed
> and do not delete the page off the it's LRU if it has not yet finished
"off the it's LRU" ?
> being reclaimed.
I'm not sure how the description makes the change understandable.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/kernel/cpu/sgx/main.c | 14 +++++++++-----
> arch/x86/kernel/cpu/sgx/sgx.h | 4 ++++
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 3b09433ffd85..8c451071fa91 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -305,13 +305,15 @@ static void __sgx_reclaim_pages(void)
>
> encl_page = epc_page->encl_owner;
>
> - if (kref_get_unless_zero(&encl_page->encl->refcount) != 0)
> + if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) {
> + epc_page->flags |= SGX_EPC_PAGE_RECLAIM_IN_PROGRESS;
> chunk[cnt++] = epc_page;
> - else
> + } else {
> /* The owner is freeing the page. No need to add the
> * page back to the list of reclaimable pages.
> */
> epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> + }
> }
> spin_unlock(&sgx_global_lru.lock);
>
> @@ -337,6 +339,7 @@ static void __sgx_reclaim_pages(void)
>
> skip:
> spin_lock(&sgx_global_lru.lock);
> + epc_page->flags &= ~SGX_EPC_PAGE_RECLAIM_IN_PROGRESS;
> sgx_epc_push_reclaimable(&sgx_global_lru, epc_page);
> spin_unlock(&sgx_global_lru.lock);
>
> @@ -360,7 +363,8 @@ static void __sgx_reclaim_pages(void)
> sgx_reclaimer_write(epc_page, &backing[i]);
>
> kref_put(&encl_page->encl->refcount, sgx_encl_release);
> - epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> + epc_page->flags &= ~(SGX_EPC_PAGE_RECLAIMER_TRACKED |
> + SGX_EPC_PAGE_RECLAIM_IN_PROGRESS);
>
> sgx_free_epc_page(epc_page);
> }
> @@ -508,7 +512,7 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
> void sgx_record_epc_page(struct sgx_epc_page *page, unsigned long flags)
> {
> spin_lock(&sgx_global_lru.lock);
> - WARN_ON(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> + WARN_ON(page->flags & SGX_EPC_PAGE_RECLAIM_FLAGS);
Please, open code SGX_EPC_PAGE_RECLAIM_FLAGS. It only adds unnecassry
need to cross-reference to the header file.
Also, please describe the changes on how state flags are used before
and after this patch to the commit message.
> page->flags |= flags;
> if (flags & SGX_EPC_PAGE_RECLAIMER_TRACKED)
> sgx_epc_push_reclaimable(&sgx_global_lru, page);
> @@ -532,7 +536,7 @@ int sgx_drop_epc_page(struct sgx_epc_page *page)
> spin_lock(&sgx_global_lru.lock);
> if (page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) {
> /* The page is being reclaimed. */
> - if (list_empty(&page->list)) {
> + if (page->flags & SGX_EPC_PAGE_RECLAIM_IN_PROGRESS) {
> spin_unlock(&sgx_global_lru.lock);
> return -EBUSY;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 969606615211..04ca644928a8 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -30,6 +30,10 @@
> #define SGX_EPC_PAGE_IS_FREE BIT(1)
> /* Pages allocated for KVM guest */
> #define SGX_EPC_PAGE_KVM_GUEST BIT(2)
> +/* page flag to indicate reclaim is in progress */
> +#define SGX_EPC_PAGE_RECLAIM_IN_PROGRESS BIT(3)
> +#define SGX_EPC_PAGE_RECLAIM_FLAGS (SGX_EPC_PAGE_RECLAIMER_TRACKED | \
> + SGX_EPC_PAGE_RECLAIM_IN_PROGRESS)
>
> struct sgx_epc_page {
> unsigned int section;
> --
> 2.37.3
>
BR, Jarkko
