Re: [PATCH v12 12/14] x86/sgx: Turn on per-cgroup EPC reclamation

Haitao Huang
2024-04-30T01:31:26Z

On Mon, 29 Apr 2024 17:18:05 -0500, Huang, Kai <kai.huang@intel.com> wrote:

>
>>>> /*
>>>> @@ -42,7 +63,8 @@ static inline struct sgx_epc_lru_list
>>>> *sgx_lru_list(struct sgx_epc_page *epc_pag
>>>> */
>>>> static inline bool sgx_can_reclaim(void)
>>>> {
>>>> - return !list_empty(&sgx_global_lru.reclaimable);
>>>> + return !sgx_cgroup_lru_empty(misc_cg_root()) ||
>>>> + !list_empty(&sgx_global_lru.reclaimable);
>>>> }
>>>
>>> Shouldn't this be:
>>>
>>> if (IS_ENABLED(CONFIG_CGROUP_MISC))
>>> return !sgx_cgroup_lru_empty(misc_cg_root());
>>> else
>>> return !list_empty(&sgx_global_lru.reclaimable);
>>> ?
>>>
>>> In this way, it is consistent with the sgx_reclaim_pages_global()
>>> below.
>>>
>> I changed to this way because sgx_cgroup_lru_empty() is now defined in
>> both KConfig cases.
>> And it seems better to minimize use of the KConfig variables based on
>> earlier feedback (some are yours).
>> Don't really have strong preference here. So let me know one way of the
>> other.
>>
>
> But IMHO your code could be confusing, e.g., it can be interpreted as:
>
> The EPC pages can be managed by both the cgroup LRUs and the
> sgx_global_lru simultaneously at runtime when CONFIG_CGROUP_MISC
> is on.
>
> Which is not true.
>
> So we should make the code clearly reflect the true behaviour.
>

Ok, I'll change back.
Thanks
Haitao