Re: [PATCH v4] mm/page_vma_mapped.c: Explicitly compare pfn for normal, hugetlbfs and THP page

[Michal Hocko <mhocko@kernel.org>]

On Wed 15-01-20 16:40:26, Mike Kravetz wrote:
> On 1/15/20 1:31 AM, Michal Hocko wrote:
> > On Tue 14-01-20 21:47:51, Li Xinhai wrote:
> >> On 2020-01-14 at 17:25 Michal Hocko wrote:
> >>> On Sat 11-01-20 10:18:05, Li Xinhai wrote:
> >>>> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
> >>>> The current implementation apply comparison as
> >>>> - normal 4K page: page_pfn <= pfn < page_pfn + 1
> >>>> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> >>>> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> >>>> in pfn_in_hpage. For hugetlbfs page, it should be
> >>>> page_pfn == pfn
> >>>>
> >>>> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
> >>>> is not only for THP and explicitly compare for these cases.
> >>>
> >>> Why is this important to do. I have asked and Mike had the same feeling
> >>> that the patch is missing any real justification. Why do we need this
> >>> change? It is great that you have dropped VM_BUG_ON btw.
> >>>
> >> I think it is important to make the code clear, as said, comparing hugetlbfs page
> >> in range page_pfn <= pfn < page_pfn + HPAGE_PMD_NR is confusion.
> > 
> > I am sorry but I do not really see a big confusion here. It is probably
> > a matter of taste. If others consider this an improvement then I will
> > not stand in the way but I consider the justification insuficient for
> > merging.
> 
> Perhaps I am confused, but the patch does update the check done for
> hugetlb pages.  IIUC, previously there was no distinction between THP
> pages and hugetlb pages in the check.  It is valid to pass in a sub-
> page of a THP page, but not that of a hugetlb page.
> 
> I do not believe it is possible for existing code to pass in a sub-page
> of a hugetlb page.  And, no one has ever seen this as an issue.  This
> is why I was questioning the need for the patch.

Exactly and that is the reason I fail the see a point. 

> With all that said, the new code/check is better (more complete) than
> the original.  It may not do anything for the current code base, but
> it 'could' catch potential errors in future code.  Because of this, I
> do consider the code an improvement.

It adds a branch for something that doesn't happen and also to a code
path which is quite internal to the MM AFAICS. That being said, if you
believe this is an improvement then I will not stand in the way. But
there are so many other places which could add checks that are not
exercised...

> Acked-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Michal Hocko
SUSE Labs