| Message ID | 20210915181538.11288-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: A few cleanup patches around zap, shmem and uffd | expand |
On Wed, 15 Sep 2021, Peter Xu wrote: > Use the helper for the checks. Rename "check_mapping" into "zap_mapping" > because "check_mapping" looks like a bool but in fact it stores the mapping > itself. When it's set, we check the mapping (it must be non-NULL). When it's > cleared we skip the check, which works like the old way. > > Move the duplicated comments to the helper too. > > Reviewed-by: Alistair Popple <apopple@nvidia.com> > Signed-off-by: Peter Xu <peterx@redhat.com> Again, I won't NAK, but I have no enthusiasm for this at all: our tastes clearly differ. I don't find the new name helpful, I don't find the separated "helper" helpful, and you have hidden the helpful comment (but I'd be on firmer ground if the unmap_shared_mapping_pages() it referred to had ever existed! perhaps it was in an intermediate tree). But then I would feel this way, wouldn't I? See dd9fd0e03de ("[PATCH] rmap: nonlinear truncation") in //git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git I'm glad to see that you have dropped 5/5 for now: I was not keen on that one either. Hugh > --- > include/linux/mm.h | 16 +++++++++++++++- > mm/memory.c | 29 ++++++----------------------- > 2 files changed, 21 insertions(+), 24 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index d1126f731221..ed44f31615d9 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1721,10 +1721,24 @@ extern void user_shm_unlock(size_t, struct ucounts *); > * Parameter block passed down to zap_pte_range in exceptional cases. > */ > struct zap_details { > - struct address_space *check_mapping; /* Check page->mapping if set */ > + struct address_space *zap_mapping; /* Check page->mapping if set */ > struct page *single_page; /* Locked page to be unmapped */ > }; > > +/* > + * We set details->zap_mappings when we want to unmap shared but keep private > + * pages. Return true if skip zapping this page, false otherwise. > + */ > +static inline bool > +zap_skip_check_mapping(struct zap_details *details, struct page *page) > +{ > + if (!details || !page) > + return false; > + > + return details->zap_mapping && > + (details->zap_mapping != page_rmapping(page)); > +} > + > struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, > pte_t pte); > struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, > diff --git a/mm/memory.c b/mm/memory.c > index a7e427177817..8db8ce0ca6ce 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1333,16 +1333,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > struct page *page; > > page = vm_normal_page(vma, addr, ptent); > - if (unlikely(details) && page) { > - /* > - * unmap_shared_mapping_pages() wants to > - * invalidate cache without truncating: > - * unmap shared but keep private pages. > - */ > - if (details->check_mapping && > - details->check_mapping != page_rmapping(page)) > - continue; > - } > + if (unlikely(zap_skip_check_mapping(details, page))) > + continue; > ptent = ptep_get_and_clear_full(mm, addr, pte, > tlb->fullmm); > tlb_remove_tlb_entry(tlb, pte, addr); > @@ -1375,17 +1367,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > is_device_exclusive_entry(entry)) { > struct page *page = pfn_swap_entry_to_page(entry); > > - if (unlikely(details && details->check_mapping)) { > - /* > - * unmap_shared_mapping_pages() wants to > - * invalidate cache without truncating: > - * unmap shared but keep private pages. > - */ > - if (details->check_mapping != > - page_rmapping(page)) > - continue; > - } > - > + if (unlikely(zap_skip_check_mapping(details, page))) > + continue; > pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); > rss[mm_counter(page)]--; > > @@ -3369,7 +3352,7 @@ void unmap_mapping_page(struct page *page) > first_index = page->index; > last_index = page->index + thp_nr_pages(page) - 1; > > - details.check_mapping = mapping; > + details.zap_mapping = mapping; > details.single_page = page; > > i_mmap_lock_write(mapping); > @@ -3398,7 +3381,7 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start, > pgoff_t first_index = start; > pgoff_t last_index = start + nr - 1; > > - details.check_mapping = even_cows ? NULL : mapping; > + details.zap_mapping = even_cows ? NULL : mapping; > if (last_index < first_index) > last_index = ULONG_MAX; > > -- > 2.31.1
On 15.09.21 20:15, Peter Xu wrote: > Use the helper for the checks. Rename "check_mapping" into "zap_mapping" > because "check_mapping" looks like a bool but in fact it stores the mapping > itself. When it's set, we check the mapping (it must be non-NULL). When it's > cleared we skip the check, which works like the old way. > > Move the duplicated comments to the helper too. > > Reviewed-by: Alistair Popple <apopple@nvidia.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/linux/mm.h | 16 +++++++++++++++- > mm/memory.c | 29 ++++++----------------------- > 2 files changed, 21 insertions(+), 24 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index d1126f731221..ed44f31615d9 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1721,10 +1721,24 @@ extern void user_shm_unlock(size_t, struct ucounts *); > * Parameter block passed down to zap_pte_range in exceptional cases. > */ > struct zap_details { > - struct address_space *check_mapping; /* Check page->mapping if set */ > + struct address_space *zap_mapping; /* Check page->mapping if set */ > struct page *single_page; /* Locked page to be unmapped */ > }; > > +/* > + * We set details->zap_mappings when we want to unmap shared but keep private > + * pages. Return true if skip zapping this page, false otherwise. > + */ > +static inline bool > +zap_skip_check_mapping(struct zap_details *details, struct page *page) I agree with Hugh that the name of this helper is suboptimal. What about inverting the conditions and getting static inline bool should_zap_page() { ... } The calling code is then if (unlikely(!should_zap_page(details, page))) continue; I don't really like renaming "zap_mapping", again, because it's contained within "struct zap_details" already. Factoring this out into a helper sounds like a good idea to me. Clear case of code de-duplication.
diff --git a/include/linux/mm.h b/include/linux/mm.h index d1126f731221..ed44f31615d9 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1721,10 +1721,24 @@ extern void user_shm_unlock(size_t, struct ucounts *); * Parameter block passed down to zap_pte_range in exceptional cases. */ struct zap_details { - struct address_space *check_mapping; /* Check page->mapping if set */ + struct address_space *zap_mapping; /* Check page->mapping if set */ struct page *single_page; /* Locked page to be unmapped */ }; +/* + * We set details->zap_mappings when we want to unmap shared but keep private + * pages. Return true if skip zapping this page, false otherwise. + */ +static inline bool +zap_skip_check_mapping(struct zap_details *details, struct page *page) +{ + if (!details || !page) + return false; + + return details->zap_mapping && + (details->zap_mapping != page_rmapping(page)); +} + struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, pte_t pte); struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, diff --git a/mm/memory.c b/mm/memory.c index a7e427177817..8db8ce0ca6ce 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -1333,16 +1333,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, struct page *page; page = vm_normal_page(vma, addr, ptent); - if (unlikely(details) && page) { - /* - * unmap_shared_mapping_pages() wants to - * invalidate cache without truncating: - * unmap shared but keep private pages. - */ - if (details->check_mapping && - details->check_mapping != page_rmapping(page)) - continue; - } + if (unlikely(zap_skip_check_mapping(details, page))) + continue; ptent = ptep_get_and_clear_full(mm, addr, pte, tlb->fullmm); tlb_remove_tlb_entry(tlb, pte, addr); @@ -1375,17 +1367,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, is_device_exclusive_entry(entry)) { struct page *page = pfn_swap_entry_to_page(entry); - if (unlikely(details && details->check_mapping)) { - /* - * unmap_shared_mapping_pages() wants to - * invalidate cache without truncating: - * unmap shared but keep private pages. - */ - if (details->check_mapping != - page_rmapping(page)) - continue; - } - + if (unlikely(zap_skip_check_mapping(details, page))) + continue; pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); rss[mm_counter(page)]--; @@ -3369,7 +3352,7 @@ void unmap_mapping_page(struct page *page) first_index = page->index; last_index = page->index + thp_nr_pages(page) - 1; - details.check_mapping = mapping; + details.zap_mapping = mapping; details.single_page = page; i_mmap_lock_write(mapping); @@ -3398,7 +3381,7 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start, pgoff_t first_index = start; pgoff_t last_index = start + nr - 1; - details.check_mapping = even_cows ? NULL : mapping; + details.zap_mapping = even_cows ? NULL : mapping; if (last_index < first_index) last_index = ULONG_MAX;