| Message ID | 20230216160528.2146188-3-fengwei.yin@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | minor cleanup of usage of flush_dcache_folio() | expand |
On Fri, Feb 17, 2023 at 12:05:28AM +0800, Yin Fengwei wrote: > zero_user_folio_segments() has same function as zero_user_segments(). > but take folio as parameter. Update folio_zero_segments(), > folio_zero_segment() and folio_zero_range() to use it. If we were going to do something like this, then it should be folio_zero_segments(), not zero_user_folio_segments(). But I don't understand the advantage to adding this right now. We're adding a new function that does exactly the same thing as zero_user_segments() for no real benefit that I can see. My plan was always to eventually kill off zero_user_segment() zero_user_segments() and zero_user(), and then move the implementation of zero_user_segments() to be the implementation of folio_zero_segments(). But we still have ~100 calls to those three functions, so we can't do that yet. If you're looking for something to do, how about batching calls to page_remove_rmap() in try_to_unmap_one()? That's a pretty hairy function, but I think that you'll see similar gains to the ones you saw with page_add_file_rmap() since it's manipulating the same counters.
On 2/17/2023 3:19 AM, Matthew Wilcox wrote: > On Fri, Feb 17, 2023 at 12:05:28AM +0800, Yin Fengwei wrote: >> zero_user_folio_segments() has same function as zero_user_segments(). >> but take folio as parameter. Update folio_zero_segments(), >> folio_zero_segment() and folio_zero_range() to use it. > > If we were going to do something like this, then it should be > folio_zero_segments(), not zero_user_folio_segments(). > > But I don't understand the advantage to adding this right now. > We're adding a new function that does exactly the same thing > as zero_user_segments() for no real benefit that I can see. > > My plan was always to eventually kill off zero_user_segment() > zero_user_segments() and zero_user(), and then move the > implementation of zero_user_segments() to be the implementation > of folio_zero_segments(). But we still have ~100 calls to > those three functions, so we can't do that yet. > > > If you're looking for something to do, how about batching calls to > page_remove_rmap() in try_to_unmap_one()? That's a pretty hairy > function, but I think that you'll see similar gains to the ones > you saw with page_add_file_rmap() since it's manipulating the > same counters. Yes. I am trying to make some change base on folio to get to know about folio's API. Thanks a lot for the suggestion and I will check how to batched page_remove_rmap() in try_to_unmap_one(). Regards Yin, Fengwei
Yin Fengwei wrote: > zero_user_folio_segments() has same function as zero_user_segments(). > but take folio as parameter. Update folio_zero_segments(), > folio_zero_segment() and folio_zero_range() to use it. > > Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> > --- > include/linux/highmem.h | 26 +++++++++++++++++--- > mm/highmem.c | 53 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 76 insertions(+), 3 deletions(-) > > diff --git a/include/linux/highmem.h b/include/linux/highmem.h > index b06254e76d99..0039116e416a 100644 > --- a/include/linux/highmem.h > +++ b/include/linux/highmem.h > @@ -266,6 +266,8 @@ static inline void tag_clear_highpage(struct page *page) > #ifdef CONFIG_HIGHMEM > void zero_user_segments(struct page *page, unsigned start1, unsigned end1, > unsigned start2, unsigned end2); > +void zero_user_folio_segments(struct folio *folio, unsigned start1, > + unsigned end1, unsigned start2, unsigned end2); > #else > static inline void zero_user_segments(struct page *page, > unsigned start1, unsigned end1, > @@ -286,6 +288,24 @@ static inline void zero_user_segments(struct page *page, > for (i = 0; i < compound_nr(page); i++) > flush_dcache_page(page + i); > } > + > +static inline void zero_user_folio_segments(struct folio *folio, > + unsigned start1, unsigned end1, > + unsigned start2, unsigned end2) > +{ > + void *kaddr = kmap_local_page(&folio->page); > + > + BUG_ON(end1 > folio_size(folio) || end2 > folio_size(folio)); > + > + if (end1 > start1) > + memset(kaddr + start1, 0, end1 - start1); > + > + if (end2 > start2) > + memset(kaddr + start2, 0, end2 - start2); > + > + kunmap_local(kaddr); > + flush_dcache_folio(folio); > +} > #endif > > static inline void zero_user_segment(struct page *page, > @@ -454,7 +474,7 @@ static inline size_t memcpy_from_file_folio(char *to, struct folio *folio, > static inline void folio_zero_segments(struct folio *folio, > size_t start1, size_t xend1, size_t start2, size_t xend2) > { > - zero_user_segments(&folio->page, start1, xend1, start2, xend2); > + zero_user_folio_segments(folio, start1, xend1, start2, xend2); > } > > /** > @@ -466,7 +486,7 @@ static inline void folio_zero_segments(struct folio *folio, > static inline void folio_zero_segment(struct folio *folio, > size_t start, size_t xend) > { > - zero_user_segments(&folio->page, start, xend, 0, 0); > + zero_user_folio_segments(folio, start, xend, 0, 0); > } > > /** > @@ -478,7 +498,7 @@ static inline void folio_zero_segment(struct folio *folio, > static inline void folio_zero_range(struct folio *folio, > size_t start, size_t length) > { > - zero_user_segments(&folio->page, start, start + length, 0, 0); > + zero_user_folio_segments(folio, start, start + length, 0, 0); > } > > #endif /* _LINUX_HIGHMEM_H */ > diff --git a/mm/highmem.c b/mm/highmem.c > index db251e77f98f..e234b249208f 100644 > --- a/mm/highmem.c > +++ b/mm/highmem.c > @@ -443,6 +443,59 @@ void zero_user_segments(struct page *page, unsigned start1, unsigned end1, > BUG_ON((start1 | start2 | end1 | end2) != 0); > } > EXPORT_SYMBOL(zero_user_segments); > + > +static inline void zero_user_folio_segment(struct folio *folio, FWIW this does not compile: s/zero_user_folio_segment/zero_user_folio_segments/ But I agree with Willy here that I don't see the point of this patch. Seems like a lot of extra code for no benefit. Ira > + unsigned start1, unsigned end1, > + unsigned start2, unsigned end2) > +{ > + void *kaddr; > + unsigned s; > + > + BUG_ON(end1 > folio_size(folio) || end2 > folio_size(folio)); > + > + if (start1 > start2) { > + swap(start1, start2); > + swap(end1, end2); > + } > + > + if (start1 >= end1) > + start1 = end1 = 0; > + > + if (start2 >= end2) > + start2 = end2 = 0; > + > + start2 = max_t(unsigned, end1, start2); > + s = start1; > + while((start1 < end1) || (start2 < end2)) { > + kaddr = kmap_local_folio(folio, > + offset_in_folio(folio, PAGE_ALIGN_DOWN(s))); > + > + if ((end2 > start2) && (end1 >= start1)) { > + unsigned this_end = min_t(unsigned, end2, > + PAGE_ALIGN_DOWN(start2) + PAGE_SIZE); > + > + memset(kaddr + offset_in_page(start2), 0, > + this_end - start2); > + > + start2 = this_end; > + } > + > + if (end1 > start1) { > + unsigned this_end = min_t(unsigned, end1, > + PAGE_ALIGN_DOWN(start1) + PAGE_SIZE); > + > + memset(kaddr + offset_in_page(start1), 0, > + this_end - start1); > + s = start1 = this_end; > + } else { > + s = start2; > + } > + kunmap_local(kaddr); > + } > + > + flush_dcache_folio(folio); > +} > +EXPORT_SYMBOL(zero_user_folio_segments); > #endif /* CONFIG_HIGHMEM */ > > #ifdef CONFIG_KMAP_LOCAL > -- > 2.30.2 > >
Hi Ira, On 2/27/2023 1:31 PM, Ira Weiny wrote: > Yin Fengwei wrote: >> zero_user_folio_segments() has same function as zero_user_segments(). >> but take folio as parameter. Update folio_zero_segments(), >> folio_zero_segment() and folio_zero_range() to use it. >> >> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> >> --- >> include/linux/highmem.h | 26 +++++++++++++++++--- >> mm/highmem.c | 53 +++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 76 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/highmem.h b/include/linux/highmem.h >> index b06254e76d99..0039116e416a 100644 >> --- a/include/linux/highmem.h >> +++ b/include/linux/highmem.h >> @@ -266,6 +266,8 @@ static inline void tag_clear_highpage(struct page *page) >> #ifdef CONFIG_HIGHMEM >> void zero_user_segments(struct page *page, unsigned start1, unsigned end1, >> unsigned start2, unsigned end2); >> +void zero_user_folio_segments(struct folio *folio, unsigned start1, >> + unsigned end1, unsigned start2, unsigned end2); >> #else >> static inline void zero_user_segments(struct page *page, >> unsigned start1, unsigned end1, >> @@ -286,6 +288,24 @@ static inline void zero_user_segments(struct page *page, >> for (i = 0; i < compound_nr(page); i++) >> flush_dcache_page(page + i); >> } >> + >> +static inline void zero_user_folio_segments(struct folio *folio, >> + unsigned start1, unsigned end1, >> + unsigned start2, unsigned end2) >> +{ >> + void *kaddr = kmap_local_page(&folio->page); >> + >> + BUG_ON(end1 > folio_size(folio) || end2 > folio_size(folio)); >> + >> + if (end1 > start1) >> + memset(kaddr + start1, 0, end1 - start1); >> + >> + if (end2 > start2) >> + memset(kaddr + start2, 0, end2 - start2); >> + >> + kunmap_local(kaddr); >> + flush_dcache_folio(folio); >> +} >> #endif >> >> static inline void zero_user_segment(struct page *page, >> @@ -454,7 +474,7 @@ static inline size_t memcpy_from_file_folio(char *to, struct folio *folio, >> static inline void folio_zero_segments(struct folio *folio, >> size_t start1, size_t xend1, size_t start2, size_t xend2) >> { >> - zero_user_segments(&folio->page, start1, xend1, start2, xend2); >> + zero_user_folio_segments(folio, start1, xend1, start2, xend2); >> } >> >> /** >> @@ -466,7 +486,7 @@ static inline void folio_zero_segments(struct folio *folio, >> static inline void folio_zero_segment(struct folio *folio, >> size_t start, size_t xend) >> { >> - zero_user_segments(&folio->page, start, xend, 0, 0); >> + zero_user_folio_segments(folio, start, xend, 0, 0); >> } >> >> /** >> @@ -478,7 +498,7 @@ static inline void folio_zero_segment(struct folio *folio, >> static inline void folio_zero_range(struct folio *folio, >> size_t start, size_t length) >> { >> - zero_user_segments(&folio->page, start, start + length, 0, 0); >> + zero_user_folio_segments(folio, start, start + length, 0, 0); >> } >> >> #endif /* _LINUX_HIGHMEM_H */ >> diff --git a/mm/highmem.c b/mm/highmem.c >> index db251e77f98f..e234b249208f 100644 >> --- a/mm/highmem.c >> +++ b/mm/highmem.c >> @@ -443,6 +443,59 @@ void zero_user_segments(struct page *page, unsigned start1, unsigned end1, >> BUG_ON((start1 | start2 | end1 | end2) != 0); >> } >> EXPORT_SYMBOL(zero_user_segments); >> + >> +static inline void zero_user_folio_segment(struct folio *folio, > > FWIW this does not compile: > > s/zero_user_folio_segment/zero_user_folio_segments/ Thanks for pointing this out. I got the build error report from LKP also. > > But I agree with Willy here that I don't see the point of this patch. > Seems like a lot of extra code for no benefit. Yes. Totally agree. Regards Yin, Fengwei > > Ira > > >> + unsigned start1, unsigned end1, >> + unsigned start2, unsigned end2) >> +{ >> + void *kaddr; >> + unsigned s; >> + >> + BUG_ON(end1 > folio_size(folio) || end2 > folio_size(folio)); >> + >> + if (start1 > start2) { >> + swap(start1, start2); >> + swap(end1, end2); >> + } >> + >> + if (start1 >= end1) >> + start1 = end1 = 0; >> + >> + if (start2 >= end2) >> + start2 = end2 = 0; >> + >> + start2 = max_t(unsigned, end1, start2); >> + s = start1; >> + while((start1 < end1) || (start2 < end2)) { >> + kaddr = kmap_local_folio(folio, >> + offset_in_folio(folio, PAGE_ALIGN_DOWN(s))); >> + >> + if ((end2 > start2) && (end1 >= start1)) { >> + unsigned this_end = min_t(unsigned, end2, >> + PAGE_ALIGN_DOWN(start2) + PAGE_SIZE); >> + >> + memset(kaddr + offset_in_page(start2), 0, >> + this_end - start2); >> + >> + start2 = this_end; >> + } >> + >> + if (end1 > start1) { >> + unsigned this_end = min_t(unsigned, end1, >> + PAGE_ALIGN_DOWN(start1) + PAGE_SIZE); >> + >> + memset(kaddr + offset_in_page(start1), 0, >> + this_end - start1); >> + s = start1 = this_end; >> + } else { >> + s = start2; >> + } >> + kunmap_local(kaddr); >> + } >> + >> + flush_dcache_folio(folio); >> +} >> +EXPORT_SYMBOL(zero_user_folio_segments); >> #endif /* CONFIG_HIGHMEM */ >> >> #ifdef CONFIG_KMAP_LOCAL >> -- >> 2.30.2 >> >> > >
diff --git a/include/linux/highmem.h b/include/linux/highmem.h index b06254e76d99..0039116e416a 100644 --- a/include/linux/highmem.h +++ b/include/linux/highmem.h @@ -266,6 +266,8 @@ static inline void tag_clear_highpage(struct page *page) #ifdef CONFIG_HIGHMEM void zero_user_segments(struct page *page, unsigned start1, unsigned end1, unsigned start2, unsigned end2); +void zero_user_folio_segments(struct folio *folio, unsigned start1, + unsigned end1, unsigned start2, unsigned end2); #else static inline void zero_user_segments(struct page *page, unsigned start1, unsigned end1, @@ -286,6 +288,24 @@ static inline void zero_user_segments(struct page *page, for (i = 0; i < compound_nr(page); i++) flush_dcache_page(page + i); } + +static inline void zero_user_folio_segments(struct folio *folio, + unsigned start1, unsigned end1, + unsigned start2, unsigned end2) +{ + void *kaddr = kmap_local_page(&folio->page); + + BUG_ON(end1 > folio_size(folio) || end2 > folio_size(folio)); + + if (end1 > start1) + memset(kaddr + start1, 0, end1 - start1); + + if (end2 > start2) + memset(kaddr + start2, 0, end2 - start2); + + kunmap_local(kaddr); + flush_dcache_folio(folio); +} #endif static inline void zero_user_segment(struct page *page, @@ -454,7 +474,7 @@ static inline size_t memcpy_from_file_folio(char *to, struct folio *folio, static inline void folio_zero_segments(struct folio *folio, size_t start1, size_t xend1, size_t start2, size_t xend2) { - zero_user_segments(&folio->page, start1, xend1, start2, xend2); + zero_user_folio_segments(folio, start1, xend1, start2, xend2); } /** @@ -466,7 +486,7 @@ static inline void folio_zero_segments(struct folio *folio, static inline void folio_zero_segment(struct folio *folio, size_t start, size_t xend) { - zero_user_segments(&folio->page, start, xend, 0, 0); + zero_user_folio_segments(folio, start, xend, 0, 0); } /** @@ -478,7 +498,7 @@ static inline void folio_zero_segment(struct folio *folio, static inline void folio_zero_range(struct folio *folio, size_t start, size_t length) { - zero_user_segments(&folio->page, start, start + length, 0, 0); + zero_user_folio_segments(folio, start, start + length, 0, 0); } #endif /* _LINUX_HIGHMEM_H */ diff --git a/mm/highmem.c b/mm/highmem.c index db251e77f98f..e234b249208f 100644 --- a/mm/highmem.c +++ b/mm/highmem.c @@ -443,6 +443,59 @@ void zero_user_segments(struct page *page, unsigned start1, unsigned end1, BUG_ON((start1 | start2 | end1 | end2) != 0); } EXPORT_SYMBOL(zero_user_segments); + +static inline void zero_user_folio_segment(struct folio *folio, + unsigned start1, unsigned end1, + unsigned start2, unsigned end2) +{ + void *kaddr; + unsigned s; + + BUG_ON(end1 > folio_size(folio) || end2 > folio_size(folio)); + + if (start1 > start2) { + swap(start1, start2); + swap(end1, end2); + } + + if (start1 >= end1) + start1 = end1 = 0; + + if (start2 >= end2) + start2 = end2 = 0; + + start2 = max_t(unsigned, end1, start2); + s = start1; + while((start1 < end1) || (start2 < end2)) { + kaddr = kmap_local_folio(folio, + offset_in_folio(folio, PAGE_ALIGN_DOWN(s))); + + if ((end2 > start2) && (end1 >= start1)) { + unsigned this_end = min_t(unsigned, end2, + PAGE_ALIGN_DOWN(start2) + PAGE_SIZE); + + memset(kaddr + offset_in_page(start2), 0, + this_end - start2); + + start2 = this_end; + } + + if (end1 > start1) { + unsigned this_end = min_t(unsigned, end1, + PAGE_ALIGN_DOWN(start1) + PAGE_SIZE); + + memset(kaddr + offset_in_page(start1), 0, + this_end - start1); + s = start1 = this_end; + } else { + s = start2; + } + kunmap_local(kaddr); + } + + flush_dcache_folio(folio); +} +EXPORT_SYMBOL(zero_user_folio_segments); #endif /* CONFIG_HIGHMEM */ #ifdef CONFIG_KMAP_LOCAL
zero_user_folio_segments() has same function as zero_user_segments(). but take folio as parameter. Update folio_zero_segments(), folio_zero_segment() and folio_zero_range() to use it. Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> --- include/linux/highmem.h | 26 +++++++++++++++++--- mm/highmem.c | 53 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-)