Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <573970eb-f466-aad4-3808-fde5f3374e7c@intel.com>
Date:   Wed, 6 Apr 2022 10:50:23 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.7.0
Subject: Re: [PATCH V3 19/30] x86/sgx: Free up EPC pages directly to support
 large page ranges
Content-Language: en-US
To:     Jarkko Sakkinen <jarkko@kernel.org>,
        Dave Hansen <dave.hansen@intel.com>
CC:     <dave.hansen@linux.intel.com>, <tglx@linutronix.de>,
        <bp@alien8.de>, <luto@kernel.org>, <mingo@redhat.com>,
        <linux-sgx@vger.kernel.org>, <x86@kernel.org>, <seanjc@google.com>,
        <kai.huang@intel.com>, <cathy.zhang@intel.com>,
        <cedric.xing@intel.com>, <haitao.huang@intel.com>,
        <mark.shanahan@intel.com>, <hpa@zytor.com>,
        <linux-kernel@vger.kernel.org>
References: <cover.1648847675.git.reinette.chatre@intel.com>
 <b3a17e1ce7bcd14db3c28903e8a97f094998ae74.1648847675.git.reinette.chatre@intel.com>
 <Ykvrk4hYvBEnNOOl@kernel.org>
 <e118f4c6-7216-15f3-2bda-3a5851b0bcb1@intel.com>
 <f8d92685-27dd-e7ef-ce0d-100a285aec4b@intel.com>
 <984834578beba18fa5b0196a0d9e4327dc22cf73.camel@kernel.org>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <984834578beba18fa5b0196a0d9e4327dc22cf73.camel@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?emZGY0hEMUNzQ1ByR0d4MGQyenBYMllucTBVanZsS3AzOGY3ZC9rVkJFZTNX?=
 =?utf-8?B?dzlMbHlpcWNpazc5TUNkZkRBVGhpem5uYzRnczUzeXI0NFVIVnlKZnJTcmtm?=
 =?utf-8?B?a01LRFVENFZSLzNDYTFjdGc3ZEQyWERRUFd4Z2piMmhhTjQzT2NocFFJK2ZN?=
 =?utf-8?B?ckk5OUwwSnoxSittbGJuUGo1WkxMQVZ5TEZhckM5ZG9oRVBXcDhRcERVRUsw?=
 =?utf-8?B?UEQ0NkZVejdRWWcxOTVHKzRGWWlTcGsyd3hTWG1rNFdVd3phVVFOMVlsT1Vn?=
 =?utf-8?B?b2Y3dUVUZUJWWDNFUFVESTBPWEYzZ0tYSGVwcjNodzB1bEFOZmZ6SERGbHBB?=
 =?utf-8?B?NEdENFNldmQ5RU1DNWNIRTEvR1diazFleW80Z1EzZGw2eWFtWVZtaHN6U0hN?=
 =?utf-8?B?MGl0ZDhIUGlCZmZDTUc1akRJbHpzVFk5akFBcTg1Q2Uxb1N1amtwR2Z3TnR2?=
 =?utf-8?B?TXN5cWtSWGZHYmFzWXFMUnI4YitHMGlsM2ZlVjZXZ2F1SXF4TWRjR3NnNjNW?=
 =?utf-8?B?d3BOY29xUEpibys2MG82cER4dFdBb25uZTlUNHVwWXJta1ZNaUpMUmo1a0pm?=
 =?utf-8?B?YlhvZ21kcTZpR1ZCdzdQeHlOR3REcVNObnZlRG5OUTBwcjcxZ1dKTUpJb1RJ?=
 =?utf-8?B?cXY1aHBwWVp6MGQvMFN2UVFsbmxqRS9nNDRjZWE4eDZOVHRub2RmRFhhaHZo?=
 =?utf-8?B?aFQvVTNHdFlzVSsrVnhMV1MvN3hxZFRrS3liVDBjb09GYU52OElTeWw2bXVp?=
 =?utf-8?B?VzFjZ0NMOGhpWVNDc0dFckJuSmZXK21jMC9hdnk5QUxXUi9vSDhrWnhQQWdF?=
 =?utf-8?B?K3hFbWsvTXc5VXZ1QWpTUnFRWVd1Nk41WC9kT3hlUGZFQTBLTFc1Y2FvUVdk?=
 =?utf-8?B?K3V6SVBDZ3hQb1ZCSWF3TTBvbmZTOEhBOVlkTGkzUWtid0trU1JhM2s3WEcr?=
 =?utf-8?B?ejlLbEE4MzREaU5WTUtXOWl3ZG1zWGpaS0NzeW52ZUgrNFpSS3QzN2ZOZTNL?=
 =?utf-8?B?b3VZUVNvZGtDRnB1dGhjck5TbEV2djIvYzZSM3FJSHhaOG5PWU5uTzVDbUx2?=
 =?utf-8?B?Q2QvL1I2Ri9FcGVxelpsNEp6WE41MlpncERTOWh2eVVLNjdCRklUcVhsWEY2?=
 =?utf-8?B?cTZPTEdqNUpkZlVWKzJqZWhUN2NkR0FTOVNPV2dlYUptcDZoemhreDhCYmJG?=
 =?utf-8?B?UllNbngxQWJjd0JJZ2pTM0V5dFZKUmxMcnBQQ3NwRGF2bCtVdm9Vb0p6YkVq?=
 =?utf-8?B?aGNkMkRCSzVJaVVmZWtBdDRyQms2eTcyajZtcHU3U2NVTG9rc1BXTFR6YUlX?=
 =?utf-8?B?ZnJoREhjZllTYjRWckgzaHBKZU9BUGdPUlZzL2NMN2pDQkZRa21xSmExL0pz?=
 =?utf-8?B?QUNkWmV6YTg0dytSK21nYlBuTWxRZHEzZzlrRnQzSmdDL0FUS3lIbkdOZ29E?=
 =?utf-8?B?Kzd3YTUyVTFTVGo1bnBOdW1COUJiVnBwWVV3KytTZk1rUEZaSGUvTDNjdTFP?=
 =?utf-8?B?KzVGQU9aVmxZMlBzaHBFRnR4SU9FTjJmUFpaQ0IvaDBQQTY1eWpYS2lsU2Mz?=
 =?utf-8?B?VE8rdDg2ekNBaDBHa0dJTTByekVqU1NxNlExUzJsZjNQaDI3emM2bnpCMzN4?=
 =?utf-8?B?RkhWMVB4RVVXZ0ZJL04rYm5qUWZJaFhBZTFYWkJxQ1labG9yVE12aUk4UHNS?=
 =?utf-8?B?S2xvY01FdTlQRzV1V3dhTlNyMEFibUZiaHQ0WGJnbWVITmw1YVljNWU1a01N?=
 =?utf-8?B?V0JDdTh3bnRxTTNTRkR0aXpubTRsK3oxQVo5Vlcyb29jNktSTkVOdjM5YXpy?=
 =?utf-8?B?SS91QUVxeU9hRVNLSlRtTmkxVWxqd0lwcGlsMFd5R01SR2RHYjE2cTMwUVBq?=
 =?utf-8?B?U1ZUUS8vUTN1bnZ3cHBDQ0ZIa0ozTFNITFIyRXRSeWpPSjRvS3BIMmc2Q0Rm?=
 =?utf-8?B?Q1N4enRsL09INmovdmhhZVdhOWtjc3BCMWlSallKWFFESUMyQWdqNFRSVVR4?=
 =?utf-8?B?MWlIKytrcVN5ekVjYkdDR0NzdGpVNjR6Y1Q5bzRDZ25oREZ3TnlUTmFtY2gy?=
 =?utf-8?B?STVzNUhibElRamFxQkRNeEg1cnRTUjdFUVFXZFI4Wk1US3BISzlPRytXU01z?=
 =?utf-8?B?QkpWWVd1MGZ1amkwU2NSTytNbEdZcDE0RStZbkpJUXVtTzE0eVJYUUp4dkVW?=
 =?utf-8?B?SmtmQ2NwT1lwNVdQWCtudHgvNmJDbE15RER2a1lMZUttQXpxL2pvS3VKWWZx?=
 =?utf-8?B?QUh5Sy9aNHVTbnU3akJNNDQ3cll5d0p6elBsTTFBVUE5N1poYitiWWl5R2pV?=
 =?utf-8?B?UDJMbVRKcGpSTjZBQUUxcmIvM1NMS01ucHRHWTNTbkNDWVBBa2NOSFpNNWRt?=
 =?utf-8?Q?14dl51SgYNnN20qM=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ff78001f-4643-4f67-a4b3-08da17f5efc2
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Apr 2022 17:50:28.5470
 (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: 7L1w/kMnkHT1040Uv+P801E7Ty8iKKh2x04MFe1eOc+Zoku4B9b2RbuI0AOi+m/gCh1A0hJdSuVQQPLb/m+zUYJ6JaNybyUdoN63N6ps/FA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4348
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jarkko,

On 4/5/2022 11:35 PM, Jarkko Sakkinen wrote:
> On Tue, 2022-04-05 at 10:25 -0700, Dave Hansen wrote:
>> On 4/5/22 10:13, Reinette Chatre wrote:
>>>>> +void sgx_direct_reclaim(void)
>>>>> +{
>>>>> +       if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
>>>>> +               sgx_reclaim_pages();
>>>>> +}
>>>> Please, instead open code this to both locations - not enough redundancy
>>>> to be worth of new function. Causes only unnecessary cross-referencing
>>>> when maintaining. Otherwise, I agree with the idea.
>>>>
>>> hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would be
>>> made available for direct use from everywhere in the driver. I will look into this.
>>
>> I like the change.  It's not about reducing code redundancy, it's about
>> *describing* what the code does.  Each location could have:
>>
>>         /* Enter direct SGX reclaim: */
>>         if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
>>                 sgx_reclaim_pages();
>>
>> Or, it could just be:
>>
>>         sgx_direct_reclaim();
>>
>> Which also provides a logical choke point to add comments, like:
>>
>> /*
>>  * sgx_direct_reclaim() should be called in locations where SGX
>>  * memory resources might be low and might be needed in order
>>  * to make forward progress.
>>  */
>> void sgx_direct_reclaim(void)
>> {
>>         ...
> 
> Maybe cutting hairs but could it be "sgx_reclaim_direct"? Rationale
> is easier grepping of reclaimer functions, e.g. when tracing.

Sure, will do.

This may not help grepping all reclaimer functions though since
the code is not consistent in this regard.

Reinette