> > > > > In other cases, the caller may invoke this function in a > > > loop to ensure enough pages reclaimed for its usage. To ensure all > > > descendant groups scanned in a round-robin fashion in those cases, > > > sgx_cgroup_reclaim_pages() takes in a starting cgroup and returns the > > > next cgroup that the caller can pass in as the new starting cgroup for a > > > subsequent call. > > > > > > AFAICT this part is new, and I believe this "round-robin" thing is just > > for the "global reclaim"? Or is it also for per-cgroup reclaim where > > more > > than SGX_NR_TO_SCAN pages needs to be reclaimed? > > > > I wish the changelog should just point out what consumers will use this > > new sgx_cgroup_reclaim_pages(), like: > > > > The sgx_cgroup_reclaim_pages() will be used in three cases: > > > > 1) direct/sync per-cgroup reclaim in try_charge() > > 2) indirect/async per-cgroup reclaim triggered in try_charge() > > 3) global reclaim > > > > And then describe how will they use sgx_cgroup_reclaim_pages(): > > > > Both 1) and 2) can result in needing to reclaim more than SGX_NR_TO_SCAN > > pages, in which case we should <fill in how to reclaim>. > > > > For 3), the new global reclaim should try tot match the existing global > > reclaim behaviour, that is to try to treat all EPC pages equally. > > <continue to explain how can sgx_cgroup_reclaim_pages() achieve this.> > > > > With above context, we can justify why to make sgx_cgroup_reclaim_pages() > > in this form. > > > This new part is only to address the issue you raised in this thread: > https://lore.kernel.org/lkml/op.2ndsydgywjvjmi@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > Really it has nothing to do whether global, direct/async, per-cgroup > contexts. They all should use the function the same way. This paragraph > describes the design and > I thought the above new statements justify the reason we return 'next' so > it can reclaim into descedant in round-robin fashion? No sure we need get > into details of different usages of the functions which are in code > actually? Please clearly define the behaviour of "per-cgroup reclaim" first. I can understand "global reclaim" means we essentially want to treats all EPC pages equally. But it's not obvious to me what is the desired behaviour of "per-cgroup reclaim", especially when the behaviour is different between this version and the previous versions (see below). > [...] > > And when there are more than SGX_NR_TO_SCAN pages that need to reclaim, > > the above ... > > Note, all sgx_cgroup_reclaim_pages() guarantees is scanning SGX_NR_TO_SCAN > pages. > > > > for (;;) { > > cg_next = sgx_cgroup_reclaim_pages(sgx_cg->cg, cg_next); > > } > > > > ... actually tries to reclaim those pages from @sgx_cg _AND_ it's > > descendants, and tries to do it _EQUALLY_. > > > > Is this desired, or should we always try to reclaim from the @sgx_cg > > first, but only moves to the desendants when the @sgx_cg shouldn't be > > reclaimed anymore? > > > > we still reclaim in sgx_cg in first scan and attempt of reclaiming for > SGX_NR_TOS_CAN pages, but if it turns out that did not satisfy caller > needs, then caller goes on to reclaim from descendants by passing in > 'next' as starting point. But why? > > > Anyway, it's different from the previous behaviour. > > > Again, this is to fix the issue you raised. I consider it improved > behavior :-) Please clearly define the _EXPECTED_ hebaviour of "per-cgroup reclaim" first. > We have two choices: 1) Always try to reclaim desired number of pages from the given cgroup, but only moves to reclaim from descendants when there's less than SGX_NR_TO_SCAN pages left; 2) Always try to reclaim desired number of pages _EQUALLY_ from the given cgroup _AND_ its descendants (in granularity of reclaiming SGX_NR_TO_SCAN pages each time). The 1) was the old behavour in the previous versions, 2) is the new behaviour in this version. I am not against any option, but the patch needs to be clear on which option to choose and why it is desired/better.
