From: Sean Christopherson <sean.j.christopherson@intel.com>
Wrap the existing reclaimable list and its spinlock in a struct to
minimize the code changes needed to handle multiple LRUs as well as
reclaimable and non-reclaimable lists, both of which will be introduced
and used by SGX EPC cgroups.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Cc: Sean Christopherson <seanjc@google.com>
---
arch/x86/kernel/cpu/sgx/main.c | 37 +++++++++++++++++-----------------
arch/x86/kernel/cpu/sgx/sgx.h | 11 ++++++++++
2 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 4cdeb915dc86..af68dc1c677b 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -26,10 +26,9 @@ static DEFINE_XARRAY(sgx_epc_address_space);
/*
* These variables are part of the state of the reclaimer, and must be accessed
- * with sgx_reclaimer_lock acquired.
+ * with sgx_global_lru.lock acquired.
*/
-static LIST_HEAD(sgx_active_page_list);
-static DEFINE_SPINLOCK(sgx_reclaimer_lock);
+static struct sgx_epc_lru sgx_global_lru;
static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
@@ -298,12 +297,12 @@ static void sgx_reclaim_pages(void)
int ret;
int i;
- spin_lock(&sgx_reclaimer_lock);
+ spin_lock(&sgx_global_lru.lock);
for (i = 0; i < SGX_NR_TO_SCAN; i++) {
- if (list_empty(&sgx_active_page_list))
+ if (list_empty(&sgx_global_lru.reclaimable))
break;
- epc_page = list_first_entry(&sgx_active_page_list,
+ epc_page = list_first_entry(&sgx_global_lru.reclaimable,
struct sgx_epc_page, list);
list_del_init(&epc_page->list);
encl_page = epc_page->owner;
@@ -316,7 +315,7 @@ static void sgx_reclaim_pages(void)
*/
epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
}
- spin_unlock(&sgx_reclaimer_lock);
+ spin_unlock(&sgx_global_lru.lock);
for (i = 0; i < cnt; i++) {
epc_page = chunk[i];
@@ -339,9 +338,9 @@ static void sgx_reclaim_pages(void)
continue;
skip:
- spin_lock(&sgx_reclaimer_lock);
- list_add_tail(&epc_page->list, &sgx_active_page_list);
- spin_unlock(&sgx_reclaimer_lock);
+ spin_lock(&sgx_global_lru.lock);
+ list_add_tail(&epc_page->list, &sgx_global_lru.reclaimable);
+ spin_unlock(&sgx_global_lru.lock);
kref_put(&encl_page->encl->refcount, sgx_encl_release);
@@ -374,7 +373,7 @@ static void sgx_reclaim_pages(void)
static bool sgx_should_reclaim(unsigned long watermark)
{
return atomic_long_read(&sgx_nr_free_pages) < watermark &&
- !list_empty(&sgx_active_page_list);
+ !list_empty(&sgx_global_lru.reclaimable);
}
/*
@@ -427,6 +426,8 @@ static bool __init sgx_page_reclaimer_init(void)
ksgxd_tsk = tsk;
+ sgx_lru_init(&sgx_global_lru);
+
return true;
}
@@ -502,10 +503,10 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
*/
void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
{
- spin_lock(&sgx_reclaimer_lock);
+ spin_lock(&sgx_global_lru.lock);
page->flags |= SGX_EPC_PAGE_RECLAIMER_TRACKED;
- list_add_tail(&page->list, &sgx_active_page_list);
- spin_unlock(&sgx_reclaimer_lock);
+ list_add_tail(&page->list, &sgx_global_lru.reclaimable);
+ spin_unlock(&sgx_global_lru.lock);
}
/**
@@ -520,18 +521,18 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
*/
int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
{
- spin_lock(&sgx_reclaimer_lock);
+ spin_lock(&sgx_global_lru.lock);
if (page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) {
/* The page is being reclaimed. */
if (list_empty(&page->list)) {
- spin_unlock(&sgx_reclaimer_lock);
+ spin_unlock(&sgx_global_lru.lock);
return -EBUSY;
}
list_del(&page->list);
page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
}
- spin_unlock(&sgx_reclaimer_lock);
+ spin_unlock(&sgx_global_lru.lock);
return 0;
}
@@ -564,7 +565,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
break;
}
- if (list_empty(&sgx_active_page_list))
+ if (list_empty(&sgx_global_lru.reclaimable))
return ERR_PTR(-ENOMEM);
if (!reclaim) {
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 5a7e858a8f98..7b208ee8eb45 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -83,6 +83,17 @@ static inline void *sgx_get_epc_virt_addr(struct sgx_epc_page *page)
return section->virt_addr + index * PAGE_SIZE;
}
+struct sgx_epc_lru {
+ spinlock_t lock;
+ struct list_head reclaimable;
+};
+
+static inline void sgx_lru_init(struct sgx_epc_lru *lru)
+{
+ spin_lock_init(&lru->lock);
+ INIT_LIST_HEAD(&lru->reclaimable);
+}
+
struct sgx_epc_page *__sgx_alloc_epc_page(void);
void sgx_free_epc_page(struct sgx_epc_page *page);
--
2.37.3On Thu, Sep 22, 2022 at 10:10:41AM -0700, Kristen Carlson Accardi wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
>
> Wrap the existing reclaimable list and its spinlock in a struct to
> minimize the code changes needed to handle multiple LRUs as well as
> reclaimable and non-reclaimable lists, both of which will be introduced
> and used by SGX EPC cgroups.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Cc: Sean Christopherson <seanjc@google.com>
The commit message could explicitly state the added data type.
The data type is not LRU: together with the LIFO list, i.e.
a queue, the code implements LRU alike policy.
I would name the data type as sgx_epc_queue because it is a
less confusing name.
> ---
> arch/x86/kernel/cpu/sgx/main.c | 37 +++++++++++++++++-----------------
> arch/x86/kernel/cpu/sgx/sgx.h | 11 ++++++++++
> 2 files changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 4cdeb915dc86..af68dc1c677b 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -26,10 +26,9 @@ static DEFINE_XARRAY(sgx_epc_address_space);
>
> /*
> * These variables are part of the state of the reclaimer, and must be accessed
> - * with sgx_reclaimer_lock acquired.
> + * with sgx_global_lru.lock acquired.
> */
> -static LIST_HEAD(sgx_active_page_list);
> -static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> +static struct sgx_epc_lru sgx_global_lru;
>
> static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
>
> @@ -298,12 +297,12 @@ static void sgx_reclaim_pages(void)
> int ret;
> int i;
>
> - spin_lock(&sgx_reclaimer_lock);
> + spin_lock(&sgx_global_lru.lock);
> for (i = 0; i < SGX_NR_TO_SCAN; i++) {
> - if (list_empty(&sgx_active_page_list))
> + if (list_empty(&sgx_global_lru.reclaimable))
> break;
>
> - epc_page = list_first_entry(&sgx_active_page_list,
> + epc_page = list_first_entry(&sgx_global_lru.reclaimable,
> struct sgx_epc_page, list);
> list_del_init(&epc_page->list);
> encl_page = epc_page->owner;
> @@ -316,7 +315,7 @@ static void sgx_reclaim_pages(void)
> */
> epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> }
> - spin_unlock(&sgx_reclaimer_lock);
> + spin_unlock(&sgx_global_lru.lock);
>
> for (i = 0; i < cnt; i++) {
> epc_page = chunk[i];
> @@ -339,9 +338,9 @@ static void sgx_reclaim_pages(void)
> continue;
>
> skip:
> - spin_lock(&sgx_reclaimer_lock);
> - list_add_tail(&epc_page->list, &sgx_active_page_list);
> - spin_unlock(&sgx_reclaimer_lock);
> + spin_lock(&sgx_global_lru.lock);
> + list_add_tail(&epc_page->list, &sgx_global_lru.reclaimable);
> + spin_unlock(&sgx_global_lru.lock);
>
> kref_put(&encl_page->encl->refcount, sgx_encl_release);
>
> @@ -374,7 +373,7 @@ static void sgx_reclaim_pages(void)
> static bool sgx_should_reclaim(unsigned long watermark)
> {
> return atomic_long_read(&sgx_nr_free_pages) < watermark &&
> - !list_empty(&sgx_active_page_list);
> + !list_empty(&sgx_global_lru.reclaimable);
> }
>
> /*
> @@ -427,6 +426,8 @@ static bool __init sgx_page_reclaimer_init(void)
>
> ksgxd_tsk = tsk;
>
> + sgx_lru_init(&sgx_global_lru);
> +
> return true;
> }
>
> @@ -502,10 +503,10 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
> */
> void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
> {
> - spin_lock(&sgx_reclaimer_lock);
> + spin_lock(&sgx_global_lru.lock);
> page->flags |= SGX_EPC_PAGE_RECLAIMER_TRACKED;
> - list_add_tail(&page->list, &sgx_active_page_list);
> - spin_unlock(&sgx_reclaimer_lock);
> + list_add_tail(&page->list, &sgx_global_lru.reclaimable);
> + spin_unlock(&sgx_global_lru.lock);
> }
>
> /**
> @@ -520,18 +521,18 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
> */
> int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
> {
> - spin_lock(&sgx_reclaimer_lock);
> + spin_lock(&sgx_global_lru.lock);
> if (page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) {
> /* The page is being reclaimed. */
> if (list_empty(&page->list)) {
> - spin_unlock(&sgx_reclaimer_lock);
> + spin_unlock(&sgx_global_lru.lock);
> return -EBUSY;
> }
>
> list_del(&page->list);
> page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> }
> - spin_unlock(&sgx_reclaimer_lock);
> + spin_unlock(&sgx_global_lru.lock);
>
> return 0;
> }
> @@ -564,7 +565,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> break;
> }
>
> - if (list_empty(&sgx_active_page_list))
> + if (list_empty(&sgx_global_lru.reclaimable))
> return ERR_PTR(-ENOMEM);
>
> if (!reclaim) {
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 5a7e858a8f98..7b208ee8eb45 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -83,6 +83,17 @@ static inline void *sgx_get_epc_virt_addr(struct sgx_epc_page *page)
> return section->virt_addr + index * PAGE_SIZE;
> }
>
> +struct sgx_epc_lru {
> + spinlock_t lock;
> + struct list_head reclaimable;
s/reclaimable/list/
> +};
> +
> +static inline void sgx_lru_init(struct sgx_epc_lru *lru)
> +{
> + spin_lock_init(&lru->lock);
> + INIT_LIST_HEAD(&lru->reclaimable);
> +}
> +
> struct sgx_epc_page *__sgx_alloc_epc_page(void);
> void sgx_free_epc_page(struct sgx_epc_page *page);
>
> --
> 2.37.3
>
Please also add these:
/*
* Must be called with queue->lock acquired.
*/
static inline struct sgx_epc_page *__sgx_epc_queue_push(struct sgx_epc_queue *queue,
struct sgx_page *page)
{
list_add_tail(&page->list, &queue->list);
}
/*
* Must be called with queue->lock acquired.
*/
static inline struct sgx_epc_page *__sgx_epc_queue_pop(struct sgx_epc_queue *queue)
{
struct sgx_epc_page *page;
if (list_empty(&queue->list)
return NULL;
page = list_first_entry(&queue->list, struct sgx_epc_page, list);
list_del_init(&page->list);
return page;
}
And use them in existing sites. It ensures coherent behavior. You should be
able to replace all uses with either, or combination of them (list_move).
BR, Jarkko
On Fri, 2022-09-23 at 16:20 +0300, Jarkko Sakkinen wrote:
> On Thu, Sep 22, 2022 at 10:10:41AM -0700, Kristen Carlson Accardi
> wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> >
> > Wrap the existing reclaimable list and its spinlock in a struct to
> > minimize the code changes needed to handle multiple LRUs as well as
> > reclaimable and non-reclaimable lists, both of which will be
> > introduced
> > and used by SGX EPC cgroups.
> >
> > Signed-off-by: Sean Christopherson
> > <sean.j.christopherson@intel.com>
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Cc: Sean Christopherson <seanjc@google.com>
>
> The commit message could explicitly state the added data type.
>
> The data type is not LRU: together with the LIFO list, i.e.
> a queue, the code implements LRU alike policy.
>
> I would name the data type as sgx_epc_queue because it is a
> less confusing name.
I think when you look at patch 05/20 which adds the unreclaimable field
this becomes less like a straight up queue data type.
>
> > ---
> > arch/x86/kernel/cpu/sgx/main.c | 37 +++++++++++++++++-------------
> > ----
> > arch/x86/kernel/cpu/sgx/sgx.h | 11 ++++++++++
> > 2 files changed, 30 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > b/arch/x86/kernel/cpu/sgx/main.c
> > index 4cdeb915dc86..af68dc1c677b 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -26,10 +26,9 @@ static DEFINE_XARRAY(sgx_epc_address_space);
> >
> > /*
> > * These variables are part of the state of the reclaimer, and
> > must be accessed
> > - * with sgx_reclaimer_lock acquired.
> > + * with sgx_global_lru.lock acquired.
> > */
> > -static LIST_HEAD(sgx_active_page_list);
> > -static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> > +static struct sgx_epc_lru sgx_global_lru;
> >
> > static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
> >
> > @@ -298,12 +297,12 @@ static void sgx_reclaim_pages(void)
> > int ret;
> > int i;
> >
> > - spin_lock(&sgx_reclaimer_lock);
> > + spin_lock(&sgx_global_lru.lock);
> > for (i = 0; i < SGX_NR_TO_SCAN; i++) {
> > - if (list_empty(&sgx_active_page_list))
> > + if (list_empty(&sgx_global_lru.reclaimable))
> > break;
> >
> > - epc_page = list_first_entry(&sgx_active_page_list,
> > + epc_page =
> > list_first_entry(&sgx_global_lru.reclaimable,
> > struct sgx_epc_page,
> > list);
> > list_del_init(&epc_page->list);
> > encl_page = epc_page->owner;
> > @@ -316,7 +315,7 @@ static void sgx_reclaim_pages(void)
> > */
> > epc_page->flags &=
> > ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > }
> > - spin_unlock(&sgx_reclaimer_lock);
> > + spin_unlock(&sgx_global_lru.lock);
> >
> > for (i = 0; i < cnt; i++) {
> > epc_page = chunk[i];
> > @@ -339,9 +338,9 @@ static void sgx_reclaim_pages(void)
> > continue;
> >
> > skip:
> > - spin_lock(&sgx_reclaimer_lock);
> > - list_add_tail(&epc_page->list,
> > &sgx_active_page_list);
> > - spin_unlock(&sgx_reclaimer_lock);
> > + spin_lock(&sgx_global_lru.lock);
> > + list_add_tail(&epc_page->list,
> > &sgx_global_lru.reclaimable);
> > + spin_unlock(&sgx_global_lru.lock);
> >
> > kref_put(&encl_page->encl->refcount,
> > sgx_encl_release);
> >
> > @@ -374,7 +373,7 @@ static void sgx_reclaim_pages(void)
> > static bool sgx_should_reclaim(unsigned long watermark)
> > {
> > return atomic_long_read(&sgx_nr_free_pages) < watermark &&
> > - !list_empty(&sgx_active_page_list);
> > + !list_empty(&sgx_global_lru.reclaimable);
> > }
> >
> > /*
> > @@ -427,6 +426,8 @@ static bool __init
> > sgx_page_reclaimer_init(void)
> >
> > ksgxd_tsk = tsk;
> >
> > + sgx_lru_init(&sgx_global_lru);
> > +
> > return true;
> > }
> >
> > @@ -502,10 +503,10 @@ struct sgx_epc_page
> > *__sgx_alloc_epc_page(void)
> > */
> > void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
> > {
> > - spin_lock(&sgx_reclaimer_lock);
> > + spin_lock(&sgx_global_lru.lock);
> > page->flags |= SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > - list_add_tail(&page->list, &sgx_active_page_list);
> > - spin_unlock(&sgx_reclaimer_lock);
> > + list_add_tail(&page->list, &sgx_global_lru.reclaimable);
> > + spin_unlock(&sgx_global_lru.lock);
> > }
> >
> > /**
> > @@ -520,18 +521,18 @@ void sgx_mark_page_reclaimable(struct
> > sgx_epc_page *page)
> > */
> > int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
> > {
> > - spin_lock(&sgx_reclaimer_lock);
> > + spin_lock(&sgx_global_lru.lock);
> > if (page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) {
> > /* The page is being reclaimed. */
> > if (list_empty(&page->list)) {
> > - spin_unlock(&sgx_reclaimer_lock);
> > + spin_unlock(&sgx_global_lru.lock);
> > return -EBUSY;
> > }
> >
> > list_del(&page->list);
> > page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > }
> > - spin_unlock(&sgx_reclaimer_lock);
> > + spin_unlock(&sgx_global_lru.lock);
> >
> > return 0;
> > }
> > @@ -564,7 +565,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
> > *owner, bool reclaim)
> > break;
> > }
> >
> > - if (list_empty(&sgx_active_page_list))
> > + if (list_empty(&sgx_global_lru.reclaimable))
> > return ERR_PTR(-ENOMEM);
> >
> > if (!reclaim) {
> > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
> > b/arch/x86/kernel/cpu/sgx/sgx.h
> > index 5a7e858a8f98..7b208ee8eb45 100644
> > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > @@ -83,6 +83,17 @@ static inline void *sgx_get_epc_virt_addr(struct
> > sgx_epc_page *page)
> > return section->virt_addr + index * PAGE_SIZE;
> > }
> >
> > +struct sgx_epc_lru {
> > + spinlock_t lock;
> > + struct list_head reclaimable;
>
> s/reclaimable/list/
It feels to me that once you add the "unreclaimable" struct list_head
field to this struct in the next patch, it would be a bit confusing to
rename this to just "list". What the final struct looks like is
actually not really a nice clean simple queue, but 2 lists - one for
EPC pages which are being tracked by the reclaimer, and one for EPC
pages which are not (such as va pages).
>
> > +};
> > +
> > +static inline void sgx_lru_init(struct sgx_epc_lru *lru)
> > +{
> > + spin_lock_init(&lru->lock);
> > + INIT_LIST_HEAD(&lru->reclaimable);
> > +}
> > +
> > struct sgx_epc_page *__sgx_alloc_epc_page(void);
> > void sgx_free_epc_page(struct sgx_epc_page *page);
> >
> > --
> > 2.37.3
> >
>
> Please also add these:
>
> /*
> * Must be called with queue->lock acquired.
> */
> static inline struct sgx_epc_page *__sgx_epc_queue_push(struct
> sgx_epc_queue *queue,
> struct
> sgx_page *page)
> {
> list_add_tail(&page->list, &queue->list);
> }
>
> /*
> * Must be called with queue->lock acquired.
> */
> static inline struct sgx_epc_page *__sgx_epc_queue_pop(struct
> sgx_epc_queue *queue)
> {
> struct sgx_epc_page *page;
>
> if (list_empty(&queue->list)
> return NULL;
>
> page = list_first_entry(&queue->list, struct sgx_epc_page,
> list);
> list_del_init(&page->list);
>
> return page;
> }
>
> And use them in existing sites. It ensures coherent behavior. You
> should be
> able to replace all uses with either, or combination of them
> (list_move).
>
> BR, Jarkko
© 2016 - 2026 Red Hat, Inc.