Re: [PATCH v5 12/18] x86/sgx: Add EPC OOM path to forcefully reclaim EPC

From: "Huang, Kai" <kai.huang@intel.com>
To: "Christopherson,, Sean" <seanjc@google.com>,
	"haitao.huang@linux.intel.com" <haitao.huang@linux.intel.com>
Cc: "Zhang, Bo" <zhanb@microsoft.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	"yangjie@microsoft.com" <yangjie@microsoft.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"tj@kernel.org" <tj@kernel.org>,
	"anakrish@microsoft.com" <anakrish@microsoft.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
	"Mehta, Sohil" <sohil.mehta@intel.com>,
	"bp@alien8.de" <bp@alien8.de>, "x86@kernel.org" <x86@kernel.org>,
	"kristen@linux.intel.com" <kristen@linux.intel.com>
Subject: Re: [PATCH v5 12/18] x86/sgx: Add EPC OOM path to forcefully reclaim EPC
Date: Mon, 16 Oct 2023 21:09:52 +0000	[thread overview]
Message-ID: <35a7fde056037a40b3b4b170e2ecd45bf8c4ba9f.camel@intel.com> (raw)
In-Reply-To: <op.2cxatlafwjvjmi@hhuan26-mobl.amr.corp.intel.com>

> 
> 
>  From this perspective, I think the current implementation is  
> "well-defined": EPC cgroup limits for VMs are only enforced at VM launch  
> time, not runtime. In practice,  SGX VM can be launched only with fixed  
> EPC size and all those EPCs are fully committed to the VM once launched.  
> Because of that, I imagine people are using VMs to primarily partition the  
> physical EPCs, i.e, the static size itself is the 'limit' for the workload  
> of a single VM and not expecting EPCs taken away at runtime.
> 
> So killing does not really add much value for the existing usages IIUC.

It's not about adding value to the existing usages, it's about fixing the issue
when we lower the EPC limit to a point that is less than total virtual EPC size.

It's a design issue, or simply a bug in the current implementation we need to
fix.

> 
> That said, I don't anticipate adding the enforcement of killing VMs at  
> runtime would break such usages as admin/user can simply choose to set the  
> limit equal to the static size to launch the VM and forget about it.
> 
> Given that, I'll propose an add-on patch to this series as RFC and have  
> some feedback from community before we decide if that needs be included in  
> first version or we can skip it until we have EPC reclaiming for VMs.

I don't understand what is the "add-on" patch you are talking about.

I mentioned the "typical deployment thing" is that can help us understand which
algorithm is better way to select the victim.  But no matter what we choose, we
still need to fix the bug mentioned above here.

I really think you should just go this simple way: 

When you want to take EPC back from VM, kill the VM.

Another bad thing about "just removing EPC pages from VM" is the enclaves in the
VM would suffer "sudden lose of EPC", or even worse, suffer it at a high
frequency.  Although we depend on that for supporting SGX VM live migration, but
that needs to avoided if possible.