On Tue, Nov 14, 2017 at 09:33:34PM +0200, Jarkko Sakkinen wrote: > On Tue, Nov 07, 2017 at 11:05:08AM -0800, Dave Hansen wrote: > > On 11/07/2017 10:47 AM, Jarkko Sakkinen wrote: > > > On Mon, Nov 06, 2017 at 07:54:00AM -0800, Dave Hansen wrote: > > >> On 10/10/2017 07:32 AM, Jarkko Sakkinen wrote: > > >>> +static LIST_HEAD(sgx_free_list); > > >>> +static DEFINE_SPINLOCK(sgx_free_list_lock); > > >> > > >> Is this a global list? Will this be a scalability problem on larger > > >> systems? > > > > > > It will be need to be refined for NUMA. > > > > > > In addition, per-CPU caches would probably make sense. > > > > > > For simplicity, I would keep it as it is up until the driver is in the > > > mainline. > > > > FWIW, I don't think we should merge things that aren't performant. > > Global locks like this are just intolerable. You can add this as a > > later patch, but please don't merge stuff like this. > > The back pointer to struct sgx_encl_page from struct sgx_epc_page is > useless. I've had this in backlog already long time ago but had forgot > it as I've been mostly working with the launch infrastructure lately. > > Your comment worked as kind of a reminder of that. Thank you. > > Once that field is removed the whole struct is useless and EPC bank > converges to an array. With an array the driver should be able reserve > pages without a global lock. > > I've started writing a patch to make all this happen and it is > progressing really well. I'm planning to include this change to v6. > As it simplifies code I'm going to squash it as part of the initial > driver patch. > > How does this sound? Every sgx_epc_bank will have a bitmap array for reserved EPC. Every unswapped sgx_encl_page will have a pointer containing physical address of the EPC page in the upper bits and bank number in the lower bits (like sgx_epc_page has now in the 'pa' field). This layout does not require a global lock. /Jarkko
