On 8 Aug 2024, at 10:59, Zi Yan wrote: > On 8 Aug 2024, at 0:14, Huang, Ying wrote: > >> Zi Yan writes: >> >>> do_numa_page() and do_huge_pmd_numa_page() share a lot of common code. To >>> reduce redundancy, move common code to numa_migrate_prep() and rename >>> the function to numa_migrate_check() to reflect its functionality. >>> >>> Now do_huge_pmd_numa_page() also checks shared folios to set TNF_SHARED >>> flag. >>> >>> Suggested-by: David Hildenbrand >>> Signed-off-by: Zi Yan >>> --- >>> mm/huge_memory.c | 14 ++------- >>> mm/internal.h | 5 ++-- >>> mm/memory.c | 76 ++++++++++++++++++++++++------------------------ >>> 3 files changed, 44 insertions(+), 51 deletions(-) >>> >>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>> index a3c018f2b554..9b312cae6775 100644 >>> --- a/mm/huge_memory.c >>> +++ b/mm/huge_memory.c >>> @@ -1699,18 +1699,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf) >>> if (!folio) >>> goto out_map; >>> >>> - /* See similar comment in do_numa_page for explanation */ >>> - if (!writable) >>> - flags |= TNF_NO_GROUP; >>> - >>> nid = folio_nid(folio); >> >> NITPICK: It appears that we can remove "nid" local variable. > > I thought about it, but > > 1. if folio is NULL from vm_normal_folio_pmd(), nid remains NUMA_NO_NODE, > 2. if migration succeeds, nid is changed to target_nid, > 3. if migration fails, nid is the folio node id, > > all three are consumed by if (nid != NUMA_NO_NODE) before task_numa_fault(). > > I will give it a try in next version and see if I can remove it. > >> >>> - /* >>> - * For memory tiering mode, cpupid of slow memory page is used >>> - * to record page access time. So use default value. >>> - */ >>> - if (!folio_use_access_time(folio)) >>> - last_cpupid = folio_last_cpupid(folio); >>> - target_nid = numa_migrate_prep(folio, vmf, haddr, nid, &flags); >>> + >>> + target_nid = numa_migrate_check(folio, vmf, haddr, &flags, writable, >>> + &last_cpupid); >>> if (target_nid == NUMA_NO_NODE) >>> goto out_map; >>> if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) { >>> diff --git a/mm/internal.h b/mm/internal.h >>> index 52f7fc4e8ac3..fb16e18c9761 100644 >>> --- a/mm/internal.h >>> +++ b/mm/internal.h >>> @@ -1191,8 +1191,9 @@ void vunmap_range_noflush(unsigned long start, unsigned long end); >>> >>> void __vunmap_range_noflush(unsigned long start, unsigned long end); >>> >>> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf, >>> - unsigned long addr, int page_nid, int *flags); >>> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf, >>> + unsigned long addr, int *flags, bool writable, >>> + int *last_cpupid); >>> >>> void free_zone_device_folio(struct folio *folio); >>> int migrate_device_coherent_page(struct page *page); >>> diff --git a/mm/memory.c b/mm/memory.c >>> index 503d493263df..b093df652c11 100644 >>> --- a/mm/memory.c >>> +++ b/mm/memory.c >>> @@ -5368,16 +5368,43 @@ static vm_fault_t do_fault(struct vm_fault *vmf) >>> return ret; >>> } >>> >>> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf, >>> - unsigned long addr, int page_nid, int *flags) >>> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf, >>> + unsigned long addr, int *flags, >>> + bool writable, int *last_cpupid) >>> { >>> struct vm_area_struct *vma = vmf->vma; >>> >>> + /* >>> + * Avoid grouping on RO pages in general. RO pages shouldn't hurt as >>> + * much anyway since they can be in shared cache state. This misses >>> + * the case where a mapping is writable but the process never writes >>> + * to it but pte_write gets cleared during protection updates and >>> + * pte_dirty has unpredictable behaviour between PTE scan updates, >>> + * background writeback, dirty balancing and application behaviour. >>> + */ >>> + if (!writable) >>> + *flags |= TNF_NO_GROUP; >>> + >>> + /* >>> + * Flag if the folio is shared between multiple address spaces. This >>> + * is later used when determining whether to group tasks together >>> + */ >>> + if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED)) >>> + *flags |= TNF_SHARED; >>> + /* >>> + * For memory tiering mode, cpupid of slow memory page is used >>> + * to record page access time. So use default value. >>> + */ >>> + if (folio_use_access_time(folio)) >>> + *last_cpupid = (-1 & LAST_CPUPID_MASK); >>> + else >>> + *last_cpupid = folio_last_cpupid(folio); >>> + >>> /* Record the current PID acceesing VMA */ >>> vma_set_access_pid_bit(vma); >>> >>> count_vm_numa_event(NUMA_HINT_FAULTS); >>> - if (page_nid == numa_node_id()) { >>> + if (folio_nid(folio) == numa_node_id()) { >>> count_vm_numa_event(NUMA_HINT_FAULTS_LOCAL); >>> *flags |= TNF_FAULT_LOCAL; >>> } >>> @@ -5442,13 +5469,13 @@ static void numa_rebuild_large_mapping(struct vm_fault *vmf, struct vm_area_stru >>> static vm_fault_t do_numa_page(struct vm_fault *vmf) >>> { >>> struct vm_area_struct *vma = vmf->vma; >>> + pte_t old_pte = vmf->orig_pte; >> >> The usage of old_pte is different from other use cases. Where, >> >> old_pte = *pte; >> check old_pte and orig_pte >> generate new_pte from old_pte >> set new_pte >> >> We have used this before in do_numa_page(), but not do that now. But I >> still think that it's better to follow the convention partly if >> possible. This makes code easier to be read. I notices that we don't >> follow it in do_huge_pmd_numa_page(), we may change that? > > Sure, since I am trying to make do_numa_page() and do_huge_pmd_numa_page() > use the same pattern. > >> >>> + pte_t pte; >>> struct folio *folio = NULL; >>> int nid = NUMA_NO_NODE; >>> bool writable = false, ignore_writable = false; >>> bool pte_write_upgrade = vma_wants_manual_pte_write_upgrade(vma); >>> - int last_cpupid; >>> - int target_nid; >>> - pte_t pte, old_pte; >>> + int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK); >> >> Because we will initialize last_cpupid in numa_migrate_check(), we don't >> need to initialize it here? > > Will remove it. > > Thanks for the review. I attached my fixup and updated patch 2. Let me know how it looks. -- Best Regards, Yan, Zi