On Fri, Aug 16, 2024 at 5:10 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote: > > > > On 15/08/2024 17:33, David Hildenbrand wrote: > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >> index 6df0e9f4f56c..c024ab0f745c 100644 > >> --- a/mm/huge_memory.c > >> +++ b/mm/huge_memory.c > >> @@ -3397,6 +3397,7 @@ int split_huge_page_to_list_to_order(struct page *page, struct list_head *list, > >> * page_deferred_list. > >> */ > >> list_del_init(&folio->_deferred_list); > >> + folio_clear_partially_mapped(folio); > >> } > >> spin_unlock(&ds_queue->split_queue_lock); > >> if (mapping) { > >> @@ -3453,11 +3454,12 @@ void __folio_undo_large_rmappable(struct folio *folio) > >> if (!list_empty(&folio->_deferred_list)) { > >> ds_queue->split_queue_len--; > >> list_del_init(&folio->_deferred_list); > >> + folio_clear_partially_mapped(folio); > >> } > >> spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); > >> } > >> -void deferred_split_folio(struct folio *folio) > >> +void deferred_split_folio(struct folio *folio, bool partially_mapped) > >> { > > /* We lost race with folio_put() */> list_del_init(&folio->_deferred_list); > > Was there some comment here? I just see ">" remove from the start of /* We lost race with folio_put() */ > > >> + folio_clear_partially_mapped(folio); > >> ds_queue->split_queue_len--; > >> } > >> if (!--sc->nr_to_scan) > >> @@ -3558,7 +3564,6 @@ static unsigned long deferred_split_scan(struct shrinker *shrink, > >> next: > >> folio_put(folio); > >> } > >> - > >> spin_lock_irqsave(&ds_queue->split_queue_lock, flags); > >> list_splice_tail(&list, &ds_queue->split_queue); > >> spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); > >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c > >> index 1fdd9eab240c..2ae2d9a18e40 100644 > >> --- a/mm/hugetlb.c > >> +++ b/mm/hugetlb.c > >> @@ -1758,6 +1758,7 @@ static void __update_and_free_hugetlb_folio(struct hstate *h, > >> free_gigantic_folio(folio, huge_page_order(h)); > >> } else { > >> INIT_LIST_HEAD(&folio->_deferred_list); > >> + folio_clear_partially_mapped(folio); > >> folio_put(folio); > >> } > >> } > >> diff --git a/mm/internal.h b/mm/internal.h > >> index 52f7fc4e8ac3..d64546b8d377 100644 > >> --- a/mm/internal.h > >> +++ b/mm/internal.h > >> @@ -662,8 +662,10 @@ static inline void prep_compound_head(struct page *page, unsigned int order) > >> atomic_set(&folio->_entire_mapcount, -1); > >> atomic_set(&folio->_nr_pages_mapped, 0); > >> atomic_set(&folio->_pincount, 0); > >> - if (order > 1) > >> + if (order > 1) { > >> INIT_LIST_HEAD(&folio->_deferred_list); > >> + folio_clear_partially_mapped(folio); > > > > Can we use the non-atomic version here? > > > > I believe we can use the non-atomic version in all places where set/clear is done as all set/clear are protected by ds_queue->split_queue_lock. So basically could replace all folio_set/clear_partially_mapped with __folio_set/clear_partially_mapped. > right. That is why I thought the below is actually safe. but i appreciate a test_set of course(and non-atomic): + if (!folio_test_partially_mapped(folio)) { + folio_set_partially_mapped(folio); + if (folio_test_pmd_mappable(folio)) + count_vm_event(THP_DEFERRED_SPLIT_PAGE); + count_mthp_stat(folio_order(folio), MTHP_STAT_SPLIT_DEFERRED); + } > But I guess its likely not going to make much difference? I will do it anyways in the next revision, rather than sending a fix patch. There haven't been any reviews for patch 5 so will wait a few days for any comments on that. > > Thanks Thanks Barry