Re: [RFC PATCH V2 1/1] sched/numa: Fix disjoint set vma scan regression

[Raghavendra K T <raghavendra.kt@amd.com>]

On 5/19/2023 1:26 PM, Bharata B Rao wrote:
> On 16-May-23 2:49 PM, Raghavendra K T wrote:
>>   With the numa scan enhancements [1], only the threads which had previously
[...]
>> -#define VMA_PID_RESET_PERIOD (4 * sysctl_numa_balancing_scan_delay)
>> +#define VMA_PID_RESET_PERIOD		(4 * sysctl_numa_balancing_scan_delay)
>> +#define DISJOINT_VMA_SCAN_RENEW_THRESH	16
>>   
>>   /*
>>    * The expensive part of numa migration is done from task_work context.
>> @@ -3058,6 +3072,8 @@ static void task_numa_work(struct callback_head *work)
>>   			/* Reset happens after 4 times scan delay of scan start */
>>   			vma->numab_state->next_pid_reset =  vma->numab_state->next_scan +
>>   				msecs_to_jiffies(VMA_PID_RESET_PERIOD);
>> +
>> +			WRITE_ONCE(vma->numab_state->scan_counter, 0);
>>   		}
>>   
>>   		/*
>> @@ -3068,6 +3084,13 @@ static void task_numa_work(struct callback_head *work)
>>   						vma->numab_state->next_scan))
>>   			continue;
>>   
>> +		/*
>> +		 * For long running tasks, renew the disjoint vma scanning
>> +		 * periodically.
>> +		 */
>> +		if (mm->numa_scan_seq && !(mm->numa_scan_seq % DISJOINT_VMA_SCAN_RENEW_THRESH))
> 
> Don't you need a READ_ONCE() accessor for mm->numa_scan_seq?
> 

Hello Bharata,

Yes.. Thanks for pointing out.. V1 I did ensure that, But in V2 somehow
leftout :( .

On the other-hand I see vma->numab_state->scan_counter does not need
READ_ONCE/WRITE_ONCE since it is not modified out of this function
(i.e. it is all done after cmpxchg above)..

Also thinking more, DISJOINT_VMA_SCAN_RENEW_THRESH reset change itself
  may need some correction, and doesn't seem to be absolutely necessary
here. (will post that separately for improving long running benchmark as
per my experiment with more detail)

will wait for any confirmation of reported regression fix with this
  patch and/or any better idea/ack for a while and repost.