| Message ID | 20210902201819.53343-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: A few cleanup patches around zap, shmem and uffd | expand |
On Friday, 3 September 2021 6:18:19 AM AEST 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. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/linux/mm.h | 15 ++++++++++++++- > mm/memory.c | 29 ++++++----------------------- > 2 files changed, 20 insertions(+), 24 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 69259229f090..81e402a5fbc9 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1720,10 +1720,23 @@ 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 != page_rmapping(page); Shouldn't this check still be details->zap_mapping && details->zap_mapping != page_rmapping(page)? Previously we wouldn't skip zapping pages if even_cows == true (ie. details->check_mapping == NULL). With this change the check when even_cows == true becomes NULL != page_rmapping(page). Doesn't this mean we will now skip zapping any pages with a mapping when even_cows == true? > +} > + > 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 6bba3b9fef7c..e5ee8399d270 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)]--; > > @@ -3368,7 +3351,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); > @@ -3396,7 +3379,7 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start, > pgoff_t first_index = start, last_index = start + nr - 1; > struct zap_details details = { }; > > - details.check_mapping = even_cows ? NULL : mapping; > + details.zap_mapping = even_cows ? NULL : mapping; > if (last_index < first_index) > last_index = ULONG_MAX; > >
On Fri, Sep 03, 2021 at 10:58:53AM +1000, Alistair Popple wrote: > On Friday, 3 September 2021 6:18:19 AM AEST 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. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/linux/mm.h | 15 ++++++++++++++- > > mm/memory.c | 29 ++++++----------------------- > > 2 files changed, 20 insertions(+), 24 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 69259229f090..81e402a5fbc9 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -1720,10 +1720,23 @@ 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 != page_rmapping(page); > > Shouldn't this check still be > details->zap_mapping && details->zap_mapping != page_rmapping(page)? > > Previously we wouldn't skip zapping pages if even_cows == true (ie. > details->check_mapping == NULL). With this change the check when > even_cows == true becomes NULL != page_rmapping(page). Doesn't this mean we > will now skip zapping any pages with a mapping when even_cows == true? Yes I think so. Thanks for pointing that out, Alistair, I'll fix in v3. But frankly I really think we should simply have that flag I used to introduce. It'll make everything much clearer.
On Friday, 3 September 2021 11:39:32 AM AEST Peter Xu wrote: > On Fri, Sep 03, 2021 at 10:58:53AM +1000, Alistair Popple wrote: > > On Friday, 3 September 2021 6:18:19 AM AEST 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. > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > include/linux/mm.h | 15 ++++++++++++++- > > > mm/memory.c | 29 ++++++----------------------- > > > 2 files changed, 20 insertions(+), 24 deletions(-) > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > index 69259229f090..81e402a5fbc9 100644 > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -1720,10 +1720,23 @@ 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 != page_rmapping(page); > > > > Shouldn't this check still be > > details->zap_mapping && details->zap_mapping != page_rmapping(page)? > > > > Previously we wouldn't skip zapping pages if even_cows == true (ie. > > details->check_mapping == NULL). With this change the check when > > even_cows == true becomes NULL != page_rmapping(page). Doesn't this mean we > > will now skip zapping any pages with a mapping when even_cows == true? > > Yes I think so. Thanks for pointing that out, Alistair, I'll fix in v3. > > But frankly I really think we should simply have that flag I used to introduce. > It'll make everything much clearer. Yeah, I think a flag would also be fine. - Alistair
On 03.09.21 03:50, Alistair Popple wrote: > On Friday, 3 September 2021 11:39:32 AM AEST Peter Xu wrote: >> On Fri, Sep 03, 2021 at 10:58:53AM +1000, Alistair Popple wrote: >>> On Friday, 3 September 2021 6:18:19 AM AEST 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. >>>> >>>> Signed-off-by: Peter Xu <peterx@redhat.com> >>>> --- >>>> include/linux/mm.h | 15 ++++++++++++++- >>>> mm/memory.c | 29 ++++++----------------------- >>>> 2 files changed, 20 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>>> index 69259229f090..81e402a5fbc9 100644 >>>> --- a/include/linux/mm.h >>>> +++ b/include/linux/mm.h >>>> @@ -1720,10 +1720,23 @@ 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 != page_rmapping(page); >>> >>> Shouldn't this check still be >>> details->zap_mapping && details->zap_mapping != page_rmapping(page)? >>> >>> Previously we wouldn't skip zapping pages if even_cows == true (ie. >>> details->check_mapping == NULL). With this change the check when >>> even_cows == true becomes NULL != page_rmapping(page). Doesn't this mean we >>> will now skip zapping any pages with a mapping when even_cows == true? >> >> Yes I think so. Thanks for pointing that out, Alistair, I'll fix in v3. >> >> But frankly I really think we should simply have that flag I used to introduce. >> It'll make everything much clearer. > > Yeah, I think a flag would also be fine. I still don't see the need for a flag quite frankly. Just factor out the checks we already have ... no change in behavior.
diff --git a/include/linux/mm.h b/include/linux/mm.h index 69259229f090..81e402a5fbc9 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1720,10 +1720,23 @@ 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 != 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 6bba3b9fef7c..e5ee8399d270 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)]--; @@ -3368,7 +3351,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); @@ -3396,7 +3379,7 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start, pgoff_t first_index = start, last_index = start + nr - 1; struct zap_details details = { }; - details.check_mapping = even_cows ? NULL : mapping; + details.zap_mapping = even_cows ? NULL : mapping; if (last_index < first_index) last_index = ULONG_MAX;
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. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/linux/mm.h | 15 ++++++++++++++- mm/memory.c | 29 ++++++----------------------- 2 files changed, 20 insertions(+), 24 deletions(-)