From: Dave Hansen <dave.hansen@intel.com>
To: Kristen Carlson Accardi <kristen@linux.intel.com>,
jarkko@kernel.org, dave.hansen@linux.intel.com, tj@kernel.org,
linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org,
cgroups@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>
Cc: zhiquan1.li@intel.com, Sean Christopherson <seanjc@google.com>
Subject: Re: [PATCH v2 07/18] x86/sgx: Use a list to track to-be-reclaimed pages during reclaim
Date: Fri, 2 Dec 2022 14:33:14 -0800 [thread overview]
Message-ID: <0646275c-0ab9-2fad-8db1-7098656d6e1d@intel.com> (raw)
In-Reply-To: <20221202183655.3767674-8-kristen@linux.intel.com>
On 12/2/22 10:36, Kristen Carlson Accardi wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
>
> Change sgx_reclaim_pages() to use a list rather than an array for
> storing the epc_pages which will be reclaimed. This change is needed
> to transition to the LRU implementation for EPC cgroup support.
>
> This change requires keeping track of whether newly recorded
> EPC pages are pages for VA Arrays, or for Enclave data. In addition,
> helper functions are added to move pages from one list to another and
> enforce a consistent queue like behavior for the LRU lists.
More changelog nit: Please use imperative voice, not passive voice.
Move from:
In addition, helper functions are added
to:
In addition, add helper functions
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 4683da9ef4f1..9ee306ac2a8e 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -252,7 +252,7 @@ static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl,
> epc_page = sgx_encl_eldu(&encl->secs, NULL);
> if (IS_ERR(epc_page))
> return ERR_CAST(epc_page);
> - sgx_record_epc_page(epc_page, 0);
> + sgx_record_epc_page(epc_page, SGX_EPC_PAGE_ENCLAVE);
> }
This is one of those patches where the first hunk seems like it is
entirely disconnected from what the changelog made me expect I would see.
I don't see sgx_reclaim_pages(), or lists or arrays.
If you need to pass additional data down into a function, then do *that*
in a separate patch.
I'm glad it eventually got fixed up, but I don't really ever like to see
bare integers that don't have obvious meaning:
sgx_record_epc_page(epc_page, 0);
Even if you had:
#define SGX_EPC_PAGE_RECLAIMER_UNTRACKED 0
sgx_record_epc_page(epc_page, SGX_EPC_PAGE_RECLAIMER_UNTRACKED);
makes a *LOT* of sense compared to other callers that do
sgx_record_epc_page(epc_page, SGX_EPC_PAGE_RECLAIMER_TRACKED);
> epc_page = sgx_encl_eldu(entry, encl->secs.epc_page);
> @@ -260,7 +260,8 @@ static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl,
> return ERR_CAST(epc_page);
>
> encl->secs_child_cnt++;
> - sgx_record_epc_page(entry->epc_page, SGX_EPC_PAGE_RECLAIMER_TRACKED);
> + sgx_record_epc_page(entry->epc_page,
> + (SGX_EPC_PAGE_ENCLAVE | SGX_EPC_PAGE_RECLAIMER_TRACKED));
>
> return entry;
> }
> @@ -1221,7 +1222,7 @@ struct sgx_epc_page *sgx_alloc_va_page(struct sgx_encl *encl, bool reclaim)
> sgx_encl_free_epc_page(epc_page);
> return ERR_PTR(-EFAULT);
> }
> - sgx_record_epc_page(epc_page, 0);
> + sgx_record_epc_page(epc_page, SGX_EPC_PAGE_VERSION_ARRAY);
>
> return epc_page;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index aca80a3f38a1..c3a9bffbc37e 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -114,7 +114,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> encl->attributes = secs->attributes;
> encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | SGX_ATTR_KSS;
>
> - sgx_record_epc_page(encl->secs.epc_page, 0);
> + sgx_record_epc_page(encl->secs.epc_page, SGX_EPC_PAGE_ENCLAVE);
>
> /* Set only after completion, as encl->lock has not been taken. */
> set_bit(SGX_ENCL_CREATED, &encl->flags);
> @@ -325,7 +325,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> goto err_out;
> }
>
> - sgx_record_epc_page(encl_page->epc_page, SGX_EPC_PAGE_RECLAIMER_TRACKED);
> + sgx_record_epc_page(encl_page->epc_page,
> + (SGX_EPC_PAGE_ENCLAVE | SGX_EPC_PAGE_RECLAIMER_TRACKED));
> mutex_unlock(&encl->lock);
> mmap_read_unlock(current->mm);
> return ret;
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index bad72498b0a7..83aaf5cea7b9 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -288,37 +288,43 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
> */
> static void __sgx_reclaim_pages(void)
> {
> - struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> struct sgx_backing backing[SGX_NR_TO_SCAN];
> + struct sgx_epc_page *epc_page, *tmp;
> struct sgx_encl_page *encl_page;
> - struct sgx_epc_page *epc_page;
> pgoff_t page_index;
> - int cnt = 0;
> + LIST_HEAD(iso);
> int ret;
> int i;
>
> spin_lock(&sgx_global_lru.lock);
> for (i = 0; i < SGX_NR_TO_SCAN; i++) {
> - epc_page = sgx_epc_pop_reclaimable(&sgx_global_lru);
> + epc_page = sgx_epc_peek_reclaimable(&sgx_global_lru);
> if (!epc_page)
> break;
>
> encl_page = epc_page->encl_owner;
>
> + if (WARN_ON_ONCE(!(epc_page->flags & SGX_EPC_PAGE_ENCLAVE)))
> + continue;
> +
> if (kref_get_unless_zero(&encl_page->encl->refcount) != 0) {
> epc_page->flags |= SGX_EPC_PAGE_RECLAIM_IN_PROGRESS;
> - chunk[cnt++] = epc_page;
> + list_move_tail(&epc_page->list, &iso);
> } else {
> - /* The owner is freeing the page. No need to add the
> - * page back to the list of reclaimable pages.
> + /* The owner is freeing the page, remove it from the
> + * LRU list
> */
> epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> + list_del_init(&epc_page->list);
> }
> }
> spin_unlock(&sgx_global_lru.lock);
>
> - for (i = 0; i < cnt; i++) {
> - epc_page = chunk[i];
> + if (list_empty(&iso))
> + return;
> +
> + i = 0;
> + list_for_each_entry_safe(epc_page, tmp, &iso, list) {
> encl_page = epc_page->encl_owner;
>
> if (!sgx_reclaimer_age(epc_page))
> @@ -333,6 +339,7 @@ static void __sgx_reclaim_pages(void)
> goto skip;
> }
>
> + i++;
> encl_page->desc |= SGX_ENCL_PAGE_BEING_RECLAIMED;
> mutex_unlock(&encl_page->encl->lock);
> continue;
> @@ -340,31 +347,25 @@ 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);
> + sgx_epc_move_reclaimable(&sgx_global_lru, epc_page);
> spin_unlock(&sgx_global_lru.lock);
>
> kref_put(&encl_page->encl->refcount, sgx_encl_release);
> -
> - chunk[i] = NULL;
> }
>
> - for (i = 0; i < cnt; i++) {
> - epc_page = chunk[i];
> - if (epc_page)
> - sgx_reclaimer_block(epc_page);
> - }
> -
> - for (i = 0; i < cnt; i++) {
> - epc_page = chunk[i];
> - if (!epc_page)
> - continue;
> -
> + list_for_each_entry(epc_page, &iso, list)
> + sgx_reclaimer_block(epc_page);
> +
> + i = 0;
> + list_for_each_entry_safe(epc_page, tmp, &iso, list) {
> encl_page = epc_page->encl_owner;
> - sgx_reclaimer_write(epc_page, &backing[i]);
> + sgx_reclaimer_write(epc_page, &backing[i++]);
>
> kref_put(&encl_page->encl->refcount, sgx_encl_release);
> epc_page->flags &= ~(SGX_EPC_PAGE_RECLAIMER_TRACKED |
> - SGX_EPC_PAGE_RECLAIM_IN_PROGRESS);
> + SGX_EPC_PAGE_RECLAIM_IN_PROGRESS |
> + SGX_EPC_PAGE_ENCLAVE |
> + SGX_EPC_PAGE_VERSION_ARRAY);
>
> sgx_free_epc_page(epc_page);
> }
> @@ -505,6 +506,7 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
> /**
> * sgx_record_epc_page() - Add a page to the LRU tracking
> * @page: EPC page
> + * @flags: Reclaim flags for the page.
> *
> * Mark a page with the specified flags and add it to the appropriate
> * (un)reclaimable list.
> @@ -535,18 +537,19 @@ 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 (page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) {
> - /* The page is being reclaimed. */
> - if (page->flags & SGX_EPC_PAGE_RECLAIM_IN_PROGRESS) {
> - spin_unlock(&sgx_global_lru.lock);
> - return -EBUSY;
> - }
> -
> - page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> + if ((page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) &&
> + (page->flags & SGX_EPC_PAGE_RECLAIM_IN_PROGRESS)) {
> + spin_unlock(&sgx_global_lru.lock);
> + return -EBUSY;
> }
> list_del(&page->list);
> spin_unlock(&sgx_global_lru.lock);
>
> + page->flags &= ~(SGX_EPC_PAGE_RECLAIMER_TRACKED |
> + SGX_EPC_PAGE_RECLAIM_IN_PROGRESS |
> + SGX_EPC_PAGE_ENCLAVE |
> + SGX_EPC_PAGE_VERSION_ARRAY);
> +
> return 0;
> }
>
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 37d66bc6ca27..ec8d567cd975 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -32,6 +32,8 @@
> #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_ENCLAVE BIT(4)
> +#define SGX_EPC_PAGE_VERSION_ARRAY BIT(5)
Could you please spend some time to clearly document what each bit means?
> +static inline void __sgx_epc_page_list_move(struct list_head *list, struct sgx_epc_page *page)
> +{
> + list_move_tail(&page->list, list);
> +}
I'm not sure I get the point of a helper like this. Why not just have
the caller call list_move() directly?
> /*
> * Must be called with queue lock acquired
> */
> @@ -157,6 +167,38 @@ static inline void sgx_epc_push_unreclaimable(struct sgx_epc_lru_lists *lrus,
> __sgx_epc_page_list_push(&(lrus)->unreclaimable, page);
> }
>
> +/*
> + * Must be called with queue lock acquired
> + */
> +static inline struct sgx_epc_page * __sgx_epc_page_list_peek(struct list_head *list)
> +{
> + struct sgx_epc_page *epc_page;
> +
> + if (list_empty(list))
> + return NULL;
> +
> + epc_page = list_first_entry(list, struct sgx_epc_page, list);
> + return epc_page;
> +}
list_first_entry_or_null() perhaps?
> +static inline struct sgx_epc_page *
> +sgx_epc_peek_reclaimable(struct sgx_epc_lru_lists *lrus)
> +{
> + return __sgx_epc_page_list_peek(&(lrus)->reclaimable);
> +}
> +
> +static inline void sgx_epc_move_reclaimable(struct sgx_epc_lru_lists *lru,
> + struct sgx_epc_page *page)
> +{
> + __sgx_epc_page_list_move(&(lru)->reclaimable, page);
> +}
> +
> +static inline struct sgx_epc_page *
> +sgx_epc_peek_unreclaimable(struct sgx_epc_lru_lists *lrus)
> +{
> + return __sgx_epc_page_list_peek(&(lrus)->unreclaimable);
> +}
In general, I'm not becoming more fond of these helpers as the series
goes along. My worry is that they're an abstraction where we don't
*really* need one. I don't seem them growing much functionality as the
series goes along.
I'll reserve judgement until the end though.
next prev parent reply other threads:[~2022-12-02 22:33 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-02 18:36 [PATCH v2 00/18] Add Cgroup support for SGX EPC memory Kristen Carlson Accardi
2022-12-02 18:36 ` [PATCH v2 01/18] x86/sgx: Call cond_resched() at the end of sgx_reclaim_pages() Kristen Carlson Accardi
2022-12-02 21:33 ` Dave Hansen
2022-12-02 21:37 ` Kristen Carlson Accardi
2022-12-02 21:45 ` Dave Hansen
2022-12-02 22:17 ` Kristen Carlson Accardi
2022-12-02 22:37 ` Dave Hansen
2022-12-02 18:36 ` [PATCH v2 02/18] x86/sgx: Store struct sgx_encl when allocating new VA pages Kristen Carlson Accardi
2022-12-02 21:35 ` Dave Hansen
2022-12-02 21:40 ` Kristen Carlson Accardi
2022-12-02 21:48 ` Dave Hansen
2022-12-02 22:35 ` Sean Christopherson
2022-12-02 22:47 ` Dave Hansen
2022-12-02 22:49 ` Sean Christopherson
2022-12-02 18:36 ` [PATCH v2 03/18] x86/sgx: Add 'struct sgx_epc_lru_lists' to encapsulate lru list(s) Kristen Carlson Accardi
2022-12-02 21:39 ` Dave Hansen
2022-12-08 15:31 ` Jarkko Sakkinen
2022-12-08 18:03 ` Kristen Carlson Accardi
2022-12-02 18:36 ` [PATCH v2 04/18] x86/sgx: Use sgx_epc_lru_lists for existing active page list Kristen Carlson Accardi
2022-12-02 21:43 ` Dave Hansen
2022-12-02 21:51 ` Kristen Carlson Accardi
2022-12-02 22:10 ` Dave Hansen
2022-12-02 18:36 ` [PATCH v2 05/18] x86/sgx: Track epc pages on reclaimable or unreclaimable lists Kristen Carlson Accardi
2022-12-02 22:13 ` Dave Hansen
2022-12-02 22:28 ` Sean Christopherson
2022-12-02 18:36 ` [PATCH v2 06/18] x86/sgx: Introduce RECLAIM_IN_PROGRESS flag for EPC pages Kristen Carlson Accardi
2022-12-02 22:15 ` Dave Hansen
2022-12-08 15:46 ` Jarkko Sakkinen
2022-12-08 18:13 ` Kristen Carlson Accardi
2022-12-02 18:36 ` [PATCH v2 07/18] x86/sgx: Use a list to track to-be-reclaimed pages during reclaim Kristen Carlson Accardi
2022-12-02 22:33 ` Dave Hansen [this message]
2022-12-05 16:33 ` Kristen Carlson Accardi
2022-12-05 17:03 ` Dave Hansen
2022-12-05 18:25 ` Kristen Carlson Accardi
2022-12-02 18:36 ` [PATCH v2 08/18] x86/sgx: Allow reclaiming up to 32 pages, but scan 16 by default Kristen Carlson Accardi
2022-12-08 9:26 ` Jarkko Sakkinen
2022-12-08 9:27 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 09/18] x86/sgx: Return the number of EPC pages that were successfully reclaimed Kristen Carlson Accardi
2022-12-08 9:30 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 10/18] x86/sgx: Add option to ignore age of page during EPC reclaim Kristen Carlson Accardi
2022-12-08 9:37 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 11/18] x86/sgx: Prepare for multiple LRUs Kristen Carlson Accardi
2022-12-08 9:42 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 12/18] x86/sgx: Expose sgx_reclaim_pages() for use by EPC cgroup Kristen Carlson Accardi
2022-12-08 9:46 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 13/18] x86/sgx: Add helper to grab pages from an arbitrary EPC LRU Kristen Carlson Accardi
2022-12-08 9:56 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 14/18] x86/sgx: Add EPC OOM path to forcefully reclaim EPC Kristen Carlson Accardi
2022-12-08 15:21 ` Jarkko Sakkinen
2022-12-09 16:05 ` Kristen Carlson Accardi
2022-12-09 16:22 ` Dave Hansen
2022-12-12 18:09 ` Sean Christopherson
2022-12-26 20:43 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 15/18] cgroup/misc: Add per resource callbacks for css events Kristen Carlson Accardi
2022-12-08 14:53 ` Jarkko Sakkinen
2022-12-08 15:15 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 16/18] cgroup/misc: Prepare for SGX usage Kristen Carlson Accardi
2022-12-08 15:23 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 17/18] x86/sgx: Add support for misc cgroup controller Kristen Carlson Accardi
2022-12-08 15:30 ` Jarkko Sakkinen
2022-12-02 18:36 ` [PATCH v2 18/18] Docs/x86/sgx: Add description for cgroup support Kristen Carlson Accardi
2023-04-03 21:26 ` [EXTERNAL] [PATCH v2 00/18] Add Cgroup support for SGX EPC memory Anand Krishnamoorthi
2023-04-13 18:49 ` Anand Krishnamoorthi
2023-04-18 16:44 ` Mikko Ylinen
2023-04-27 16:53 ` Anand Krishnamoorthi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0646275c-0ab9-2fad-8db1-7098656d6e1d@intel.com \
--to=dave.hansen@intel.com \
--cc=bp@alien8.de \
--cc=cgroups@vger.kernel.org \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--cc=kristen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=tj@kernel.org \
--cc=x86@kernel.org \
--cc=zhiquan1.li@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).