Messages in this thread | | | From | "Huang, Kai" <> | Subject | Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup | Date | Tue, 23 Apr 2024 22:41:29 +0000 |
| |
On Wed, 2024-04-24 at 00:27 +0300, Jarkko Sakkinen wrote: > On Tue Apr 23, 2024 at 8:08 PM EEST, Reinette Chatre wrote: > > Hi Kai, > > > > On 4/23/2024 4:50 AM, Huang, Kai wrote: > > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > > > > index b65ab214bdf5..2340a82fa796 100644 > > > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > > > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > > > > @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl, > > > > } > > > > > > > > mutex_unlock(&encl->lock); > > > > + > > > > + if (need_resched()) > > > > + cond_resched(); > > > > } > > > > > > > > ret = 0; > > > > @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl, > > > > entry->type = page_type; > > > > > > > > mutex_unlock(&encl->lock); > > > > + > > > > + if (need_resched()) > > > > + cond_resched(); > > > > } > > > > > > > > ret = 0; > > > > @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl, > > > > kfree(entry); > > > > > > > > mutex_unlock(&encl->lock); > > > > + > > > > + if (need_resched()) > > > > + cond_resched(); > > > > } > > > > > > > > > > You can remove the need_reshced() in all 3 places above but just call > > > cond_resched() directly. > > > > > > > This change will call cond_resched() after dealing with each page in a > > potentially large page range (cover mentions 30GB but we have also had to > > make optimizations for enclaves larger than this). Adding a cond_resched() > > here will surely placate the soft lockup detector, but we need to take care > > how changes like this impact the performance of the system and having actions > > on these page ranges take much longer than necessary. > > For reference, please see 7b72c823ddf8 ("x86/sgx: Reduce delay and interference > > of enclave release") that turned frequent cond_resched() into batches > > to address performance issues.
Ah I didn't know this. Thanks for the info.
> > > > It looks to me like the need_resched() may be a quick check that can be used > > to improve performance? > >
Perhaps? My assumption is eventually cond_resched() will do similar check of need_resched() but I am not entirely sure about of that.
Reading the code, it seems cond_resched() eventually does should_resched(). The generic version indeed does similar check of need_resched() but it seems the x86 version has a different implementation.
> > I am not familiar with all use cases that need to be > > considered to determine if a batching solution may be needed.
Looks at least the REMOVE_PAGES ioctls() could have the same impact to the performance downgrade problem mentioned in commit 7b72c823ddf8 ("x86/sgx: Reduce delay and interference of enclave release"), but I guess it's acceptable to fix softlockup first and then improve performance if there's someone hit any real issue.
> > Ya, well no matter it is the reasoning will need to be documented > because this should have symmetry with sgx_ioc_enclave_add_pages() > (see my response to Kai).
Yeah I was actually going to mention this, but somehow I didn't choose to.
> > I because this makes dealing with need_resched() a change in code > even if it is left out as a side-effect, I'd support of not removing > which means adding need_resched() as a side-effect.
I am fine with keeping the need_resched().
| |