[v5,09/18] x86/sgx: Store struct sgx_encl when allocating new VA pages

  From: Sean Christopherson <sean.j.christopherson@intel.com>

In a later patch, when a cgroup has exceeded the max capacity for EPC
pages, it may need to identify and OOM kill a less active enclave to
make room for other enclaves within the same group. Such a victim
enclave would have no active pages other than the unreclaimable Version
Array (VA) and SECS pages.  Therefore, the cgroup needs examine its
unreclaimable page list, and finding an enclave given a SECS page or a
VA page. This will require a backpointer from a page to an enclave,
which is not available for VA pages.

Because struct sgx_epc_page instances of VA pages are not owned by an
sgx_encl_page instance, mark their owner as sgx_encl: pass the struct
sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(),
which will store this value in the owner field of the struct
sgx_epc_page.  In a later patch, VA pages will be placed in an
unreclaimable queue that can be examined by the cgroup to select the OOM
killed enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Co-developed-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Cc: Sean Christopherson <seanjc@google.com>
---
V5:
- Fixed some comments in code (Jarkko)

V4:
- Changes needed for patch reordering
- Revised commit messages (Jarkko)
---
 arch/x86/kernel/cpu/sgx/encl.c  |  5 +++--
 arch/x86/kernel/cpu/sgx/encl.h  |  2 +-
 arch/x86/kernel/cpu/sgx/ioctl.c |  2 +-
 arch/x86/kernel/cpu/sgx/main.c  | 20 ++++++++++----------
 arch/x86/kernel/cpu/sgx/sgx.h   |  7 ++++++-
 5 files changed, 21 insertions(+), 15 deletions(-)
  

On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> In a later patch, when a cgroup has exceeded the max capacity for EPC
> pages, it may need to identify and OOM kill a less active enclave to
> make room for other enclaves within the same group. Such a victim
> enclave would have no active pages other than the unreclaimable Version
> Array (VA) and SECS pages.  Therefore, the cgroup needs examine its
> unreclaimable page list, and finding an enclave given a SECS page or a
> VA page. This will require a backpointer from a page to an enclave,
> which is not available for VA pages.
> 
> Because struct sgx_epc_page instances of VA pages are not owned by an
> sgx_encl_page instance, mark their owner as sgx_encl: pass the struct
> sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(),
> which will store this value in the owner field of the struct
> sgx_epc_page.  In a later patch, VA pages will be placed in an
> unreclaimable queue that can be examined by the cgroup to select the OOM
> killed enclave.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> Cc: Sean Christopherson <seanjc@google.com>
> 

[...]

> @@ -562,7 +562,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
>  	for ( ; ; ) {
>  		page = __sgx_alloc_epc_page();
>  		if (!IS_ERR(page)) {
> -			page->owner = owner;
> +			page->encl_page = owner;

Looks using 'encl_page' is arbitrary.

Also actually for virtual EPC page the owner is set to the 'sgx_vepc' instance.

>  			break;
>  		}
>  
> @@ -607,7 +607,7 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
>  
>  	spin_lock(&node->lock);
>  
> -	page->owner = NULL;
> +	page->encl_page = NULL;

Ditto.

>  	if (page->poison)
>  		list_add(&page->list, &node->sgx_poison_page_list);
>  	else
> @@ -642,7 +642,7 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
>  	for (i = 0; i < nr_pages; i++) {
>  		section->pages[i].section = index;
>  		section->pages[i].flags = 0;
> -		section->pages[i].owner = NULL;
> +		section->pages[i].encl_page = NULL;
>  		section->pages[i].poison = 0;
>  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>  	}
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 764cec23f4e5..5110dd433b80 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -68,7 +68,12 @@ struct sgx_epc_page {
>  	unsigned int section;
>  	u16 flags;
>  	u16 poison;
> -	struct sgx_encl_page *owner;
> +
> +	/* Possible owner types */
> +	union {
> +		struct sgx_encl_page *encl_page;
> +		struct sgx_encl *encl;
> +	};

Sadly for virtual EPC page the owner is set to the 'sgx_vepc' instance it
belongs to.

Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page, perhaps we
should do below?

	union {
		struct sgx_encl_page *encl_page;
		struct sgx_encl *encl;
		struct sgx_vepc *vepc;
		void *owner;
	};

And in sgx_{alloc|free}_epc_page() we can use 'owner' instead.
  

On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> In a later patch, when a cgroup has exceeded the max capacity for EPC
> pages, it may need to identify and OOM kill a less active enclave to
> make room for other enclaves within the same group. Such a victim
> enclave would have no active pages other than the unreclaimable Version
> Array (VA) and SECS pages.  
> 

What does "no active pages" mean?

A "less active enclave" doesn't necessarily mean it has "no active pages"?


> Therefore, the cgroup needs examine its
			^
			needs to

> unreclaimable page list, and finding an enclave given a SECS page or a
				^
				find

> VA page. This will require a backpointer from a page to an enclave,
> which is not available for VA pages.
> 
> Because struct sgx_epc_page instances of VA pages are not owned by an
> sgx_encl_page instance, mark their owner as sgx_encl: pass the struct
> sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(),
> which will store this value in the owner field of the struct
> sgx_epc_page.  
> 

IMHO this paragraph is hard to understand and can be more concise:

One VA page can be shared by multiple enclave pages thus cannot be associated
with any 'struct sgx_encl_page' instance.  Set the owner of VA page to the
enclave instead.


> In a later patch, VA pages will be placed in an
> unreclaimable queue that can be examined by the cgroup to select the OOM
> killed enclave.

The code to "place the VA page to unreclaimable queue" has been done in earlier
patch ("x86/sgx: Introduce EPC page states").  Just the unreclaimable list isn't
introduced yet.  I think you should just introduce it first then you can get rid
of those "in a later patch" staff.

And nit: please use "unreclaimable list" consistently (not queue).


Btw, probably a dumb question:

Theoretically if you only need to find a victim enclave you don't need to put VA
pages to the unreclaimable list, because those VA pages will be freed anyway
when enclave is killed.  So keeping VA pages in the list is for accounting all
the pages that the cgroup is having?
  

Hi Kai,

On Wed, 27 Sep 2023 06:14:20 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
>> From: Sean Christopherson <sean.j.christopherson@intel.com>
>>
>> In a later patch, when a cgroup has exceeded the max capacity for EPC
>> pages, it may need to identify and OOM kill a less active enclave to
>> make room for other enclaves within the same group. Such a victim
>> enclave would have no active pages other than the unreclaimable Version
>> Array (VA) and SECS pages.  Therefore, the cgroup needs examine its
>> unreclaimable page list, and finding an enclave given a SECS page or a
>> VA page. This will require a backpointer from a page to an enclave,
>> which is not available for VA pages.
>>
>> Because struct sgx_epc_page instances of VA pages are not owned by an
>> sgx_encl_page instance, mark their owner as sgx_encl: pass the struct
>> sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(),
>> which will store this value in the owner field of the struct
>> sgx_epc_page.  In a later patch, VA pages will be placed in an
>> unreclaimable queue that can be examined by the cgroup to select the OOM
>> killed enclave.
>>
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Co-developed-by: Kristen Carlson Accardi <kristen@linux.intel.com>
>> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
>> Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
>> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
>> Cc: Sean Christopherson <seanjc@google.com>
>>
>
> [...]
>
>> @@ -562,7 +562,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void  
>> *owner, bool reclaim)
>>  	for ( ; ; ) {
>>  		page = __sgx_alloc_epc_page();
>>  		if (!IS_ERR(page)) {
>> -			page->owner = owner;
>> +			page->encl_page = owner;
>
> Looks using 'encl_page' is arbitrary.
>
> Also actually for virtual EPC page the owner is set to the 'sgx_vepc'  
> instance.
>
>>  			break;
>>  		}
>>
>> @@ -607,7 +607,7 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
>>
>>  	spin_lock(&node->lock);
>>
>> -	page->owner = NULL;
>> +	page->encl_page = NULL;
>
> Ditto.
>
>>  	if (page->poison)
>>  		list_add(&page->list, &node->sgx_poison_page_list);
>>  	else
>> @@ -642,7 +642,7 @@ static bool __init sgx_setup_epc_section(u64  
>> phys_addr, u64 size,
>>  	for (i = 0; i < nr_pages; i++) {
>>  		section->pages[i].section = index;
>>  		section->pages[i].flags = 0;
>> -		section->pages[i].owner = NULL;
>> +		section->pages[i].encl_page = NULL;
>>  		section->pages[i].poison = 0;
>>  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>>  	}
>> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h  
>> b/arch/x86/kernel/cpu/sgx/sgx.h
>> index 764cec23f4e5..5110dd433b80 100644
>> --- a/arch/x86/kernel/cpu/sgx/sgx.h
>> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
>> @@ -68,7 +68,12 @@ struct sgx_epc_page {
>>  	unsigned int section;
>>  	u16 flags;
>>  	u16 poison;
>> -	struct sgx_encl_page *owner;
>> +
>> +	/* Possible owner types */
>> +	union {
>> +		struct sgx_encl_page *encl_page;
>> +		struct sgx_encl *encl;
>> +	};
>
> Sadly for virtual EPC page the owner is set to the 'sgx_vepc' instance it
> belongs to.
>
> Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page,  
> perhaps we
> should do below?
>
> 	union {
> 		struct sgx_encl_page *encl_page;
> 		struct sgx_encl *encl;
> 		struct sgx_vepc *vepc;
> 		void *owner;
> 	};
>
> And in sgx_{alloc|free}_epc_page() we can use 'owner' instead.
>

As I mentioned in cover letter and change log in 11/18, this series does  
not track virtual EPC.
We can add vepc field into the union in future if such tracking is needed.  
Don't think "void *owner" is needed though.

Thanks
Haitao
  

On Wed, 2023-09-27 at 10:35 -0500, Haitao Huang wrote:
> > > +
> > > +	/* Possible owner types */
> > > +	union {
> > > +		struct sgx_encl_page *encl_page;
> > > +		struct sgx_encl *encl;
> > > +	};
> > 
> > Sadly for virtual EPC page the owner is set to the 'sgx_vepc' instance it
> > belongs to.
> > 
> > Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page,  
> > perhaps we
> > should do below?
> > 
> >  	union {
> >  		struct sgx_encl_page *encl_page;
> >  		struct sgx_encl *encl;
> >  		struct sgx_vepc *vepc;
> >  		void *owner;
> >  	};
> > 
> > And in sgx_{alloc|free}_epc_page() we can use 'owner' instead.
> > 
> 
> As I mentioned in cover letter and change log in 11/18, this series does  
> not track virtual EPC.

It's not about how does the cover letter says.  We cannot ignore the fact that
currently virtual EPC uses owner too.

But given the virtual EPC code currently doesn't use the owner, I can live with
not having the 'vepc' member in the union now.

> We can add vepc field into the union in future if such tracking is needed.  
> Don't think "void *owner" is needed though.

As mentioned, using 'encl_page' arbitrarily in sgx_alloc_epc_page() doesn't look
nice.  Do you have example in the current kernel code to prove it is acceptable?
  

On Wed, 27 Sep 2023 16:21:19 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Wed, 2023-09-27 at 10:35 -0500, Haitao Huang wrote:
>> > > +
>> > > +	/* Possible owner types */
>> > > +	union {
>> > > +		struct sgx_encl_page *encl_page;
>> > > +		struct sgx_encl *encl;
>> > > +	};
>> >
>> > Sadly for virtual EPC page the owner is set to the 'sgx_vepc'  
>> instance it
>> > belongs to.
>> >
>> > Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page,>  
>> perhaps we
>> > should do below?
>> >
>> >  	union {
>> >  		struct sgx_encl_page *encl_page;
>> >  		struct sgx_encl *encl;
>> >  		struct sgx_vepc *vepc;
>> >  		void *owner;
>> >  	};
>> >
>> > And in sgx_{alloc|free}_epc_page() we can use 'owner' instead.
>> >
>>
>> As I mentioned in cover letter and change log in 11/18, this series does 
>> not track virtual EPC.
>
> It's not about how does the cover letter says.  We cannot ignore the  
> fact that
> currently virtual EPC uses owner too.
>
> But given the virtual EPC code currently doesn't use the owner, I can  
> live with
> not having the 'vepc' member in the union now.

Ah, I forgot even though we don't use the owner field assigned by vepc, it  
is still passed into sgx_alloc/free_epc_page().

Will add back "void* owner" and use it for assignment inside  
sgx_alloc/free_epc_page().

Thanks

Haitao
  

On Fri, 2023-09-29 at 10:06 -0500, Haitao Huang wrote:
> On Wed, 27 Sep 2023 16:21:19 -0500, Huang, Kai <kai.huang@intel.com> wrote:
> 
> > On Wed, 2023-09-27 at 10:35 -0500, Haitao Huang wrote:
> > > > > +
> > > > > +	/* Possible owner types */
> > > > > +	union {
> > > > > +		struct sgx_encl_page *encl_page;
> > > > > +		struct sgx_encl *encl;
> > > > > +	};
> > > > 
> > > > Sadly for virtual EPC page the owner is set to the 'sgx_vepc'  
> > > instance it
> > > > belongs to.
> > > > 
> > > > Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page,>  
> > > perhaps we
> > > > should do below?
> > > > 
> > > >  	union {
> > > >  		struct sgx_encl_page *encl_page;
> > > >  		struct sgx_encl *encl;
> > > >  		struct sgx_vepc *vepc;
> > > >  		void *owner;
> > > >  	};
> > > > 
> > > > And in sgx_{alloc|free}_epc_page() we can use 'owner' instead.
> > > > 
> > > 
> > > As I mentioned in cover letter and change log in 11/18, this series does 
> > > not track virtual EPC.
> > 
> > It's not about how does the cover letter says.  We cannot ignore the  
> > fact that
> > currently virtual EPC uses owner too.
> > 
> > But given the virtual EPC code currently doesn't use the owner, I can  
> > live with
> > not having the 'vepc' member in the union now.
> 
> Ah, I forgot even though we don't use the owner field assigned by vepc, it  
> is still passed into sgx_alloc/free_epc_page().
> 
> Will add back "void* owner" and use it for assignment inside  
> sgx_alloc/free_epc_page().
> 
> 

And also sgx_setup_epc_section().
  

On Wed, 27 Sep 2023 06:35:57 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
>> From: Sean Christopherson <sean.j.christopherson@intel.com>
>>
>> In a later patch, when a cgroup has exceeded the max capacity for EPC
>> pages, it may need to identify and OOM kill a less active enclave to
>> make room for other enclaves within the same group. Such a victim
>> enclave would have no active pages other than the unreclaimable Version
>> Array (VA) and SECS pages.
>
> What does "no active pages" mean?
>

EPC pages in use.

> A "less active enclave" doesn't necessarily mean it has "no active  
> pages"?
>

I'll rephrase the above sentences

>
>> Therefore, the cgroup needs examine its
> 			^
> 			needs to
>
>> unreclaimable page list, and finding an enclave given a SECS page or a
> 				^
> 				find
>
>> VA page. This will require a backpointer from a page to an enclave,
>> which is not available for VA pages.
>>
>> Because struct sgx_epc_page instances of VA pages are not owned by an
>> sgx_encl_page instance, mark their owner as sgx_encl: pass the struct
>> sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(),
>> which will store this value in the owner field of the struct
>> sgx_epc_page.
>
> IMHO this paragraph is hard to understand and can be more concise:
>
> One VA page can be shared by multiple enclave pages thus cannot be  
> associated
> with any 'struct sgx_encl_page' instance.  Set the owner of VA page to  
> the
> enclave instead.
>
>

Agreed

>> In a later patch, VA pages will be placed in an
>> unreclaimable queue that can be examined by the cgroup to select the OOM
>> killed enclave.
>
> The code to "place the VA page to unreclaimable queue" has been done in  
> earlier
> patch ("x86/sgx: Introduce EPC page states").  Just the unreclaimable  
> list isn't
> introduced yet.  I think you should just introduce it first then you can  
> get rid
> of those "in a later patch" staff.
>

I hope I was able to clarify to you in other threads that VA pages are not  
placed in any queue/list until [PATCH v5 11/18] x86/sgx: store  
unreclaimable pages in LRU lists.

This patch is the first one to implement tracking for unreclaimable pages.  
I'll add that as a transition hint.

> And nit: please use "unreclaimable list" consistently (not queue).
>

Yes will do

>
> Btw, probably a dumb question:
>
> Theoretically if you only need to find a victim enclave you don't need  
> to put VA
> pages to the unreclaimable list, because those VA pages will be freed  
> anyway
> when enclave is killed.  So keeping VA pages in the list is for  
> accounting all
> the pages that the cgroup is having?

Yes basically tracking them in cgroups as they are allocated.

VAs and SECS may also come and go as swapping/unswapping happens. But if a  
cgroup is OOM, and all reclaimables are gone (swapped out), it'd have to  
reclaim VAs/SECs in the same cgroup starting from the front of the LRU  
list. To reclaim a VA/SECS, it identifies the enclave from the owner of  
the VA/SECS page and kills it, as killing enclave is the only way to  
reclaim VA/SECS pages.
  

On Tue, 2023-10-03 at 01:45 -0500, Haitao Huang wrote:
> > 
> > Btw, probably a dumb question:
> > 
> > Theoretically if you only need to find a victim enclave you don't need  
> > to put VA
> > pages to the unreclaimable list, because those VA pages will be freed  
> > anyway
> > when enclave is killed.  So keeping VA pages in the list is for  
> > accounting all
> > the pages that the cgroup is having?
> 
> Yes basically tracking them in cgroups as they are allocated.
> 
> VAs and SECS may also come and go as swapping/unswapping happens. But if a  
> cgroup is OOM, and all reclaimables are gone (swapped out), it'd have to  
> reclaim VAs/SECs in the same cgroup starting from the front of the LRU  
> list. To reclaim a VA/SECS, it identifies the enclave from the owner of  
> the VA/SECS page and kills it, as killing enclave is the only way to  
> reclaim VA/SECS pages.

To kill enclave you just need to track SECS in  the unreclaimable list.  

Only when you want to account the total EPC pages via some list you _probably_
need to track VA as well.  But I am not quite sure about this either.
  

On Tue, 03 Oct 2023 15:07:42 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Tue, 2023-10-03 at 01:45 -0500, Haitao Huang wrote:
>> >
>> > Btw, probably a dumb question:
>> >
>> > Theoretically if you only need to find a victim enclave you don't need 
>> > to put VA
>> > pages to the unreclaimable list, because those VA pages will be freed 
>> > anyway
>> > when enclave is killed.  So keeping VA pages in the list is for>  
>> accounting all
>> > the pages that the cgroup is having?
>>
>> Yes basically tracking them in cgroups as they are allocated.
>>
>> VAs and SECS may also come and go as swapping/unswapping happens. But  
>> if acgroup is OOM, and all reclaimables are gone (swapped out), it'd  
>> have toreclaim VAs/SECs in the same cgroup starting from the front of  
>> the LRUlist. To reclaim a VA/SECS, it identifies the enclave from the  
>> owner ofthe VA/SECS page and kills it, as killing enclave is the only  
>> way toreclaim VA/SECS pages.
>
> To kill enclave you just need to track SECS in  the unreclaimable list.  
> Only when you want to account the total EPC pages via some list you  
> _probably_
> need to track VA as well.  But I am not quite sure about this either.

There is a case where even SECS is paged out for an enclave with all  
reclaimables out. So cgroup needs to track each page used by an enclave  
and kill enclave when cgroup needs to lower usage by evicting an VA or  
SECS page.
There were some discussion on paging out VAs without killing enclaves but  
it'd be complicated and not implemented yet.

BTW, I need clarify tracking pages which is done by LRUs vs usage  
accounting which is done by charge/uncharge to misc. To me tracking is for  
reclaiming not accounting. Also vEPCs not tracked at all but they are  
accounted for.

Haitao
  

On Wed, 2023-10-04 at 10:03 -0500, Haitao Huang wrote:
> On Tue, 03 Oct 2023 15:07:42 -0500, Huang, Kai <kai.huang@intel.com> wrote:
> 
> > On Tue, 2023-10-03 at 01:45 -0500, Haitao Huang wrote:
> > > > 
> > > > Btw, probably a dumb question:
> > > > 
> > > > Theoretically if you only need to find a victim enclave you don't need 
> > > > to put VA
> > > > pages to the unreclaimable list, because those VA pages will be freed 
> > > > anyway
> > > > when enclave is killed.  So keeping VA pages in the list is for>  
> > > accounting all
> > > > the pages that the cgroup is having?
> > > 
> > > Yes basically tracking them in cgroups as they are allocated.
> > > 
> > > VAs and SECS may also come and go as swapping/unswapping happens. But  
> > > if acgroup is OOM, and all reclaimables are gone (swapped out), it'd  
> > > have toreclaim VAs/SECs in the same cgroup starting from the front of  
> > > the LRUlist. To reclaim a VA/SECS, it identifies the enclave from the  
> > > owner ofthe VA/SECS page and kills it, as killing enclave is the only  
> > > way toreclaim VA/SECS pages.
> > 
> > To kill enclave you just need to track SECS in  the unreclaimable list.  
> > Only when you want to account the total EPC pages via some list you  
> > _probably_
> > need to track VA as well.  But I am not quite sure about this either.
> 
> There is a case where even SECS is paged out for an enclave with all  
> reclaimables out. 
> 

Yes.  But this essentially means these enclaves are not active, thus shouldn't
be the victim of OOM?

> So cgroup needs to track each page used by an enclave  
> and kill enclave when cgroup needs to lower usage by evicting an VA or  
> SECS page.

Let's discuss more on tracking SECS on unreclaimable list only.

Could we assume that when the OOM wants to pick up a victim to serve the new
enclave, there must be at least another one *active* enclave which still has the
SECS page in EPC?

If yes, that enclave will be selected as victim.

If not, then no other enclave will be selected as victim.  Instead, only the new
enclave which is requesting more EPC will be selected, because it's SECS is on
the unreclaimable list.

Somehow this is unacceptable, thus we need to track VA pages too in order to
kill other inactive enclave?

> There were some discussion on paging out VAs without killing enclaves but  
> it'd be complicated and not implemented yet.

No we don't involve swapping VA pages now.  It's a separate topic.

> 
> BTW, I need clarify tracking pages which is done by LRUs vs usage  
> accounting which is done by charge/uncharge to misc. To me tracking is for  
> reclaiming not accounting. Also vEPCs not tracked at all but they are  
> accounted for.

I'll review the rest patches.  Thanks.
  

On Wed, 04 Oct 2023 16:13:41 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Wed, 2023-10-04 at 10:03 -0500, Haitao Huang wrote:
>> On Tue, 03 Oct 2023 15:07:42 -0500, Huang, Kai <kai.huang@intel.com>  
>> wrote:
>>
>> > On Tue, 2023-10-03 at 01:45 -0500, Haitao Huang wrote:
>> > > >
>> > > > Btw, probably a dumb question:
>> > > >
>> > > > Theoretically if you only need to find a victim enclave you don't  
>> need
>> > > > to put VA
>> > > > pages to the unreclaimable list, because those VA pages will be  
>> freed
>> > > > anyway
>> > > > when enclave is killed.  So keeping VA pages in the list is for>
>> > > accounting all
>> > > > the pages that the cgroup is having?
>> > >
>> > > Yes basically tracking them in cgroups as they are allocated.
>> > >
>> > > VAs and SECS may also come and go as swapping/unswapping happens.  
>> But
>> > > if acgroup is OOM, and all reclaimables are gone (swapped out), it'd
>> > > have toreclaim VAs/SECs in the same cgroup starting from the front  
>> of
>> > > the LRUlist. To reclaim a VA/SECS, it identifies the enclave from  
>> the
>> > > owner ofthe VA/SECS page and kills it, as killing enclave is the  
>> only
>> > > way toreclaim VA/SECS pages.
>> >
>> > To kill enclave you just need to track SECS in  the unreclaimable  
>> list.
>> > Only when you want to account the total EPC pages via some list you
>> > _probably_
>> > need to track VA as well.  But I am not quite sure about this either.
>>
>> There is a case where even SECS is paged out for an enclave with all
>> reclaimables out.
>
> Yes.  But this essentially means these enclaves are not active, thus  
> shouldn't
> be the victim of OOM?
>

But there are VA pages for the enclave at that moment. So it can be  
candidate for OOM victim.

>> So cgroup needs to track each page used by an enclave
>> and kill enclave when cgroup needs to lower usage by evicting an VA or
>> SECS page.
>
> Let's discuss more on tracking SECS on unreclaimable list only.
>
> Could we assume that when the OOM wants to pick up a victim to serve the  
> new
> enclave, there must be at least another one *active* enclave which still  
> has the
> SECS page in EPC?
>
No, at a given instant when OOM happens, "active" enclave's SECS may not  
be in EPC, but lots of VAs.

OOM := "no reclaimable pages left in the cgroup to reclaim and total usage  
is still at/near limit".



> If yes, that enclave will be selected as victim.
>
> If not, then no other enclave will be selected as victim.  Instead, only  
> the new
> enclave which is requesting more EPC will be selected, because it's SECS  
> is on
> the unreclaimable list.
>

You can't assume the requesting enclave's SECS is in unreclaimable list  
either. Think the request is from #PF in the scenario we fixed the NULL  
pointer of SECS by reloading it.

> Somehow this is unacceptable, thus we need to track VA pages too in  
> order to
> kill other inactive enclave?
>

If we know for sure SECS will always be in EPC, thus tracked in  
unreclaimables, then we probably can do it (see below).
I hope the reason given above is clear.

>> There were some discussion on paging out VAs without killing enclaves  
>> but
>> it'd be complicated and not implemented yet.
>
> No we don't involve swapping VA pages now.  It's a separate topic.
>
Only mentioned it as a kind of constraints impacting current design.

Another potential alternative: we don't reclaim SECS either until OOM and  
only track SECS pages for cgroups. But that would change current behavior.  
And I'm not sure about other consequences, e.g., enclaves theoretically  
can allocate pages (including VA pages) in different cgroups/processes, so  
we may still end up tracking all VA pages for cgroups or we track SECS  
page in all cgroups in which enclave allocated any pages. Let me know your  
thoughts.

>>
>> BTW, I need clarify tracking pages which is done by LRUs vs usage
>> accounting which is done by charge/uncharge to misc. To me tracking is  
>> for
>> reclaiming not accounting. Also vEPCs not tracked at all but they are
>> accounted for.
>
> I'll review the rest patches.  Thanks.


Thank you!
Haitao
  

On Wed, 2023-10-04 at 23:22 -0500, Haitao Huang wrote:
> On Wed, 04 Oct 2023 16:13:41 -0500, Huang, Kai <kai.huang@intel.com> wrote:
> 
> > On Wed, 2023-10-04 at 10:03 -0500, Haitao Huang wrote:
> > > On Tue, 03 Oct 2023 15:07:42 -0500, Huang, Kai <kai.huang@intel.com>  
> > > wrote:
> > > 
> > > > On Tue, 2023-10-03 at 01:45 -0500, Haitao Huang wrote:
> > > > > > 
> > > > > > Btw, probably a dumb question:
> > > > > > 
> > > > > > Theoretically if you only need to find a victim enclave you don't  
> > > need
> > > > > > to put VA
> > > > > > pages to the unreclaimable list, because those VA pages will be  
> > > freed
> > > > > > anyway
> > > > > > when enclave is killed.  So keeping VA pages in the list is for>
> > > > > accounting all
> > > > > > the pages that the cgroup is having?
> > > > > 
> > > > > Yes basically tracking them in cgroups as they are allocated.
> > > > > 
> > > > > VAs and SECS may also come and go as swapping/unswapping happens.  
> > > But
> > > > > if acgroup is OOM, and all reclaimables are gone (swapped out), it'd
> > > > > have toreclaim VAs/SECs in the same cgroup starting from the front  
> > > of
> > > > > the LRUlist. To reclaim a VA/SECS, it identifies the enclave from  
> > > the
> > > > > owner ofthe VA/SECS page and kills it, as killing enclave is the  
> > > only
> > > > > way toreclaim VA/SECS pages.
> > > > 
> > > > To kill enclave you just need to track SECS in  the unreclaimable  
> > > list.
> > > > Only when you want to account the total EPC pages via some list you
> > > > _probably_
> > > > need to track VA as well.  But I am not quite sure about this either.
> > > 
> > > There is a case where even SECS is paged out for an enclave with all
> > > reclaimables out.
> > 
> > Yes.  But this essentially means these enclaves are not active, thus  
> > shouldn't
> > be the victim of OOM?
> > 
> 
> But there are VA pages for the enclave at that moment. So it can be  
> candidate for OOM victim.

Yes.  I am not familiar with how does OOM choose victim, but it seems choosing
inactive enclaves seems more reasonable.


[...]

> > > There were some discussion on paging out VAs without killing enclaves  
> > > but
> > > it'd be complicated and not implemented yet.
> > 
> > No we don't involve swapping VA pages now.  It's a separate topic.
> > 
> Only mentioned it as a kind of constraints impacting current design.
> 
> Another potential alternative: we don't reclaim SECS either until OOM and  
> only track SECS pages for cgroups. But that would change current behavior.  
> And I'm not sure about other consequences, e.g., enclaves theoretically  
> can allocate pages (including VA pages) in different cgroups/processes, so  
> we may still end up tracking all VA pages for cgroups or we track SECS  
> page in all cgroups in which enclave allocated any pages. Let me know your  
> thoughts.

Let's not change current behaviour.  I seriously doubt that is needed.

So it seems to me that what we need is just some way to let the OOM find some
victim enclave.  I am not sure whether "tracking EPC pages in some lists" has
anything to do with cgroup accounting EPC pages, so will take a look the rest of
the patches.