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

Haitao Huang
2024-04-23T15:31:10Z

On Tue, 23 Apr 2024 09:19:53 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Tue, 2024-04-23 at 08:08 -0500, Haitao Huang wrote:
>> On Mon, 22 Apr 2024 17:16:34 -0500, Huang, Kai <kai.huang@intel.com>
>> wrote:
>>
>> > On Mon, 2024-04-22 at 11:17 -0500, Haitao Huang wrote:
>> > > On Sun, 21 Apr 2024 19:22:27 -0500, Huang, Kai <kai.huang@intel.com>
>> > > wrote:
>> > >
>> > > > On Fri, 2024-04-19 at 20:14 -0500, Haitao Huang wrote:
>> > > > > > > I think we can add support for "sgx_cgroup=disabled" in
>> future
>> > > if
>> > > > > indeed
>> > > > > > > needed. But just for init failure, no?
>> > > > > > >
>> > > > > >
>> > > > > > It's not about the commandline, which we can add in the future
>> > > when
>> > > > > > needed. It's about we need to have a way to handle SGX cgroup
>> > > being
>> > > > > > disabled at boot time nicely, because we already have a case
>> > > where we
>> > > > > > need
>> > > > > > to do so.
>> > > > > >
>> > > > > > Your approach looks half-way to me, and is not future
>> > > extendible. If
>> > > > > we
>> > > > > > choose to do it, do it right -- that is, we need a way to
>> disable
>> > > it
>> > > > > > completely in both kernel and userspace so that userspace
>> won't be
>> > > > > able> to
>> > > > > > see it.
>> > > > >
>> > > > > That would need more changes in misc cgroup implementation to
>> > > support
>> > > > > sgx-disable. Right now misc does not have separate files for
>> > > different
>> > > > > resource types. So we can only block echo "sgx_epc..." to those
>> > > > > interfacefiles, can't really make files not visible.
>> > > >
>> > > > "won't be able to see" I mean "only for SGX EPC resource", but
>> not the
>> > > > control files for the entire MISC cgroup.
>> > > >
>> > > > I replied at the beginning of the previous reply:
>> > > >
>> > > > "
>> > > > Given SGX EPC is just one type of MISC cgroup resources, we cannot
>> > > just
>> > > > disable MISC cgroup as a whole.
>> > > > "
>> > > >
>> > > Sorry I missed this point. below.
>> > >
>> > > > You just need to set the SGX EPC "capacity" to 0 to disable SGX
>> EPC.
>> > > See
>> > > > the comment of @misc_res_capacity:
>> > > >
>> > > > * Miscellaneous resources capacity for the entire machine. 0
>> capacity
>> > > > * means resource is not initialized or not present in the host.
>> > > >
>> > >
>> > > IIUC I don't think the situation we have is either of those cases.
>> For
>> > > our
>> > > case, resource is inited and present on the host but we have
>> allocation
>> > > error for sgx cgroup infra.
>> >
>> > You have calculated the "capacity", but later you failed something and
>> > then reset the "capacity" to 0, i.e., cleanup. What's wrong with
>> that?
>> >
>> > >
>> > > > And "blocking echo sgx_epc ... to those control files" is already
>> > > > sufficient for the purpose of not exposing SGX EPC to userspace,
>> > > correct?
>> > > >
>> > > > E.g., if SGX cgroup is enabled, you can see below when you read
>> "max":
>> > > >
>> > > > # cat /sys/fs/cgroup/my_group/misc.max
>> > > > # <resource1> <max1>
>> > > > sgx_epc ...
>> > > > ...
>> > > >
>> > > > Otherwise you won't be able to see "sgx_epc":
>> > > >
>> > > > # cat /sys/fs/cgroup/my_group/misc.max
>> > > > # <resource1> <max1>
>> > > > ...
>> > > >
>> > > > And when you try to write the "max" for "sgx_epc", you will hit
>> error:
>> > > >
>> > > > # echo "sgx_epc 100" > /sys/fs/cgroup/my_group/misc.max
>> > > > # ... echo: write error: Invalid argument
>> > > >
>> > > > The above applies to all the control files. To me this is pretty
>> much
>> > > > means "SGX EPC is disabled" or "not supported" for userspace.
>> > > >
>> > > You are right, capacity == 0 does block echoing max and users see an
>> > > error
>> > > if they do that. But 1) doubt you literately wanted "SGX EPC is
>> > > disabled"
>> > > and make it unsupported in this case,
>> >
>> > I don't understand. Something failed during SGX cgroup
>> initialization,
>> > you _literally_ cannot continue to support it.
>> >
>> >
>>
>> Then we should just return -ENOMEM from sgx_init() when sgx cgroup
>> initialization fails?
>> I thought we only disable SGX cgroup support. SGX can still run.
>
> I am not sure how you got this conclusion. I specifically said something
> failed during SGX "cgroup" initialization, so only SGX "cgroup" needs to
> be disabled, not SGX as a whole.
>
>>
>> > > 2) even if we accept this is "sgx
>> > > cgroup disabled" I don't see how it is much better user experience
>> than
>> > > current solution or really helps user better.
>> >
>> > In your way, the userspace is still able to see "sgx_epc" in control
>> > files
>> > and is able to update them. So from userspace's perspective SGX
>> cgroup
>> > is
>> > enabled, but obviously updating to "max" doesn't have any impact.
>> This
>> > will confuse userspace.
>> >
>> > >
>>
>> Setting capacity to zero also confuses user space. Some application may
>> rely on this file to know the capacity.
>
>
> Why??
>
> Are you saying before this SGX cgroup patchset those applications cannot
> run?
>
>>
>> > > Also to implement this approach, as you mentioned, we need
>> workaround
>> > > the
>> > > fact that misc_try_charge() fails when capacity set to zero, and
>> adding
>> > > code to return root always?
>> >
>> > Why this is a problem?
>> >
>>
>> It changes/overrides the the original meaning of capacity==0: no one can
>> allocate if capacity is zero.
>
> Why??
>
> Are you saying before this series, no one can allocate EPC page?
>
>>
>> > > So it seems like more workaround code to just
>> > > make it work for a failing case no one really care much and end
>> result
>> > > is
>> > > not really much better IMHO.
>> >
>> > It's not workaround, it's the right thing to do.
>> >
>> > The result is userspace will see it being disabled when kernel
>> disables
>> > it.
>> >
>> >
>> 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 does
not mean sgx epc cgroup not enabled. That's also consistent with the
behavior try_charge() fails if capacity is zero.

>>
>> 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.

>
>> So in future if we want really disable sgx_epc cgroup specifically
>> we should not use capacity. Therefore your approach is not
>> extensible/reusable.
>>
>> Given this is a rare corner case caused by configuration, we can only do
>> as much as possible IMHO, not trying to implement a perfect solution at
>> the moment. Maybe BUG_ON() is more appropriate?
>>
>
> I think I will reply this thread for the last time:
>
> I don't have strong opinion to against using BUG_ON() when you fail to
> allocate workqueue. If you choose to do this, I'll leave to others.
>
> If you want to "disable SGX cgroup" when you fail to allocate workqueue,
> reset the "capacity" to 0 to disable it.

Unless I hear otherwise, I'll revert to BUG_ON().

Thanks
Haitao