lkml.org 
[lkml]   [2022]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 05/18] x86/sgx: Track epc pages on reclaimable or unreclaimable lists
From
On 12/2/22 10:36, Kristen Carlson Accardi wrote:
> Replace functions sgx_mark_page_reclaimable() and
> sgx_unmark_page_reclaimable() with sgx_record_epc_page() and
> sgx_drop_epc_page(). sgx_record_epc_page() wil add the epc_page
> to the correct "reclaimable" or "unreclaimable" list in the
> sgx_epc_lru_lists struct. sgx_drop_epc_page() will delete the page
> from the LRU list. Tracking pages that are not tracked by
> the reclaimer in the sgx_epc_lru_lists "unreclaimable" list allows
> an OOM event to cause all the pages in use by an enclave to be freed,
> regardless of whether they were reclaimable pages or not.

This might be more a comment about Sean's stuff, but could you please
start using paragraphs in these changelogs?

Also, on the content, I really prefer that patches start off talking in
English as much as possible and not just talk about the code.

Right now, SGX has a single LRU list. The code is transitioning
over to use multiple LRU lists.

I'd also prefer that _this_ patch do the:

> - sgx_mark_page_reclaimable(entry->epc_page);
> + sgx_record_epc_page(entry->epc_page, SGX_EPC_PAGE_RECLAIMER_TRACKED);

bits and then *another* patch do the unreclaimable side. This patch
could be a straight replacement which is easy to audit. The
unreclaimable one needs more thought.

I also think this ends up looking a bit weird:

> - sgx_epc_push_reclaimable(&sgx_global_lru, page);
> + WARN_ON(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> + page->flags |= flags;
> + if (flags & SGX_EPC_PAGE_RECLAIMER_TRACKED)
> + sgx_epc_push_reclaimable(&sgx_global_lru, page);
> + else
> + sgx_epc_push_unreclaimable(&sgx_global_lru, page);
> spin_unlock(&sgx_global_lru.lock);
> }

I think that would be better with a single "push" helper and then let
the callers specify the list:

if (flags & SGX_EPC_PAGE_RECLAIMER_TRACKED)
sgx_lru_push(&sgx_global_lru.reclaimable, page);
else
sgx_lru_push(&sgx_global_lru.unreclaimable, page);

\
 
 \ /
  Last update: 2022-12-02 23:14    [W:0.327 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site