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

[Haitao Huang]

On Fri, 19 Apr 2024 17:44:59 -0500, Huang, Kai <kai.huang@xxxxxxxxx> wrote:

> On Fri, 2024-04-19 at 13:55 -0500, Haitao Huang wrote:
> > On Thu, 18 Apr 2024 20:32:14 -0500, Huang, Kai <kai.huang@xxxxxxxxx>  
> > wrote:
> >
> > >
> > >
> > > On 16/04/2024 3:20 pm, Haitao Huang wrote:
> > > > From: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> > > >  In cases EPC pages need be allocated during a page fault and the  
> > cgroup
> > > > usage is near its limit, an asynchronous reclamation needs be  
> > triggered
> > > > to avoid blocking the page fault handling.
> > > >  Create a workqueue, corresponding work item and function  
> > definitions
> > > > for EPC cgroup to support the asynchronous reclamation.
> > > >  In case the workqueue allocation is failed during init, disable  
> > cgroup.
> > >
> > > It's fine and reasonable to disable (SGX EPC) cgroup.  The problem is
> > > "exactly what does this mean" isn't quite clear.
> > >
> > First, this is really some corner case most people don't care: during
> > init, kernel can't even allocate a workqueue object. So I don't think we
> > should write extra code to implement some sophisticated solution. Any
> > solution we come up with may just not work as the way user want or solve
> > the real issue due to the fact such allocation failure even happens at
> > init time.
>
> I think for such boot time failure we can either choose directly  
> BUG_ON(),
> or we try to handle it _nicely_, but not half-way.  My experience is
> adding BUG_ON() should be avoided in general, but it might be acceptable
> during kernel boot.  I will leave it to others.
>
>
> [...]
>
> > >
> > > ..., IIUC you choose a (third) solution that is even one more step  
> > back:
> > >
> > > It just makes try_charge() always succeed, but EPC pages are still
> > > managed in the "per-cgroup" list.
> > >
> > > But this solution, AFAICT, doesn't work.  The reason is when you fail  
> > to
> > > allocate EPC page you will do the global reclaim, but now the global
> > > list is empty.
> > >
> > > Am I missing anything?
> >
> > But when cgroups enabled in config, global reclamation starts from root
> > and reclaim from the whole hierarchy if user may still be able to  
> > create.
> > Just that we don't have async/sync per-cgroup reclaim triggered.
>
> OK.  I missed this as it is in a later patch.
>
> > >
> > > So my thinking is, we have two options:
> > >
> > > 1) Modify the MISC cgroup core code to allow the kernel to disable one
> > > particular resource.  It shouldn't be hard, e.g., we can add a
> > > 'disabled' flag to the 'struct misc_res'.
> > >
> > > Hmm.. wait, after checking, the MISC cgroup won't show any control  
> > files
> > > if the "capacity" of the resource is 0:
> > >
> > > "
> > >   * Miscellaneous resources capacity for the entire machine. 0  
> > capacity
> > >   * means resource is not initialized or not present in the host.
> > > "
> > >
> > > So I really suppose we should go with this route, i.e., by just  
> > setting
> > > the EPC capacity to 0?
> > >
> > > Note misc_cg_try_charge() will fail if capacity is 0, but we can make  
> > it
> > > return success by explicitly check whether SGX cgroup is disabled by
> > > using a helper, e.g., sgx_cgroup_disabled().
> > >
> > > And you always return the root SGX cgroup in sgx_get_current_cg() when
> > > sgx_cgroup_disabled() is true.
> > >
> > > And in sgx_reclaim_pages_global(), you do something like:
> > >
> > > 	static void sgx_reclaim_pages_global(..)
> > > 	{
> > > 	#ifdef CONFIG_CGROUP_MISC
> > > 		if (sgx_cgroup_disabled())
> > > 			sgx_reclaim_pages(&sgx_root_cg.lru);
> > > 		else
> > > 			sgx_cgroup_reclaim_pages(misc_cg_root());
> > > 	#else
> > > 		sgx_reclaim_pages(&sgx_global_list);
> > > 	#endif
> > > 	}
> > >
> > > I am perhaps missing some other spots too but you got the idea.
> > >
> > > At last, after typing those, I believe we should have a separate patch
> > > to handle disable SGX cgroup at initialization time.  And you can even
> > > put this patch _somewhere_ after the patch
> > >
> > > 	"x86/sgx: Implement basic EPC misc cgroup functionality"
> > >
> > > and before this patch.
> > >
> > > It makes sense to have such patch anyway, because with it we can  
> > easily
> > > to add a kernel command line 'sgx_cgroup=disabled" if the user wants  
> > it
> > > disabled (when someone has such requirement in the future).
> > >
> >
> > 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 interface  
files, can't really make files not visible.

Anyway, I see this is really a configuration failure case. And we handle  
it without added complexity and do as much as we can gracefully until  
another fatal error happens. So if we need support disable sgx through  
command line in future, then we can also make this failure handling even  
more graceful at that time. Nothing really is lost IMHO.

Originally we put in BUG_ON() but then we changed to handle it based on  
your feedback. I can put BUG_ON() back if we agree that's more appropriate  
at the moment.

@Dave, @Jarkko, @Michal, your thoughts?

Thanks
Haitao