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

Huang, Kai
2024-04-29T22:19:35Z

>>>  /*
>>> @@ -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.