[RFC PATCH 0/2] minor cleanup of usage of flush_dcache_folio()

[RFC PATCH 0/2] minor cleanup of usage of flush_dcache_folio()

[Yin Fengwei]

When tried to review the usage of flush_dcache_folio(), noticed several
unnecessary call to flush_dcache_folio(). Patch1 removes them.

Patch2 adds zero_user_folio_segment() which has same function as
zero_user_segment() but with folio as parameter. And use
flush_dcache_folio() to flush cache instead of loop call flush_dcache_page().

Test:
Boot and run firefox/email/editor with Qemu x86_64 system.

The base is next-20230214.

Yin Fengwei (2):
  mm: remove duplicated flush_dcache_folio()
  mm: add zero_user_folio_segments()

 fs/libfs.c              |  1 -
 include/linux/highmem.h | 26 +++++++++++++++++---
 mm/highmem.c            | 53 +++++++++++++++++++++++++++++++++++++++++
 mm/shmem.c              |  7 +-----
 4 files changed, 77 insertions(+), 10 deletions(-)

-- 
2.30.2

[RFC PATCH 1/2] mm: remove duplicated flush_dcache_folio()

[Yin Fengwei]

folio_zero_range() calls the flush_dcache_folio() already. Remove
unnecessary flush_dcache_folio() call.

Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
---
 fs/libfs.c | 1 -
 mm/shmem.c | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 4eda519c3002..d57370c8e382 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -543,7 +543,6 @@ EXPORT_SYMBOL(simple_setattr);
 static int simple_read_folio(struct file *file, struct folio *folio)
 {
 	folio_zero_range(folio, 0, folio_size(folio));
-	flush_dcache_folio(folio);
 	folio_mark_uptodate(folio);
 	folio_unlock(folio);
 	return 0;
diff --git a/mm/shmem.c b/mm/shmem.c
index 448f393d8ab2..66e50f0a15ab 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1401,7 +1401,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 				goto redirty;
 		}
 		folio_zero_range(folio, 0, folio_size(folio));
-		flush_dcache_folio(folio);
 		folio_mark_uptodate(folio);
 	}
 
@@ -2010,11 +2009,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
 	 * it now, lest undo on failure cancel our earlier guarantee.
 	 */
 	if (sgp != SGP_WRITE && !folio_test_uptodate(folio)) {
-		long i, n = folio_nr_pages(folio);
-
-		for (i = 0; i < n; i++)
-			clear_highpage(folio_page(folio, i));
-		flush_dcache_folio(folio);
+		folio_zero_range(folio, 0, folio_size(folio));
 		folio_mark_uptodate(folio);
 	}
 
-- 
2.30.2

Re: [RFC PATCH 1/2] mm: remove duplicated flush_dcache_folio()

[Ira Weiny]

Yin Fengwei wrote:
> folio_zero_range() calls the flush_dcache_folio() already. Remove
> unnecessary flush_dcache_folio() call.

The change is probably reasonable but this statement is not exactly true.

The detail is that flush_dcache_page() is already called and another loop
through the folio pages is unneeded.  Not to mention hiding these flush
calls is nice because it is so hard to know when to use them.

> 
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> ---
>  fs/libfs.c | 1 -
>  mm/shmem.c | 7 +------
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 4eda519c3002..d57370c8e382 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -543,7 +543,6 @@ EXPORT_SYMBOL(simple_setattr);
>  static int simple_read_folio(struct file *file, struct folio *folio)
>  {
>  	folio_zero_range(folio, 0, folio_size(folio));
> -	flush_dcache_folio(folio);
>  	folio_mark_uptodate(folio);
>  	folio_unlock(folio);
>  	return 0;
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 448f393d8ab2..66e50f0a15ab 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1401,7 +1401,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  				goto redirty;
>  		}
>  		folio_zero_range(folio, 0, folio_size(folio));
> -		flush_dcache_folio(folio);
>  		folio_mark_uptodate(folio);
>  	}
>  
> @@ -2010,11 +2009,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
>  	 * it now, lest undo on failure cancel our earlier guarantee.
>  	 */
>  	if (sgp != SGP_WRITE && !folio_test_uptodate(folio)) {
> -		long i, n = folio_nr_pages(folio);
> -
> -		for (i = 0; i < n; i++)
> -			clear_highpage(folio_page(folio, i));
> -		flush_dcache_folio(folio);
> +		folio_zero_range(folio, 0, folio_size(folio));

This is a separate optimization from what your cover letter explained.

Ira

Re: [RFC PATCH 1/2] mm: remove duplicated flush_dcache_folio()

[Yin, Fengwei]

Hi Ira,

On 2/27/2023 1:46 PM, Ira Weiny wrote:
> Yin Fengwei wrote:
>> folio_zero_range() calls the flush_dcache_folio() already. Remove
>> unnecessary flush_dcache_folio() call.
> 
> The change is probably reasonable but this statement is not exactly true.
> 
> The detail is that flush_dcache_page() is already called and another loop
> through the folio pages is unneeded.  Not to mention hiding these flush
> calls is nice because it is so hard to know when to use them.
Thanks again for checking the patch and sharing the comments.

Yes. This patch mainly focus on the unneeded dcache flush when I checked the
flush_dcache_folio() related code.


Regards
Yin, Fengwei

> 
>>
>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>> ---
>>  fs/libfs.c | 1 -
>>  mm/shmem.c | 7 +------
>>  2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/fs/libfs.c b/fs/libfs.c
>> index 4eda519c3002..d57370c8e382 100644
>> --- a/fs/libfs.c
>> +++ b/fs/libfs.c
>> @@ -543,7 +543,6 @@ EXPORT_SYMBOL(simple_setattr);
>>  static int simple_read_folio(struct file *file, struct folio *folio)
>>  {
>>  	folio_zero_range(folio, 0, folio_size(folio));
>> -	flush_dcache_folio(folio);
>>  	folio_mark_uptodate(folio);
>>  	folio_unlock(folio);
>>  	return 0;
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 448f393d8ab2..66e50f0a15ab 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1401,7 +1401,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>>  				goto redirty;
>>  		}
>>  		folio_zero_range(folio, 0, folio_size(folio));
>> -		flush_dcache_folio(folio);
>>  		folio_mark_uptodate(folio);
>>  	}
>>  
>> @@ -2010,11 +2009,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
>>  	 * it now, lest undo on failure cancel our earlier guarantee.
>>  	 */
>>  	if (sgp != SGP_WRITE && !folio_test_uptodate(folio)) {
>> -		long i, n = folio_nr_pages(folio);
>> -
>> -		for (i = 0; i < n; i++)
>> -			clear_highpage(folio_page(folio, i));
>> -		flush_dcache_folio(folio);
>> +		folio_zero_range(folio, 0, folio_size(folio));
> 
> This is a separate optimization from what your cover letter explained.
> 
> Ira

[RFC PATCH 2/2] mm: add zero_user_folio_segments()

[Yin Fengwei]

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,
+		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

Re: [RFC PATCH 2/2] mm: add zero_user_folio_segments()

[kernel test robot]

Hi Yin,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc8 next-20230216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yin-Fengwei/mm-remove-duplicated-flush_dcache_folio/20230217-004045
patch link:    https://lore.kernel.org/r/20230216160528.2146188-3-fengwei.yin%40intel.com
patch subject: [RFC PATCH 2/2] mm: add zero_user_folio_segments()
config: csky-defconfig (https://download.01.org/0day-ci/archive/20230217/202302170219.WWlo7XAx-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/24ce10c4a39cf847063c9d8ea4157882f13d7143
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yin-Fengwei/mm-remove-duplicated-flush_dcache_folio/20230217-004045
        git checkout 24ce10c4a39cf847063c9d8ea4157882f13d7143
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302170219.WWlo7XAx-lkp@intel.com/

All errors (new ones prefixed by >>):

   csky-linux-ld: mm/truncate.o: in function `truncate_inode_partial_folio':
   truncate.c:(.text+0x754): undefined reference to `zero_user_folio_segments'
>> csky-linux-ld: truncate.c:(.text+0x7e4): undefined reference to `zero_user_folio_segments'
   csky-linux-ld: mm/shmem.o: in function `shmem_get_folio_gfp.constprop.0':
   shmem.c:(.text+0x1cb8): undefined reference to `zero_user_folio_segments'
>> csky-linux-ld: shmem.c:(.text+0x1d40): undefined reference to `zero_user_folio_segments'
>> csky-linux-ld: mm/highmem.o:(___ksymtab+zero_user_folio_segments+0x0): undefined reference to `zero_user_folio_segments'
   csky-linux-ld: fs/libfs.o:libfs.c:(.text+0x614): more undefined references to `zero_user_folio_segments' follow

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH 2/2] mm: add zero_user_folio_segments()

[Matthew Wilcox]

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.

Re: [RFC PATCH 2/2] mm: add zero_user_folio_segments()

[Yin, Fengwei]

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

Re: [RFC PATCH 2/2] mm: add zero_user_folio_segments()

[kernel test robot]

Hi Yin,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc8 next-20230216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yin-Fengwei/mm-remove-duplicated-flush_dcache_folio/20230217-004045
patch link:    https://lore.kernel.org/r/20230216160528.2146188-3-fengwei.yin%40intel.com
patch subject: [RFC PATCH 2/2] mm: add zero_user_folio_segments()
config: arc-randconfig-r024-20230215 (https://download.01.org/0day-ci/archive/20230217/202302170306.e1RoAdR7-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/24ce10c4a39cf847063c9d8ea4157882f13d7143
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yin-Fengwei/mm-remove-duplicated-flush_dcache_folio/20230217-004045
        git checkout 24ce10c4a39cf847063c9d8ea4157882f13d7143
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302170306.e1RoAdR7-lkp@intel.com/

All errors (new ones prefixed by >>):

   arceb-elf-ld: mm/shmem.o: in function `shmem_writepage':
>> shmem.c:(.text+0x24d8): undefined reference to `zero_user_folio_segments'
>> arceb-elf-ld: shmem.c:(.text+0x24d8): undefined reference to `zero_user_folio_segments'
   arceb-elf-ld: mm/shmem.o: in function `shmem_get_folio_gfp.constprop.0':
   shmem.c:(.text+0x2f4e): undefined reference to `zero_user_folio_segments'
   arceb-elf-ld: shmem.c:(.text+0x2f4e): undefined reference to `zero_user_folio_segments'
   arceb-elf-ld: fs/buffer.o: in function `block_read_full_folio':
>> buffer.c:(.text+0x3bee): undefined reference to `zero_user_folio_segments'
   arceb-elf-ld: fs/buffer.o:buffer.c:(.text+0x3bee): more undefined references to `zero_user_folio_segments' follow

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH 2/2] mm: add zero_user_folio_segments()

[Ira Weiny]

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
> 
> 

Re: [RFC PATCH 2/2] mm: add zero_user_folio_segments()

[Yin, Fengwei]

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
>>
>>
> 
>