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);
