Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 3 Jun 2021 14:36:11 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Matthew Wilcox <willy@infradead.org>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Yang Shi <shy828301@gmail.com>,
        Wang Yugui <wangyugui@e16-tech.com>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Alistair Popple <apopple@nvidia.com>,
        Ralph Campbell <rcampbell@nvidia.com>, Zi Yan <ziy@nvidia.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Minchan Kim <minchan@kernel.org>, Jue Wang <juew@google.com>,
        Peter Xu <peterx@redhat.com>, Jan Kara <jack@suse.cz>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/7] mm/thp: fix vma_address() if virtual address below
 file offset
In-Reply-To: <YLanDVVbKwwPmW/0@casper.infradead.org>
Message-ID: <alpine.LSU.2.11.2106031427200.11704@eggly.anvils>
References: <alpine.LSU.2.11.2106011353270.2148@eggly.anvils> <alpine.LSU.2.11.2106011408020.2148@eggly.anvils> <YLanDVVbKwwPmW/0@casper.infradead.org>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Precedence: bulk

On Tue, 1 Jun 2021, Matthew Wilcox wrote:
> On Tue, Jun 01, 2021 at 02:09:31PM -0700, Hugh Dickins wrote:
> >  static inline unsigned long
> > -__vma_address(struct page *page, struct vm_area_struct *vma)
> > +vma_address(struct page *page, struct vm_area_struct *vma)
> >  {
> > -	pgoff_t pgoff = page_to_pgoff(page);
> > -	return vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
> > +	pgoff_t pgoff;
> > +	unsigned long address;
> > +
> > +	VM_BUG_ON_PAGE(PageKsm(page), page);	/* KSM page->index unusable */
> > +	pgoff = page_to_pgoff(page);
> > +	if (pgoff >= vma->vm_pgoff) {
> > +		address = vma->vm_start +
> > +			((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
> > +		/* Check for address beyond vma (or wrapped through 0?) */
> > +		if (address < vma->vm_start || address >= vma->vm_end)
> > +			address = -EFAULT;
> > +	} else if (PageHead(page) &&
> > +		   pgoff + (1UL << compound_order(page)) > vma->vm_pgoff) {
> 
> 	} else if (PageHead(page) &&
> 		   pgoff + compound_nr(page) > vma->vm_pgoff) {

Yes, that's better, thanks.

I was tempted to leave out the preliminary PageHead() test altogether;
but you didn't suggest that, and I've concluded that perhaps it makes
for good documentation, even though not strictly needed.

> 
> > +vma_address_end(struct page *page, struct vm_area_struct *vma)
> >  {
> > +	pgoff_t pgoff;
> > +	unsigned long address;
> > +
> > +	VM_BUG_ON_PAGE(PageKsm(page), page);	/* KSM page->index unusable */
> > +	pgoff = page_to_pgoff(page);
> > +	if (PageHead(page))
> > +		pgoff += 1UL << compound_order(page);
> > +	else
> > +		pgoff++;
> 
> Again, can use compound_nr here.  In fact, the whole thing can be:
> 
> 	pgoff += compound_nr(page);

Even nicer; and I've taken the liberty of just saying

	pgoff = page_to_pgoff(page) + compound_nr(page);

v2 of this patch coming up now, thanks.

Hugh