Received-SPF: pass (google.com: domain of linux-kernel+bounces-155642-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <bfdb5787-65db-4c64-bce1-d39f37ad09fa@intel.com>
Date: Tue, 23 Apr 2024 10:08:36 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's
 ioctl() to avoid softlockup
To: "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>, "jarkko@kernel.org" <jarkko@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
CC: "Liu, Shuang" <ls123674@antgroup.com>
References: <20240423092550.59297-1-zhubojun.zbj@antgroup.com>
 <20240423092550.59297-2-zhubojun.zbj@antgroup.com>
 <02fe00c3b10e4476d500ad7a34024b7eae5e3c97.camel@intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <02fe00c3b10e4476d500ad7a34024b7eae5e3c97.camel@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Y0JaMGl5dStIZ0Foa0FmZUl4ZFh0Ykd5WG8zc2JjckkxMnRiNkg1KzVnU3Zr?=
 =?utf-8?B?cmxydDlKSjYyRmVjbk5RMlpUTzJCRjdqUDVVYWljSFFTdWpvMjZkakduTXdp?=
 =?utf-8?B?cld2QXRkOGVIM3lkWXVDWEJ0dThQeVVXQjF5aFQxU2dYNVpSQ28wKzhLckpQ?=
 =?utf-8?B?WldXUUhQZE12UE9FSXJXS1dxb2oxMHpKcjRPcHQrWC95ZXozUVJQV2tMUXg1?=
 =?utf-8?B?Y2JKOGtFQWNZTmsyclRHRVFzK3FReXZuSVZPRnJmSGI3QXVsK3NTWG5vZGF4?=
 =?utf-8?B?REZoeEsvdFdmOXR1ZXJsYkk1dWk0TXk5a1k3MExuMnU1NVZUcmJyakNUTFRt?=
 =?utf-8?B?M3ZBOGJ1eXFsSFhqMm16Z2dxM1p4U0QwYk5GTDJuRlBwVFdLRDBqSG5nWXYr?=
 =?utf-8?B?YzY3ZFJzaXRFRXZnK0RnTXZ1U2kyODZlUWJmc29yeVZEVk9tSVBOSXVuVGFM?=
 =?utf-8?B?Qm8wOWJjckRPbnFZL1AvU040N0tvMjF0dlFDM1hWK3NvM0wwS3JrUFpOQXhP?=
 =?utf-8?B?U01pNjREZHN4WDFJNkVFZ1Z4d3FPQmlpbnFCdHdzQUtuazI0b1U4NjlOSHZ4?=
 =?utf-8?B?WlQzUG9nRjJZdlVmN0grSTRIbDhkbDlhRzgvRHFLTHJBSG5XY1N1WHQ2NXBN?=
 =?utf-8?B?VVowOVorSWpVSmZzQXdobzJBTytzdEVHTDl2SEQwUHBoSHhrOVhVUllVQ0Rp?=
 =?utf-8?B?TTBzMVhteVJiK1JYNEwvM1VIeFkwTU9ieUJVQnc3MVgzZFZsR21uWmRocW0v?=
 =?utf-8?B?Y0RvZGZ5L09pWHl6Q3RZVjBLdGt0ajJyYzgwckJweUM0dWVFVnI3QkZIdk5V?=
 =?utf-8?B?eDhoZTdwL1FnUXFSK1RNajNsb1hTZ0xOZ25FTVZHakhXbHdLMlZUcndnUVd1?=
 =?utf-8?B?a3gzenJVMWIzWXFUQ1QyU0dNRmJlemV0ZW8vS3I5ZjJrdm12ak1CZlJTZmRl?=
 =?utf-8?B?MEVxVzBuUit0bWdLcG9FdHJKaURubFBGNjF4Y1d2YkxmWVcxQzdFYWJWN3Jx?=
 =?utf-8?B?b3oyZXdVMWV5WXVaS2ZyYkFsTDZMYm5lS3pBcUIyU01IK3lSV0p0R2lKOTU4?=
 =?utf-8?B?QnFWSUl1Tk5wNUpYSDlCdjZDUE80b251OEhqZGtLOWxwNGpyR0gvVGVVRjcx?=
 =?utf-8?B?QnhsOHhJa2Jnc1lTbFNWbEVtZ29LVllyK0s0T3VHd2IxdHFXdWRpdDYyd1la?=
 =?utf-8?B?aGVGTCtKNzBqanFDS0ZXRlV5QStpN1ltUmJDcDRGSUsraUFTTlgzWTdBNFdW?=
 =?utf-8?B?allSQjVwRFNtdmZGMVQvcTBrOWhDZEhsNzNlZUZIS2xsajdNbnV1Um9PeXlB?=
 =?utf-8?B?WFhYV1U3UDBrWjkyN21yV0ZtcnMxUVJTd3lpWjRCRXl4cEpTay9CK3JQRHhB?=
 =?utf-8?B?WWxVL3RuK1pLSkQyc3lPamo2R05HVkZhTUJTVjhxbDN3RmJZYVZqTktKN3R2?=
 =?utf-8?B?V2gzYmd0YzdhbG9FdXczekRJRWFSbktlUmEwV0FYOTkraERNdkpmVUlPVDBQ?=
 =?utf-8?B?N0ZadGhvRWJQYlppNlBISithSDdGTEMxYUxJaDl6cXJFSm1OUnVHY2czRWdD?=
 =?utf-8?B?RU1rT0o1bVF3Y0pzbWxodEt6RURwN1pmRE9iajhzVEhVZUJRS3Rydzh3UTJh?=
 =?utf-8?B?MmVpelBEaUE4aHd3ZWtJODZBSjV2MDMvN2xlWmo4VUlTU3dETVVUWVR3YldH?=
 =?utf-8?B?MkVLbVpJeGdPU29JckFySEhmYlM4OTNiUXNzY1l4U2c5blFPS0ZkK1RENlRB?=
 =?utf-8?B?c1hJaTh3eFRZUWMwSnNyQ1huMXN2SGpqd2ZSbDJ4VFBLVVVKQzdKV3kya0ti?=
 =?utf-8?B?UXpYWnRtMjFzSUJiTXpFa0d2US9FQ3AyK3dVUVpSREM3MStMZ3c2YUpIWjVD?=
 =?utf-8?B?N213OVMwblFxMG5hVWZOZndHYjFCc1B0TUtQWERmT0paWXhQMXRaUmJGRXIr?=
 =?utf-8?B?dVJNbjJyK2ljWVBJYWFvcm8wL2Uwd3doVGF1Umd5ZjA5blFvcStBVWpNd04x?=
 =?utf-8?B?eVFHSHJ6b0ludFd0eldvRHZBN29HUkZobnZEdHFQSTdHSXNCY2VjZDcvUTNB?=
 =?utf-8?B?ZDRVNm1vNStBUkh4dlFNaElmRDRodXZqMmNGVk9VakJaRXBhRmFna0JJL09X?=
 =?utf-8?B?RGRPU0FQOE8rSmYrYmFaOXZ2VThMMGhmQ1JYRmhLTGxkVUh6K3E2akZDMndk?=
 =?utf-8?B?YkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 67b6f7dc-9d3a-48ab-8c81-08dc63b8051d
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 17:08:39.2216
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 54toL8EbY+v+CTIaZ+zBkV242qQ7lUyVS26s50wIKxwLxbaDCRE0ZHvXn4BjlqVG9tv0x9luPvfYQY5G3qn0jtlGr3gRFwRZLGshOw742Ms=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5063
X-OriginatorOrg: intel.com

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.

It looks to me like the need_resched() may be a quick check that can be used
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.

Reinette