Re: [PATCH v2 7/7] iomap: Copy larger chunks from userspace

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

On 6/7/23 23:55, Matthew Wilcox wrote:
> On Wed, Jun 07, 2023 at 10:21:41AM +0800, Yin Fengwei wrote:
>> On 6/7/23 02:07, Matthew Wilcox wrote:
>>> +++ b/lib/iov_iter.c
>>> @@ -857,24 +857,36 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i)
>>>  }
>>>  EXPORT_SYMBOL(iov_iter_zero);
>>>  
>>> -size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes,
>>> -				  struct iov_iter *i)
>>> +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
>>> +		size_t bytes, struct iov_iter *i)
>>>  {
>>> -	char *kaddr = kmap_atomic(page), *p = kaddr + offset;
>>> -	if (!page_copy_sane(page, offset, bytes)) {
>>> -		kunmap_atomic(kaddr);
>>> +	size_t n = bytes, copied = 0;
>>> +
>>> +	if (!page_copy_sane(page, offset, bytes))
>>>  		return 0;
>>> -	}
>>> -	if (WARN_ON_ONCE(!i->data_source)) {
>>> -		kunmap_atomic(kaddr);
>>> +	if (WARN_ON_ONCE(!i->data_source))
>>>  		return 0;
>>> +
>>> +	page += offset / PAGE_SIZE;
>>> +	offset %= PAGE_SIZE;
>>> +	if (PageHighMem(page))
>>> +		n = min_t(size_t, bytes, PAGE_SIZE);
>> This is smart.
> 
> Thanks ;-)
> 
>>> +	while (1) {
>>> +		char *kaddr = kmap_atomic(page) + offset;
>>> +		iterate_and_advance(i, n, base, len, off,
>>> +			copyin(kaddr + off, base, len),
>>> +			memcpy_from_iter(i, kaddr + off, base, len)
>>> +		)
>>> +		kunmap_atomic(kaddr);
>>> +		copied += n;
>>> +		if (!PageHighMem(page) || copied == bytes || n == 0)
>>> +			break;
>> My understanding is copied == bytes could cover !PageHighMem(page).
> 
> It could!  But the PageHighMem test serves two purposes.  One is that
> it tells the human reader that this is all because of HighMem.  The
> other is that on !HIGHMEM systems it compiles to false:
> 
> PAGEFLAG_FALSE(HighMem, highmem)
> 
> static inline int Page##uname(const struct page *page) { return 0; }
> 
> So it tells the _compiler_ that all of this code is ignorable and
> it can optimise it out.  Now, you and I know that it will always
> be true, but it lets the compiler remove the test.  Hopefully the
> compiler can also see that:
> 
> 	while (1) {
> 		...
> 		if (true)
> 			break;
> 	}
> 
> means it can optimise away the entire loop structure and just produce
> the same code that it always did.
I thought about the first purpose. But the second purpose is new thing
I learned from this thread. Thanks a lot for detail explanation.


Regards
Yin, Fengwei