Re: [PATCH v5 02/18] cgroup/misc: Add SGX EPC resource type and export APIs for SGX driver

[Huang, Kai]

On Tue, 2023-10-03 at 02:00 -0500, Haitao Huang wrote:
> On Wed, 27 Sep 2023 22:59:12 -0500, Huang, Kai <kai.huang@xxxxxxxxx> wrote:
> 
> > On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> > > From: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> > > 
> > > Add SGX EPC memory, MISC_CG_RES_SGX_EPC, to be a valid resource type
> > > for the misc controller.
> > > 
> > > Add per resource type private data so that SGX can store additional per
> > > cgroup data in misc_cg->misc_cg_res[MISC_CG_RES_SGX_EPC].
> > 
> > To be honest I don't quite understand why putting the above two changes  
> > in this
> > patch together with exporting misc_cg_root/parent() below.
> > 
> > Any reason why the above two cannot be done together with patch ("  
> > x86/sgx:
> > Limit process EPC usage with misc cgroup controller"), where these  
> > changes are
> > actually related?
> > 
> > We all already know that a new EPC misc cgroup will be added.  There's  
> > no need
> > to actually introduce the new type here only to justify exporting some  
> > helper
> > functions.
> > 
> 
> I think previous authors intended to separate all prerequisite misc  
> changes from SGX changes.
> I can combine them if maintainers are fine with it.

That's fine.  But IMHO for this particular one I think you are mixing things
together:  Adding SGX EPC resource type and exporting APIs don't have dependency
on the code.

It will be easier to review if you separate this two parts out.  For instance,
at least it's not super clear whether adding a 'priv' is a right move here w/o
looking at the later patches.

Also if you take a look at:

7aef27f0b2a8 ("svm/sev: Register SEV and SEV-ES ASIDs to the misc controller")

Adding the resource type is added together with the implementation.

So I have no problem if you want to split out "adding SGX EPC resource type" out
as a separate patch, but this patch looks should be split. 

> 
> > > 
> > > Export misc_cg_root() so the SGX driver can initialize and add those
> > > additional structures to the root misc cgroup as part of initialization
> > > for EPC cgroup support. This bootstraps the same additional
> > > initialization for non-root cgroups in the 'alloc()' callback added in  
> > > the
> > > previous patch.
> > > 
> > > The SGX driver, as the EPC memory provider, will have a background
> > > worker to reclaim EPC pages to make room for new allocations in the same
> > > cgroup when its usage counter reaches near the limit controlled by the
> > > cgroup and its ancestors. Therefore it needs to do a walk from the
> > > current cgroup up to the root. To enable this walk, move parent_misc()
> > > into misc_cgroup.h and make inline to make this function available to
> > > SGX, rename it to misc_cg_parent(), and update kernel/cgroup/misc.c to
> > > use the new name.
> > 
> > Looks too many details in the above two paragraphs.  Could we have a more
> > concise justification for exporting these two functions?
> > 
> 
> This was added to address Jarkko's question, "why does SGX driver need to  
> do iterative walks?"
> See: https://lore.kernel.org/all/CVHOU5G1SCUT.RCBVZ3W8G2NJ@suppilovahvero/

Agree with Jarkko we need a justification (that what I said above too).  What I
am saying is you can make it more concise.  I can try to do if you want me to.

> 
> > And if it were me, I would put it at a relatively later position (e.g.,  
> > before
> > the patch actually implements EPC cgroup) for better review.  This also  
> > applies
> > to the first patch.
> > 
> 
> I was told to move all prerequisites to the front or separate out.
> 
> https://lore.kernel.org/linux-sgx/CU4H43P3H35X.1BCA3CE4D1250@seitikki/
> 
> 

I don't see any conflict.  Please see the first reply.