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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <47c86959a6e18730b1dfacecbdad4bad991f7a5b.camel@intel.com>
Date: Thu, 20 Jun 2024 22:52:26 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Mehta, Sohil" <sohil.mehta@...el.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "jarkko@...nel.org" <jarkko@...nel.org>, "x86@...nel.org"
	<x86@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "cgroups@...r.kernel.org"
	<cgroups@...r.kernel.org>, "hpa@...or.com" <hpa@...or.com>,
	"tim.c.chen@...ux.intel.com" <tim.c.chen@...ux.intel.com>,
	"linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>, "mkoutny@...e.com"
	<mkoutny@...e.com>, "haitao.huang@...ux.intel.com"
	<haitao.huang@...ux.intel.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"tj@...nel.org" <tj@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "bp@...en8.de" <bp@...en8.de>
CC: "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "anakrish@...rosoft.com"
	<anakrish@...rosoft.com>, "Zhang, Bo" <zhanb@...rosoft.com>,
	"kristen@...ux.intel.com" <kristen@...ux.intel.com>, "yangjie@...rosoft.com"
	<yangjie@...rosoft.com>, "Li, Zhiquan1" <zhiquan1.li@...el.com>,
	"chrisyan@...rosoft.com" <chrisyan@...rosoft.com>
Subject: Re: [PATCH v15 08/14] x86/sgx: Add basic EPC reclamation flow for
 cgroup

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