Re: [PATCH v6 07/12] x86/sgx: Introduce EPC page states

Haitao Huang
2024-01-06T01:45:37Z

On Fri, 05 Jan 2024 11:57:03 -0600, Dave Hansen <dave.hansen@intel.com>
wrote:

> On 10/30/23 11:20, Haitao Huang wrote:
>> @@ -527,16 +530,13 @@ void sgx_mark_page_reclaimable(struct
>> sgx_epc_page *page)
>> int sgx_unmark_page_reclaimable(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 (list_empty(&page->list)) {
>> - spin_unlock(&sgx_global_lru.lock);
>> - return -EBUSY;
>> - }
>> -
>> - list_del(&page->list);
>> - page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>> + 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);
>
> I want to know how much if this series is basically line-for-line
> abstraction shifting like:
>
> - page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> + sgx_epc_page_reset_state(page);
>
> versus actually adding complexity. That way, I might be able to offer
> some advice on where this can be pared down. That's really hard to do
> with the current series.
>
> Please don't just "introduce new page states". This should have first
> abstracted out the sgx_epc_page_reclaim_in_progress() operation, using
> the list_empty() check as the implementation.
>
> Then, in a separate patch, introduce the concept of the "reclaim in
> progress" flag and finally flip the implementation over.
>
> Ditto for the sgx_epc_page_reset_state() abstraction. It should have
> been introduced separately as a concept and then have the implementation
> changed.
>
> On in to patch 10 (which is much too big) which introduces the
> sgx_lru_list() abstraction.
>

Sure. I'll try to refactor according to this plan.
Thanks
Haitao