lkml.org 
[lkml]   [2023]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V2 2/3] sched/numa: Enhance vma scanning logic
From
On 2/3/2023 4:45 PM, Peter Zijlstra wrote:
> On Wed, Feb 01, 2023 at 01:32:21PM +0530, Raghavendra K T wrote:
>> During the Numa scanning make sure only relevant vmas of the
>> tasks are scanned.
>>
>> Before:
>> All the tasks of a process participate in scanning the vma
>> even if they do not access vma in it's lifespan.
>>
>> Now:
>> Except cases of first few unconditional scans, if a process do
>> not touch vma (exluding false positive cases of PID collisions)
>> tasks no longer scan all vma.
>>
>> Logic used:
>> 1) 6 bits of PID used to mark active bit in vma numab status during
>> fault to remember PIDs accessing vma. (Thanks Mel)
>>
>> 2) Subsequently in scan path, vma scanning is skipped if current PID
>> had not accessed vma.
>>
>> 3) First two times we do allow unconditional scan to preserve earlier
>> behaviour of scanning.
>>
>> Acknowledgement to Bharata B Rao <bharata@amd.com> for initial patch
>> to store pid information.
>>
>> Suggested-by: Mel Gorman <mgorman@techsingularity.net>
>> Signed-off-by: Raghavendra K T <raghavendra.kt@amd.com>
>> ---
>> include/linux/mm.h | 14 ++++++++++++++
>> include/linux/mm_types.h | 1 +
>> kernel/sched/fair.c | 15 +++++++++++++++
>> mm/huge_memory.c | 1 +
>> mm/memory.c | 1 +
>> 5 files changed, 32 insertions(+)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 74d9df1d8982..489422942482 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1381,6 +1381,16 @@ static inline int xchg_page_access_time(struct page *page, int time)
>> last_time = page_cpupid_xchg_last(page, time >> PAGE_ACCESS_TIME_BUCKETS);
>> return last_time << PAGE_ACCESS_TIME_BUCKETS;
>> }
>> +
>> +static inline void vma_set_active_pid_bit(struct vm_area_struct *vma)
>> +{
>> + unsigned int active_pid_bit;
>> +
>> + if (vma->numab) {
>> + active_pid_bit = current->pid % BITS_PER_LONG;
>> + vma->numab->accessing_pids |= 1UL << active_pid_bit;
>> + }
>> +}
>
> Perhaps:
>
> if (vma->numab)
> __set_bit(current->pid % BITS_PER_LONG, &vma->numab->pids);
>
> ?
>
> Or maybe even:
>
> bit = current->pid % BITS_PER_LONG;
> if (vma->numab && !__test_bit(bit, &vma->numab->pids))
> __set_bit(bit, &vma->numab->pids);
>
>

Sure ..will use one of the above.

>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 060b241ce3c5..3505ae57c07c 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -2916,6 +2916,18 @@ static void reset_ptenuma_scan(struct task_struct *p)
>> p->mm->numa_scan_offset = 0;
>> }
>>
>> +static bool vma_is_accessed(struct vm_area_struct *vma)
>> +{
>> + unsigned int active_pid_bit;
>> +
> /*
> * Tell us why 2....
> */

Agree. The logic is more towards allowing unconditional scan first two
times to build task/page relation. I will experiment if we further need
to allow for two full passes if "multi-stage node selection" (=4), to
take care of early migration.

But only doubt I have is numa_scan_seq is per mm and thus will it create
corner cases or we need to have a per vma count separately when a new
VMA is created..

>> + if (READ_ONCE(current->mm->numa_scan_seq) < 2)
>> + return true;
>> +
>> + active_pid_bit = current->pid % BITS_PER_LONG;
>> +
>> + return vma->numab->accessing_pids & (1UL << active_pid_bit);
> return __test_bit(current->pid % BITS_PER_LONG, &vma->numab->pids)
>> +}
>> +
>> /*
>> * The expensive part of numa migration is done from task_work context.
>> * Triggered from task_tick_numa().
>> @@ -3032,6 +3044,9 @@ static void task_numa_work(struct callback_head *work)
>> if (mm->numa_scan_seq && time_before(jiffies, vma->numab->next_scan))
>> continue;
>>
> /*
> * tell us more...
> */

Sure. Since this is the core of the whole logic where we want to confine
VMA scan to PIDs of interest mostly.

>> + if (!vma_is_accessed(vma))
>> + continue;
>> +
>> do {
>> start = max(start, vma->vm_start);
>> end = ALIGN(start + (pages << PAGE_SHIFT), HPAGE_SIZE);
>
>
> This feels wrong, specifically we track numa_scan_offset per mm, now, if
> we divide the threads into two dis-joint groups each only using their
> own set of vmas (in fact quite common for workloads with proper data
> partitioning) it is possible to consistently sample one set of threads
> and thus not scan the other set of vmas.
>
> It seems somewhat unlikely, but not impossible to create significant
> unfairness.
>

Agree, But that is the reason why we want to allow first few
unconditional scans Or am I missing something?

>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 811d19b5c4f6..d908aa95f3c3 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1485,6 +1485,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
>> bool was_writable = pmd_savedwrite(oldpmd);
>> int flags = 0;
>>
>> + vma_set_active_pid_bit(vma);
>> vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
>> if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
>> spin_unlock(vmf->ptl);
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 8c8420934d60..2ec3045cb8b3 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4718,6 +4718,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>> bool was_writable = pte_savedwrite(vmf->orig_pte);
>> int flags = 0;
>>
>> + vma_set_active_pid_bit(vma);
>> /*
>> * The "pte" at this point cannot be used safely without
>> * validation through pte_unmap_same(). It's of NUMA type but
>
> Urghh... do_*numa_page() is two near identical functions.. is there
> really no sane way to de-duplicate at least some of that?
>

Agree. I will explore and will take that as a separate TODO.

> Also, is this placement right, you're marking the thread even before we
> know there's even a page there. I would expect this somewhere around
> where we track lastpid.
>

Good point. I will check this again

> Maybe numa_migrate_prep() ?

yes.. there was no hurry to record accessing pid early above...

\
 
 \ /
  Last update: 2023-03-27 00:10    [W:2.097 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site