Re: [PATCH v5 13/18] x86/sgx: Expose sgx_reclaim_pages() for use by EPC cgroup

[Huang, Kai]

> > > 
> > > -/*
> > > +/**
> > > + * sgx_reclaim_epc_pages() - Reclaim EPC pages from the consumers
> > > + * @nr_to_scan:		 Number of EPC pages to scan for reclaim
> > > + * @ignore_age:		 Reclaim a page even if it is young
> > > + *
> > >   * Take a fixed number of pages from the head of the active page pool  
> > > and
> > >   * reclaim them to the enclave's private shmem files. Skip the pages,  
> > > which have
> > >   * been accessed since the last scan. Move those pages to the tail of  
> > > active
> > > @@ -292,15 +301,14 @@ static void sgx_reclaimer_write(struct  
> > > sgx_epc_page *epc_page,
> > >   * problematic as it would increase the lock contention too much,  
> > > which would
> > >   * halt forward progress.
> > >   */
> > > -static void sgx_reclaim_pages(void)
> > > +size_t sgx_reclaim_epc_pages(size_t nr_to_scan, bool ignore_age)
> > 
> > 'size_t' looks odd.  Any reason to use it?
> > 
> > Given you only scan 32 at maximum, seems 'int' is good enough?
> > 
> 
> Initially was int.
> Jarkko was suggesting ssize_t. I changed to size_t as this function will  
> never return negative.

Then 'unsigned int'.  We are talking about 32 at max here.

size_t is more suitable for bytes, but we are dealing with number of pages.

Maybe Jarkko could comment why size_t is better.

[...]

> > 
> > >  	i = 0;
> > >  	list_for_each_entry_safe(epc_page, tmp, &iso, list) {
> > >  		encl_page = epc_page->encl_page;
> > > 
> > > -		if (!sgx_reclaimer_age(epc_page))
> > > +		if (i == SGX_NR_TO_SCAN_MAX ||
> > 
> > i == nr_to_scan?
> > 
> Not needed if above for statement fixed for nr_to_scan.
> Anything above MAX will be skipped and put back to LRU.

I believe using nr_to_scan is more logically correct.

[...]


> > 
> > I found this function a little bit odd, given the mixing of 'nr_to_scan',
> > SGX_NR_TO_SCAN and SGX_NR_TO_SCAN_MAX.
> > 
> > From the changelog:
> > 
> > 	1) To take a parameter that specifies the number of pages to scan for
> > 	reclaiming. Define a max value of 32, but scan 16 in the case for the
> > 	global reclaimer (ksgxd).
> > 
> > It appears we want to make this function to scan @nr_to_scan for cgroup,  
> > but
> > still want to scan a fixed value for ksgxd, which is SGX_NR_TO_SCAN.  And
> > @nr_to_scan can be larger than SGX_NR_TO_SCAN but smaller than
> > SGX_NR_TO_SCAN_MAX.
> > 
> > Putting behind the mystery of why above is needed, to achieve it, is it  
> > more
> > clear if we do below?
> > 
> > int __sgx_reclaim_epc_pages(int nr_to_scan, bool ignore_age)
> > {
> > 	struct sgx_backing backing[SGX_NR_TO_SCAN_MAX];
> > 	...
> > 
> > 	if (nr_to_scan > SGX_NR_TO_SCAN_MAX)
> > 		return 0;
> 
> We could set nr_to_scan to MAX but since this is code internal to driver,  
> maybe just make sure callers don't call with bigger numbers.

Please add this check, using WARN_ON_ONCE() if it's better.

Then the code is much easier to review.

> 
> > 
> > 	for (i = 0; i < nr_to_scan; i++) {
> > 		...
> > 	}
> > 
> 
> yes

please fix this up, then ...

> 
> > 	return reclaimed;
> > }
> > 
> > /* This is for ksgxd() */
> > int sgx_reclaim_epc_page(void)
> > {
> > 	return __sgx_reclaim_epc_pages(SGX_NR_TO_SCAN, false);
> > }
> 
> Some maintainers may prefer no wrapping.
> 

... OK.