Re: [PATCH v12 09/14] x86/sgx: Implement async reclamation for cgroup

[Haitao Huang]

On Tue, 23 Apr 2024 17:13:15 -0500, Huang, Kai <kai.huang@xxxxxxxxx> wrote:

> On Tue, 2024-04-23 at 10:30 -0500, Haitao Huang wrote:
> > > > It's a workaround because you use the capacity==0 but it does not  
> > really
> > > > mean to disable the misc cgroup for specific resource IIUC.
> > >
> > > Please read the comment around @misc_res_capacity again:
> > >
> > >   * Miscellaneous resources capacity for the entire machine. 0  
> > capacity
> > >   * means resource is not initialized or not present in the host.
> > >
> >
> > I mentioned this in earlier email. I think this means no SGX EPC. It  
> > doesnot mean sgx epc cgroup not enabled. That's also consistent with the 
> > behavior try_charge() fails if capacity is zero.
>
> OK. To me the "capacity" is purely the concept of cgroup, so it must be
> interpreted within the scope of "cgroup".  If cgroup, in our case, SGX
> cgroup, is disabled, then whether "leaving the capacity to reflect the
> presence of hardware resource" doesn't really matter.
> So what you are saying is that, the kernel must initialize the capacity  
> of
> some MISC resource once it is added as valid type.  
> And you must initialize the "capacity" even MISC cgroup is disabled
> entirely by kernel commandline, in which case, IIUC, misc.capacity is not
> even going to show in the /fs.
>
> If this is your point, then your patch:
>
> 	cgroup/misc: Add SGX EPC resource type
>
> is already broken, because you added the new type w/o initializing the
> capacity.
>
> Please fix that up.
>
> > > >
> > > > There is explicit way for user to disable misc without setting  
> > capacity> > to
> > > > zero.
> > >
> > > Which way are you talking about?
> >
> > Echo "-misc" to cgroup.subtree_control at root level for example still 
> > shows non-zero sgx_epc capacity.
>
> I guess "having to disable all MISC resources just in order to disable  
> SGX
> EPC cgroup" is a brilliant idea.
>
> You can easily disable the entire MISC cgroup by commandline for that
> purpose if that's acceptable.
>
> And I have no idea why "still showing non-zero EPC capacity" is important
> if SGX cgroup cannot be supported at all.

Okay, all I'm trying to say is we should care about consistency in code  
and don't want SGX do something different. Mixing "disable" with  
"capacity==0" causes inconsistencies AFAICS:

1) The try_charge() API currently returns error when capacity is zero. So  
it appears not to mean that the cgroup is disabled otherwise it should  
return success.

2) The current explicit way ("-misc") to disable misc still shows non-zero  
entries in misc.capacity. (At least for v2 cgroup, it does when I tested).  
Maybe this is not important but I just don't feel good about this  
inconsistency.

For now I'll just do BUG_ON() unless there are more strong opinions one  
way or the other.

BR
Haitao