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: <9f95a85f-5396-b8bd-50cf-c4eeeac2a013@amd.com>
Date:   Tue, 1 Feb 2022 17:52:55 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC PATCH v0 1/3] sched/numa: Process based autonuma scan period
 framework
Content-Language: en-US
To:     Mel Gorman <mgorman@suse.de>
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, bristot@redhat.com,
        dishaa.talreja@amd.com, Wei Huang <wei.huang2@amd.com>
References: <20220128052851.17162-1-bharata@amd.com>
 <20220128052851.17162-2-bharata@amd.com> <20220131121714.GX3301@suse.de>
From:   Bharata B Rao <bharata@amd.com>
In-Reply-To: <20220131121714.GX3301@suse.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SWlla0xJS1EvTXlWbzJ5bG04NEVSUWFQdGRhUkJPMFVPQTYyWDIvS3ZSOTlr?=
 =?utf-8?B?YkU3eHNhbzNER1cweVRDMVJPUFVZM2JVMWR0ZGs3QXBZdjdyeE1CR01UcUVO?=
 =?utf-8?B?RStqWGtPRkN5amZmMk8xSlhMWEgzakNXRWpVL1dDbXJLbFpON0tOMy9uc2Ir?=
 =?utf-8?B?SlVDMGU0YlRncCtvdlhoSE05WEtKY2xEVXN2OTBsWlhENE5tQWVFdVJ2bnhO?=
 =?utf-8?B?VHpxNFRsYkFEeFhNVFhSRzc4d0Z1YmV1ZnV3aTFGRlI3RnVIRXpjUGtNb0hU?=
 =?utf-8?B?N0RQcEh2dEFXWER3QUwwaHRxdEhTSGZIRVdDekhEWlMxdWdTMDlHMnQ1bUpN?=
 =?utf-8?B?NlNqYzhQVDM5YmNVOFU5Q0FIcE9KVHlFOWJYam40Yjd3N0xQZ1pPOEllbW84?=
 =?utf-8?B?Q1pvcVE2eDJ5ZFlzanpOcVhyYXc1dVo1VlFOVFA3MUg3RTkrWk9DeGFrcjZM?=
 =?utf-8?B?UWRQYTZZZUpyRjhPUFI1bjN5dWNOMmUvcWZGWXJtOXN5ZmhxU1M5SFgxN1ZQ?=
 =?utf-8?B?dmNTUVpaTFpnbU1NdUhkdm1KOEV5WVRsT25Oa2JDY2hMRGQxdE9qemtWaWZx?=
 =?utf-8?B?cVpWS0t3NUs0NlBNOWxJWXRlMmJVRE5zbzlCZ0hDSzV6QlBHMDRRaTVBSGpF?=
 =?utf-8?B?STdLY284TzdtRGRrYXJuZ0xWRS9iYlp3T1UxMGhaNVpndlZQekkvc1JUVWpB?=
 =?utf-8?B?V1IrdlljdmZHZHdjZlJkQlF5S3p5b0hNVk5SMHczMXJNc3hRV0V5dVJPdEI0?=
 =?utf-8?B?OHRDb2hMRWoxRlV4NHA3d3ZGRk1NM0Qrb1JiMDdMdVBZZ2lVOUo0Qy9XbGth?=
 =?utf-8?B?RUdmQzlrWWs1SSthM2M0eEpsK1hsamlZcmdjbXJGbGdacHEvTWtYRkVxNElp?=
 =?utf-8?B?QkxYcXlzQ2JUME5KMmVmaTc0aVdIZFFwUlhhLzZ2TGRRUlZSR0dUbXNOT3Uy?=
 =?utf-8?B?Rm9MMldSd05FNDVBZ3hWU0ZXUnNqY0dKZ1dPZTh1ckIyYVVxdHhCSUNTdkFz?=
 =?utf-8?B?a1FnbnV1NlVFV2laWmxMa09uMjhmVXAxQlJLUHBaeVkvYXhWMEFPR0c5ZDgw?=
 =?utf-8?B?VXg0K1pMVUlRajh4SERjR29XRy83c1JHS1RnRFpaZjh6WGY3WHU1ZlB6ZUdK?=
 =?utf-8?B?S1FWY2JrMHJabStHODAwcnpoV2FKam05M0pLbDlqOHRCekFDVE9KS3Btdjdh?=
 =?utf-8?B?di9ST0VLWTFOWmNVeUhKODVvelFhRldzdWhScktMekxQWWkwZlJYL1ZaSlgx?=
 =?utf-8?B?cUZqU29sOHZqVzhZeHM2YmVVREJENmlzVGVEaUpWUTZWYnlFVEVKNjduQ2Iz?=
 =?utf-8?B?UjkyeGd0d0V3L1JxdTNEOW5seHNLM0U4cDhmbHk2cWJQU0RORERQalFUdTFs?=
 =?utf-8?B?ZkhoSW5rcnZwZjJISVZmNVlyNDNFL2ZYUkdYdDhsT3hJMHphQ2hKWmtDdEFu?=
 =?utf-8?B?NkFIWjQydTcrdkpUbXJ5RnhVOXAvaTU2SW1HZmhDbVU3dWdjc1pna010NEpE?=
 =?utf-8?B?b3J1azg3ODkyYzJwOFl6NjAxdTIzMit0eUpSc09DcTNhVHJTaVUyOVY1WWpJ?=
 =?utf-8?B?Tlh5NU1ZT3FJcklsVkFRUnpJSWhKS3dWdCttTjVqYzZFckNUK2xjRTRWRjdQ?=
 =?utf-8?B?bzQ1TlpKSFpwMXJkbEhyMXZDWmtZWVJVbFhWYlNmcGF0d21uUTVvbWQyaHFo?=
 =?utf-8?B?MUJNTGFGeUpIQWNXL0JJZElMSVd0K293cmljbWJFSjF0VGRMaFZKVy81YUlz?=
 =?utf-8?B?eXoyVXZqUFR3NWUxNVhUMUFpamkwS1ZQQWlSdWR5eVdjVm5jSm9pMThtOEs2?=
 =?utf-8?B?TC93d0NtY1F5NXNGcUsrSkRJLzVCMzFYRmtUMTlnMHAxSytzcHVLWERiU0wy?=
 =?utf-8?B?WXE4Tzl1dHJFSStKUVB3NlNoTUVnV0ZpMmFsdFVUcEpNbnNXTG42c1BWQ2FV?=
 =?utf-8?B?Nkl2SFR4UE50K1g5UFZZQVFoYnZVQWQxdTN2M21heEV3TzBQZ2xwcEhocDgy?=
 =?utf-8?B?L2JwbU5TdElTRXpnVUY0QklMREV0bjhXVUN2cW9mQUUzR2Nic2N0djhUWU5N?=
 =?utf-8?B?UnhSZm5HdGwyanYxNnBiZ3JVeGllaCtWekFNd3dFQ1FaUkxDVXplUmY0MkR0?=
 =?utf-8?B?TUxvekVUZG1jTUZCNjBDdEN3Wm5raGV1aEdQRjd6c3ZUQ0dYOFBOMWpXNEJi?=
 =?utf-8?Q?gRF+f1OyoqK/s3AsxE6cJfk=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3eff0fe4-46aa-42af-920b-08d9e57d9a2a
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4289.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2022 12:23:07.4636
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /sWP9t2ZjTNSgaJ+D6c01dLlrtvN0wnfmiDdNXN755TwBvYrdyploPaL9nBr2f5rhLK0e3Dk2WWQB1fCauOzcQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW2PR12MB2539
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Thanks Mel for taking time to look at the patchset and providing your valuable review comments.

On 1/31/2022 5:47 PM, Mel Gorman wrote:
> On Fri, Jan 28, 2022 at 10:58:49AM +0530, Bharata B Rao wrote:
>> From: Disha Talreja <dishaa.talreja@amd.com>
>>
>> Add a new framework that calculates autonuma scan period
>> based on per-process NUMA fault stats.
>>
>> NUMA faults can be classified into different categories, such
>> as local vs. remote, or private vs. shared. It is also important
>> to understand such behavior from the perspective of a process.
>> The per-process fault stats added here will be used for
>> calculating the scan period in the adaptive NUMA algorithm.
>>
> 
> Be more specific no how the local vs remote, private vs shared states
> are reflections of per-task activity of the same.

Sure, will document the algorithm better. However the overall thinking
here is that the address-space scanning is a per-process activity and
hence the scan period value derived from the accumulated per-process
faults is more appropriate than calculating per-task (per-thread) scan
periods. Participating threads may have their local/shared and private/shared
behaviors, but when aggregated at the process level, it gives a better
input for eventual scan period variation. The understanding is that individual
thread fault rates will start altering the overall process metrics in
such a manner that we respond by changing the scan rate to do more aggressive
or less aggressive scanning.  

> 
>> The actual scan period is still using the original value
>> p->numa_scan_period before the real implementation is added in
>> place in a later commit.
>>
>> Co-developed-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Disha Talreja <dishaa.talreja@amd.com>
>> Signed-off-by: Bharata B Rao <bharata@amd.com>
>> ---
>>  include/linux/mm_types.h |  7 +++++++
>>  kernel/sched/fair.c      | 40 ++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>> index 9db36dc5d4cf..4f978c09d3db 100644
>> --- a/include/linux/mm_types.h
>> +++ b/include/linux/mm_types.h
>> @@ -610,6 +610,13 @@ struct mm_struct {
>>  
>>  		/* numa_scan_seq prevents two threads setting pte_numa */
>>  		int numa_scan_seq;
>> +
>> +		/* Process-based Adaptive NUMA */
>> +		atomic_long_t faults_locality[2];
>> +		atomic_long_t faults_shared[2];
>> +
>> +		spinlock_t pan_numa_lock;
> 
> Document what this lock protects. In the context of this patch it appears
> to protect a read of p->numa_scan_period and it's overkill to use a
> spinlock for that.  Also, given that it's a trylock, the task_numa_work
> ends up doing no scanning or updates. This might have some value in
> terms of avoiding multiple threads doing updates if they happen to start
> at the same time but that's a narrow side-effect given the short hold
> time of the lock.

Sure, I put it in the code comment, but will document the usage here.

If the try_lock fails, it means some other thread is updating the stat and
most likely that thread will go ahead with the atomic update to mm->numa_next_scan
and start the scanning. So can't see how this will stall scanning or stat updates
in general. Please note that in the existing scheme, the stats aggregation
happens at fault time but in PAN it happens in task work context.

> 
>> +		unsigned int numa_scan_period;
> 
> Document how the per-mm numa_scan_period is related to the per-task
> numa_scan_period.

They aren't related, per-mm numa_scan_period is in fact a replacement of
per-task numa_scan_period. However numa_migrate_retry interval still depends
on per-task period as you noted elsewhere. I think we could replace that usage
too with per-mm numa_scan_period and completely remove the per-task version.

Regards,
Bharata.