Received-SPF: pass (google.com: domain of linux-kernel+bounces-77467-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <f242cfb2-e8ab-49f3-9b8e-1236dd361b64@intel.com>
Date: Fri, 23 Feb 2024 11:31:18 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 08/15] x86/sgx: Implement EPC reclamation flows for
 cgroup
Content-Language: en-US
To: Haitao Huang <haitao.huang@linux.intel.com>, "Mehta, Sohil"
	<sohil.mehta@intel.com>, "mingo@redhat.com" <mingo@redhat.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>, "x86@kernel.org" <x86@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>, "hpa@zytor.com"
	<hpa@zytor.com>, "tim.c.chen@linux.intel.com" <tim.c.chen@linux.intel.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>, "mkoutny@suse.com"
	<mkoutny@suse.com>, "tglx@linutronix.de" <tglx@linutronix.de>,
	"tj@kernel.org" <tj@kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "bp@alien8.de" <bp@alien8.de>
CC: "mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
	"seanjc@google.com" <seanjc@google.com>, "anakrish@microsoft.com"
	<anakrish@microsoft.com>, "Zhang, Bo" <zhanb@microsoft.com>,
	"kristen@linux.intel.com" <kristen@linux.intel.com>, "yangjie@microsoft.com"
	<yangjie@microsoft.com>, "Li, Zhiquan1" <zhiquan1.li@intel.com>,
	"chrisyan@microsoft.com" <chrisyan@microsoft.com>
References: <20240205210638.157741-1-haitao.huang@linux.intel.com>
 <20240205210638.157741-9-haitao.huang@linux.intel.com>
 <fa091e657c2d3f3cc14aff15ad3484e0d7079b6f.camel@intel.com>
 <op.2jhbnui0wjvjmi@hhuan26-mobl.amr.corp.intel.com>
 <d4aacbea13f0ff03ad94c31a4735746aedd009ba.camel@intel.com>
 <op.2jjzsnz0wjvjmi@hhuan26-mobl.amr.corp.intel.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <op.2jjzsnz0wjvjmi@hhuan26-mobl.amr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R3ZMVnN6cTF4d2VHWWU4d1RLMkhlejlvcjVMN1Eyb1l1NXNyRlF3MmtocktI?=
 =?utf-8?B?VjhoT1VXczBlNWliUXhuYlpMbmVPMHlabkVBVVFoMFA5NzJ5ZEFXTU9yVGNO?=
 =?utf-8?B?cHYrRnJEMlpZeld1UTNYZ0c2Y0NPaFRyVnAyZmlTZHdWSDZEK0JmdXJNTWpi?=
 =?utf-8?B?VFBrZk5zWWNOVE5aVlVybmRXSnN0K1ZiNEsyTmxZMEtrYTJ1WTlBdTlpdDRw?=
 =?utf-8?B?MkZ5YjRnVXdwZDEvZW1vMmd3UjlhSk01STFQTld4SzNGR2NXSlUreEFjY28x?=
 =?utf-8?B?aE91VmgvWEVSbldUODhRSGFzaFA2WEJkSVJpMm5maVhhaENoS0t1STdKUGh1?=
 =?utf-8?B?YzNYNUk0bUpMYmxYUW96T3FDaHRLMjU3MFd4ZjZIT09MUGhqZmhLb01lYys1?=
 =?utf-8?B?MXpoT0k2amVkT2pxeldxM3BCWWpnVThtdWRMMytzWElCVDk4a0o1QlJNaGFM?=
 =?utf-8?B?dGVzaS9rbG1FNTVqTzdwWTZFbStMZnJ1TDRjK2ZnK1h6d3MxZHB3enRQL28x?=
 =?utf-8?B?VnVwTy85ZTB3UXh6Q2FpWDBzWXFxbWNGT0F3NWF6VjZMcHROMkVFS0NBNlhM?=
 =?utf-8?B?Q2FjOWwzVW53T0ZKWFdnc2lDY0ZxblBQWC8yd3IrWk1NczFBTUhIWEZ3L04w?=
 =?utf-8?B?cWYyMHI1ZkpUOVVUTlVJSmp6czF5dVJyaWRacWsrM08rSkRpRTZhNGVJbmRI?=
 =?utf-8?B?MDlvQ3NGWXZsbmMyUzJNaHlyVkh2S1UrWVdsZFlrb1JpeTB1VUNXN0FzWFJh?=
 =?utf-8?B?S2JJTXh3YzRzL1RaSEJ0T3BDd1lPYUNwUXdIRnpMRHpBaGV6eTI0OFNqLzhh?=
 =?utf-8?B?YlR3R1RsdXB6bXphREpjSHRueE1SS1k1QXREbG9RL05xNUc1WU9xbjVjbTN2?=
 =?utf-8?B?ZVZSQmlUV0RmZWZBd1FkbnJvUG05MldDWktUZkdvcWljL251YWpSSy92RjB1?=
 =?utf-8?B?OGFuZDU0clVwNTJiSDMza3FXaC91dEFVVllMVVMzdERSRURZK0RIQ3JiYk01?=
 =?utf-8?B?MCtKTTY2eTBlWnRCUWlmcmpGOGxrVDVjbGN6THFhRitEL093dGZhSzdGZ1Fn?=
 =?utf-8?B?dEc0SGRreXpxSW9DUHdHdm9URjlZVGxVejB1UDZxVC9JZlFpWm1qUGcwWll2?=
 =?utf-8?B?dzB3RmVOdWhmMUQwc0tBL1FXS1hRRFFPS1M1SW91UFIwNlY1d2drRmRUMGpz?=
 =?utf-8?B?RS9pQkg3SVNSZUNoYjlhRmdnYXBpYnZJSlJxMDJUM3dYNklhUDhqZ25Bb0x2?=
 =?utf-8?B?bndleGUvdzA1SHpNNTNyTytieVpUcDhFWVc2UjdOK2YxTDNiT21kd2tsR3lS?=
 =?utf-8?B?U29YYU1nMlR2KzhoSHBzSktsU2RyTXBzUEhnamUxREZjTktZOHFhWUtrSyta?=
 =?utf-8?B?TFJXa0p5WEpRNnBZZzBGTVJQVUFqaklEdkR2bHVLdWRTejZIbWNndy9Ob2pT?=
 =?utf-8?B?U2w5QlBNOFhSeUJzSjJUMDJiUDR1b0h1SXRFSVNxWFRSc1crd0tXb2hhM3VS?=
 =?utf-8?B?dk9rYlM3dDl5eTBwZWFBWjIzQjF0Tm80WXBXL21vYVZ5REZtWXBEQTBPL3BP?=
 =?utf-8?B?Vi9DaUZzc1d5NWsyMys3NnFldzdHNXZoakpXYk5iV0VDak0xdk9BN0s4Q0RK?=
 =?utf-8?B?LzdpNkNoazZTWVY0M3drcFNpUGRxNlp3WEg0eEcwenRwU2VzODcxVy9xK1Mx?=
 =?utf-8?B?SDloL2FNekcxYWVOcThMKytTWTFmUi9YN0kyaitQMjhTQ2Y2VHJjZHdaNmpv?=
 =?utf-8?B?T3hNUG54djI2aUUveWpNRVNiaE85aGpjNzdxWUVsczd3SWFZWGpJcnNhNC9O?=
 =?utf-8?B?M2x5VkhySDZxd2tPei9LVHE3MCt4Z0Y3TllNeEFyZFBhRUZNRnF3Q2VYdzB2?=
 =?utf-8?B?enVWdnpXODlEK2JyTEprR3E5QXA2L2JIUFNtTnlyL0ZnTkZiWjEzV1c0U05M?=
 =?utf-8?B?TkNwMzNtUllmaEYwOUljNGduVTIvWWcwVXVzTG1XeitqWUkvZUFVK20zWWlp?=
 =?utf-8?B?Wm9JNjl1OVBWTktlbmQ4TGF5TDRQSE9lSXMxQ3ZxeFdoWDl1NjZ2ZERkbzZT?=
 =?utf-8?B?RjFaeVZzSXlSQjVzTGsvNDlpQjA0Zm9qdTdxT2NJNGUxWE1BT2RoSzlCRlF5?=
 =?utf-8?Q?oS3EJQiBNpavpFGaJYtn/PAYw?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 59b0f093-14aa-4928-e131-08dc33f6051c
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2024 22:31:32.2420
 (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: M57GuI/RTPt5kIiJw6U+tXFqZOQ8EtWX8AVG/crV37qySGoSQ8iM7UWWlxbg6vOENJGbFkt6wxlI4Lx9AFNxqA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6055
X-OriginatorOrg: intel.com


On 23/02/2024 6:20 am, Haitao Huang wrote:
> On Wed, 21 Feb 2024 05:00:27 -0600, Huang, Kai <kai.huang@intel.com> wrote:
> 
>> On Wed, 2024-02-21 at 00:44 -0600, Haitao Huang wrote:
>>> [...]
>>> >
>>> > Here the @nr_to_scan is reduced by the number of pages that are
>>> > isolated, but
>>> > not actually reclaimed (which is reflected by @cnt).
>>> >
>>> > IIUC, looks you want to make this function do "each cycle" as what you
>>> > mentioned
>>> > in the v8 [1]:
>>> >
>>> >     I tested with that approach and found we can only target number of
>>> > pages
>>> >     attempted to reclaim not pages actually reclaimed due to the
>>> > uncertainty
>>> >     of how long it takes to reclaim pages. Besides targeting number of
>>> >     scanned pages for each cycle is also what the ksgxd does.
>>> >
>>> >     If we target actual number of pages, sometimes it just takes 
>>> too long.
>>> > I
>>> >     saw more timeouts with the default time limit when running 
>>> parallel
>>> >     selftests.
>>> >
>>> > I am not sure what does "sometimes it just takes too long" mean, but
>>> > what I am
>>> > thinking is you are trying to do some perfect but yet complicated code
>>> > here.
>>>
>>> I think what I observed was that the try_charge() would block too long
>>> before getting chance of schedule() to yield, causing more timeouts than
>>> necessary.
>>> I'll do some re-test to be sure.
>>
>> Looks this is a valid information that can be used to justify whatever 
>> you are
>> implementing in the EPC cgroup reclaiming function(s).
>>
> I'll add some comments. Was assuming this is just following the old 
> design as ksgxd.
> There were some comments at the beginning of 
> sgx_epc_cgrooup_reclaim_page().
>          /*
>           * Attempting to reclaim only a few pages will often fail and is
>           * inefficient, while reclaiming a huge number of pages can 
> result in
>           * soft lockups due to holding various locks for an extended 
> duration.
>           */
>          unsigned int nr_to_scan = SGX_NR_TO_SCAN;
> 
> I think it can be improved to emphasize we only "attempt" to finish 
> scanning fixed number of pages for reclamation, not enforce number of 
> pages successfully reclaimed.

Not sure need to be this comment, but at somewhere just state you are 
trying to follow the ksgxd() (the current sgx_reclaim_pages()), but 
trying to do it "_across_ given cgroup and all the descendants".

That's the reason you made @nr_to_scan as a pointer.

And also some text to explain why to follow ksgxd() -- not wanting to 
block longer due to loop over descendants etc -- so we can focus on 
discussing whether such justification is reasonable.