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
