Re: [PATCH v15 08/14] x86/sgx: Add basic EPC reclamation flow for cgroup

["Huang, Kai" <kai.huang@intel.com>]

> > 
> > > 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@hhuan26-mobl.amr.corp.intel.com/
> 
> 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.