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@xxxxxxxxxx> -- Michal Hocko SUSE Labs
