| Message ID | 20230923030657.16148-8-haitao.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Cgroup support for SGX EPC memory | expand |
On Sat Sep 23, 2023 at 6:06 AM EEST, Haitao Huang wrote: > From: Sean Christopherson <sean.j.christopherson@intel.com> > > Add RECLAIM_IN_PROGRESS state to not rely on list_empty(&epc_page->list) > to determine if an EPC page is selected as a reclaiming candidate. > > When a page is being reclaimed from the page pool (sgx_global_lru), > there is an intermediate stage where a page may have been identified as > a candidate for reclaiming, but has not yet been reclaimed. Currently > such pages are list_del_init()'d from the global LRU list, and stored in > a an array on stack. To prevent another thread from dropping the same > page in the middle of reclaiming, sgx_drop_epc_page() checks for > list_empty(&epc_page->list). > > A later patch will replace the array on stack with a temporary list to > store the candidate pages, so list_empty() should no longer be used for > this purpose. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Co-developed-by: Kristen Carlson Accardi <kristen@linux.intel.com> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com> > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> > Cc: Sean Christopherson <seanjc@google.com> > --- > V4: > - Fixed some typos. > - Revised commit message. > > V3: > - Extend the sgx_epc_page_state enum introduced earlier to replace the > flag based approach. > --- > arch/x86/kernel/cpu/sgx/main.c | 21 ++++++++++----------- > arch/x86/kernel/cpu/sgx/sgx.h | 16 ++++++++++++++++ > 2 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index b26860399402..c1ae19a154d0 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -312,13 +312,15 @@ static void sgx_reclaim_pages(void) > list_del_init(&epc_page->list); > encl_page = epc_page->owner; > > - if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) > + if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) { > + sgx_epc_page_set_state(epc_page, 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. > */ > sgx_epc_page_reset_state(epc_page); > + } > } > spin_unlock(&sgx_global_lru.lock); > > @@ -528,16 +530,13 @@ void sgx_record_epc_page(struct sgx_epc_page *page, unsigned long flags) > int sgx_drop_epc_page(struct sgx_epc_page *page) > { > spin_lock(&sgx_global_lru.lock); > - if (sgx_epc_page_reclaimable(page->flags)) { > - /* The page is being reclaimed. */ > - if (list_empty(&page->list)) { > - spin_unlock(&sgx_global_lru.lock); > - return -EBUSY; > - } > - > - list_del(&page->list); > - sgx_epc_page_reset_state(page); > + if (sgx_epc_page_reclaim_in_progress(page->flags)) { > + spin_unlock(&sgx_global_lru.lock); > + return -EBUSY; > } > + > + list_del(&page->list); > + sgx_epc_page_reset_state(page); > spin_unlock(&sgx_global_lru.lock); > > return 0; > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index 2faeb40b345f..764cec23f4e5 100644 > --- a/arch/x86/kernel/cpu/sgx/sgx.h > +++ b/arch/x86/kernel/cpu/sgx/sgx.h > @@ -40,6 +40,8 @@ enum sgx_epc_page_state { > > /* Page is in use and tracked in a reclaimable LRU list > * Becomes NOT_TRACKED after sgx_drop_epc() > + * Becomes RECLAIM_IN_PROGRESS in sgx_reclaim_pages() when identified > + * for reclaiming > */ > SGX_EPC_PAGE_RECLAIMABLE = 2, > > @@ -50,6 +52,14 @@ enum sgx_epc_page_state { > */ > SGX_EPC_PAGE_UNRECLAIMABLE = 3, > > + /* Page is being prepared for reclamation, tracked in a temporary > + * isolated list by the reclaimer. > + * Changes in sgx_reclaim_pages() back to RECLAIMABLE if preparation > + * fails for any reason. > + * Becomes NOT_TRACKED if reclaimed successfully in sgx_reclaim_pages() > + * and immediately sgx_free_epc() is called to make it FREE. > + */ > + SGX_EPC_PAGE_RECLAIM_IN_PROGRESS = 4, > }; > > #define SGX_EPC_PAGE_STATE_MASK GENMASK(2, 0) > @@ -73,6 +83,12 @@ static inline void sgx_epc_page_set_state(struct sgx_epc_page *page, unsigned lo > page->flags |= (flags & SGX_EPC_PAGE_STATE_MASK); > } > > +static inline bool sgx_epc_page_reclaim_in_progress(unsigned long flags) > +{ > + return SGX_EPC_PAGE_RECLAIM_IN_PROGRESS == (flags & > + SGX_EPC_PAGE_STATE_MASK); > +} return SGX_EPC_PAGE_RECLAIM_IN_PROGRESS == (flags & SGX_EPC_PAGE_STATE_MASK); > + > static inline bool sgx_epc_page_reclaimable(unsigned long flags) > { > return SGX_EPC_PAGE_RECLAIMABLE == (flags & SGX_EPC_PAGE_STATE_MASK); > -- > 2.25.1 BR, Jarkko
> @@ -312,13 +312,15 @@ static void sgx_reclaim_pages(void) > list_del_init(&epc_page->list); > encl_page = epc_page->owner; > > - if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) > + if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) { > + sgx_epc_page_set_state(epc_page, 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. > */ Please use proper comment style: For single line comment: /* ... */ For multiple lines comment: /* * ... */ > sgx_epc_page_reset_state(epc_page); > + } Nit: unintended new {} around 'else'? > } > spin_unlock(&sgx_global_lru.lock); > > @@ -528,16 +530,13 @@ void sgx_record_epc_page(struct sgx_epc_page *page, unsigned long flags) > int sgx_drop_epc_page(struct sgx_epc_page *page) > { > spin_lock(&sgx_global_lru.lock); > - if (sgx_epc_page_reclaimable(page->flags)) { > - /* The page is being reclaimed. */ > - if (list_empty(&page->list)) { > - spin_unlock(&sgx_global_lru.lock); > - return -EBUSY; > - } > - > - list_del(&page->list); > - sgx_epc_page_reset_state(page); > + if (sgx_epc_page_reclaim_in_progress(page->flags)) { > + spin_unlock(&sgx_global_lru.lock); > + return -EBUSY; > } > + > + list_del(&page->list); > + sgx_epc_page_reset_state(page); > spin_unlock(&sgx_global_lru.lock); > > return 0; > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index 2faeb40b345f..764cec23f4e5 100644 > --- a/arch/x86/kernel/cpu/sgx/sgx.h > +++ b/arch/x86/kernel/cpu/sgx/sgx.h > @@ -40,6 +40,8 @@ enum sgx_epc_page_state { > > /* Page is in use and tracked in a reclaimable LRU list > * Becomes NOT_TRACKED after sgx_drop_epc() > + * Becomes RECLAIM_IN_PROGRESS in sgx_reclaim_pages() when identified > + * for reclaiming > */ > SGX_EPC_PAGE_RECLAIMABLE = 2, > > @@ -50,6 +52,14 @@ enum sgx_epc_page_state { > */ > SGX_EPC_PAGE_UNRECLAIMABLE = 3, > > + /* Page is being prepared for reclamation, tracked in a temporary > + * isolated list by the reclaimer. > + * Changes in sgx_reclaim_pages() back to RECLAIMABLE if preparation > + * fails for any reason. > + * Becomes NOT_TRACKED if reclaimed successfully in sgx_reclaim_pages() > + * and immediately sgx_free_epc() is called to make it FREE. > + */ Ditto for the comment style. And please use blank line to separate paragraphs for better readability.
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index b26860399402..c1ae19a154d0 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -312,13 +312,15 @@ static void sgx_reclaim_pages(void) list_del_init(&epc_page->list); encl_page = epc_page->owner; - if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) + if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) { + sgx_epc_page_set_state(epc_page, 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. */ sgx_epc_page_reset_state(epc_page); + } } spin_unlock(&sgx_global_lru.lock); @@ -528,16 +530,13 @@ void sgx_record_epc_page(struct sgx_epc_page *page, unsigned long flags) int sgx_drop_epc_page(struct sgx_epc_page *page) { spin_lock(&sgx_global_lru.lock); - if (sgx_epc_page_reclaimable(page->flags)) { - /* The page is being reclaimed. */ - if (list_empty(&page->list)) { - spin_unlock(&sgx_global_lru.lock); - return -EBUSY; - } - - list_del(&page->list); - sgx_epc_page_reset_state(page); + if (sgx_epc_page_reclaim_in_progress(page->flags)) { + spin_unlock(&sgx_global_lru.lock); + return -EBUSY; } + + list_del(&page->list); + sgx_epc_page_reset_state(page); spin_unlock(&sgx_global_lru.lock); return 0; diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index 2faeb40b345f..764cec23f4e5 100644 --- a/arch/x86/kernel/cpu/sgx/sgx.h +++ b/arch/x86/kernel/cpu/sgx/sgx.h @@ -40,6 +40,8 @@ enum sgx_epc_page_state { /* Page is in use and tracked in a reclaimable LRU list * Becomes NOT_TRACKED after sgx_drop_epc() + * Becomes RECLAIM_IN_PROGRESS in sgx_reclaim_pages() when identified + * for reclaiming */ SGX_EPC_PAGE_RECLAIMABLE = 2, @@ -50,6 +52,14 @@ enum sgx_epc_page_state { */ SGX_EPC_PAGE_UNRECLAIMABLE = 3, + /* Page is being prepared for reclamation, tracked in a temporary + * isolated list by the reclaimer. + * Changes in sgx_reclaim_pages() back to RECLAIMABLE if preparation + * fails for any reason. + * Becomes NOT_TRACKED if reclaimed successfully in sgx_reclaim_pages() + * and immediately sgx_free_epc() is called to make it FREE. + */ + SGX_EPC_PAGE_RECLAIM_IN_PROGRESS = 4, }; #define SGX_EPC_PAGE_STATE_MASK GENMASK(2, 0) @@ -73,6 +83,12 @@ static inline void sgx_epc_page_set_state(struct sgx_epc_page *page, unsigned lo page->flags |= (flags & SGX_EPC_PAGE_STATE_MASK); } +static inline bool sgx_epc_page_reclaim_in_progress(unsigned long flags) +{ + return SGX_EPC_PAGE_RECLAIM_IN_PROGRESS == (flags & + SGX_EPC_PAGE_STATE_MASK); +} + static inline bool sgx_epc_page_reclaimable(unsigned long flags) { return SGX_EPC_PAGE_RECLAIMABLE == (flags & SGX_EPC_PAGE_STATE_MASK);