Re: [PATCH v9 08/15] x86/sgx: Implement EPC reclamation flows for cgroup

[Haitao Huang]

On Wed, 21 Feb 2024 05:00:27 -0600, Huang, Kai <kai.huang@xxxxxxxxx> wrote:

> On Wed, 2024-02-21 at 00:44 -0600, Haitao Huang wrote:
> > [...]
> > >
> > > Here the @nr_to_scan is reduced by the number of pages that are
> > > isolated, but
> > > not actually reclaimed (which is reflected by @cnt).
> > >
> > > IIUC, looks you want to make this function do "each cycle" as what you
> > > mentioned
> > > in the v8 [1]:
> > >
> > > 	I tested with that approach and found we can only target number of
> > > pages
> > > 	attempted to reclaim not pages actually reclaimed due to the
> > > uncertainty
> > > 	of how long it takes to reclaim pages. Besides targeting number of
> > > 	scanned pages for each cycle is also what the ksgxd does.
> > >
> > > 	If we target actual number of pages, sometimes it just takes too  
> > long.
> > > I
> > > 	saw more timeouts with the default time limit when running parallel
> > > 	selftests.
> > >
> > > I am not sure what does "sometimes it just takes too long" mean, but
> > > what I am
> > > thinking is you are trying to do some perfect but yet complicated code
> > > here.
> >
> > I think what I observed was that the try_charge() would block too long
> > before getting chance of schedule() to yield, causing more timeouts than
> > necessary.
> > I'll do some re-test to be sure.
>
> Looks this is a valid information that can be used to justify whatever  
> you are
> implementing in the EPC cgroup reclaiming function(s).
I'll add some comments. Was assuming this is just following the old design  
as ksgxd.
There were some comments at the beginning of  
sgx_epc_cgrooup_reclaim_page().
        /*
         * Attempting to reclaim only a few pages will often fail and is
         * inefficient, while reclaiming a huge number of pages can result  
in
         * soft lockups due to holding various locks for an extended  
duration.
         */
        unsigned int nr_to_scan = SGX_NR_TO_SCAN;

I think it can be improved to emphasize we only "attempt" to finish  
scanning fixed number of pages for reclamation, not enforce number of  
pages successfully reclaimed.

> > >
> > > For instance, I don't think selftest reflect the real workload, and I
> > > believe
> > > adjusting the limit of a given EPC cgroup shouldn't be a frequent
> > > operation,
> > > thus it is acceptable to use some easy-maintain code but less perfect
> > > code.
> > >
> > > Here I still think having @nr_to_scan as a pointer is  
> > over-complicated.
> > > For
> > > example, we can still let sgx_reclaim_pages() to always scan
> > > SGX_NR_TO_SCAN
> > > pages, but give up when there's enough pages reclaimed or when the EPC
> > > cgroup
> > > and its descendants have been looped:
> > >
> > > unsigned int sgx_epc_cgroup_reclaim_pages(struct misc_cg *root)
> > > {
> > > 	unsigned int cnt = 0;
> > > 	...
> > >
> > > 	css_for_each_descendant_pre(pos, css_root) {
> > > 		...
> > > 		epc_cg = sgx_epc_cgroup_from_misc_cg(css_misc(pos));
> > > 		cnt += sgx_reclaim_pages(&epc_cg->lru);
> > >
> > > 		if (cnt >= SGX_NR_TO_SCAN)
> > > 			break;
> > > 	}
> > >
> > > 	...
> > > 	return cnt;
> > > }
> > >
> > > Yeah it may reclaim more than SGX_NR_TO_SCAN when the loop actually
> > > reaches any
> > > descendants, but that should be rare and we don't care that much, do  
> > we?
> > >
> > I assume you meant @cnt here to be number of pages actually reclaimed.
>
> Yes.
>
> > This could cause  sgx_epc_cgroup_reclaim_pages() block too long as @cnt
> > may always be zero (all pages are too young) and you have to loop all
> > descendants.
>
> I am not sure whether this is a valid point.
>
> For example, your change in patch 10 "x86/sgx: Add EPC reclamation in  
> cgroup
> try_charge()" already loops all descendants in below code:
>
> +		if (sgx_epc_cgroup_lru_empty(epc_cg->cg))
> +			return -ENOMEM;

I meant looping all descendants for reclamation which is expensive and we  
want to avoid. Not just checking emptiness of LRUs.

> Anyway, I can see all these can be justification to your  
> design/implementation.
> My point is please put these justification in changelog/comments so that  
> we can
> actually understand.
Yes, will add clarifying comments.
Thanks