Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 30 Apr 2024 10:18:05 +1200 | Subject | Re: [PATCH v12 12/14] x86/sgx: Turn on per-cgroup EPC reclamation | From | "Huang, Kai" <> |
| |
>>> /* >>> @@ -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.
| ![\](/images/icornerr.gif) |