Received-SPF: pass (google.com: domain of linux-kernel+bounces-155921-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 24 Apr 2024 00:27:18 +0300
Message-Id: <D0RTJVEHLLZ8.3967ZI3I5UO28@kernel.org>
Cc: "Liu, Shuang" <ls123674@antgroup.com>
Subject: Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's
 ioctl() to avoid softlockup
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Reinette Chatre" <reinette.chatre@intel.com>, "Huang, Kai"
 <kai.huang@intel.com>, "linux-sgx@vger.kernel.org"
 <linux-sgx@vger.kernel.org>, "zhubojun.zbj@antgroup.com"
 <zhubojun.zbj@antgroup.com>, "linux-kernel@vger.kernel.org"
 <linux-kernel@vger.kernel.org>, "dave.hansen@linux.intel.com"
 <dave.hansen@linux.intel.com>
References: <20240423092550.59297-1-zhubojun.zbj@antgroup.com>
 <20240423092550.59297-2-zhubojun.zbj@antgroup.com>
 <02fe00c3b10e4476d500ad7a34024b7eae5e3c97.camel@intel.com>
 <bfdb5787-65db-4c64-bce1-d39f37ad09fa@intel.com>
In-Reply-To: <bfdb5787-65db-4c64-bce1-d39f37ad09fa@intel.com>

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,
> >>  		}
> >> =20
> >>  		mutex_unlock(&encl->lock);
> >> +
> >> +		if (need_resched())
> >> +			cond_resched();
> >>  	}
> >> =20
> >>  	ret =3D 0;
> >> @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_=
encl *encl,
> >>  		entry->type =3D page_type;
> >> =20
> >>  		mutex_unlock(&encl->lock);
> >> +
> >> +		if (need_resched())
> >> +			cond_resched();
> >>  	}
> >> =20
> >>  	ret =3D 0;
> >> @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_enc=
l *encl,
> >>  		kfree(entry);
> >> =20
> >>  		mutex_unlock(&encl->lock);
> >> +
> >> +		if (need_resched())
> >> +			cond_resched();
> >>  	}
> >>
> >=20
> > You can remove the need_reshced() in all 3 places above but just call
> > cond_resched() directly.
> >=20
>
> 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 ca=
re
> how changes like this impact the performance of the system and having act=
ions
> on these page ranges take much longer than necessary.
> For reference, please see 7b72c823ddf8 ("x86/sgx: Reduce delay and interf=
erence
> of enclave release") that turned frequent cond_resched() into batches
> to address performance issues.
>
> It looks to me like the need_resched() may be a quick check that can be u=
sed
> to improve performance? I am not familiar with all use cases that need to=
 be
> considered to determine if a batching solution may be needed.

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

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.

From this follows that *if* need_resched() needs to be removed then
that is not really part of the bug fix, so in all cases the bug fix
itself must include need_resched() :-)

phew, hope you got my logic here, i think it reasonable...

BR, Jarkko