Messages in this thread | | | Date | Sat, 4 Feb 2023 23:44:58 +0530 | Subject | Re: [PATCH V2 2/3] sched/numa: Enhance vma scanning logic | From | Raghavendra K T <> |
| |
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...
| |