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().
