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

["Yin, Fengwei" <fengwei.yin@intel.com>]

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