linux-kernel - Re: [PATCH v5 13/18] x86/sgx: Expose sgx_reclaim

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <44db76f151a83367069d83e3f9db585f3fa627bf.camel@intel.com>
Date:   Thu, 5 Oct 2023 20:25:02 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "mingo@...hat.com" <mingo@...hat.com>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.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>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "bp@...en8.de" <bp@...en8.de>,
        "haitao.huang@...ux.intel.com" <haitao.huang@...ux.intel.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "tj@...nel.org" <tj@...nel.org>,
        "Mehta, Sohil" <sohil.mehta@...el.com>
CC:     "kristen@...ux.intel.com" <kristen@...ux.intel.com>,
        "anakrish@...rosoft.com" <anakrish@...rosoft.com>,
        "Li, Zhiquan1" <zhiquan1.li@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
        "yangjie@...rosoft.com" <yangjie@...rosoft.com>,
        "Zhang, Bo" <zhanb@...rosoft.com>
Subject: Re: [PATCH v5 13/18] x86/sgx: Expose sgx_reclaim_pages() for use by
 EPC cgroup


> > > 
> > > -/*
> > > +/**
> > > + * 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.