Re: [PATCH 1/5] rmap: move hugetlb try_to_unmap to dedicated function

[Mike Kravetz <mike.kravetz@oracle.com>]

On 02/24/23 02:51, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Thu, Feb 23, 2023 at 05:28:10PM +0000, Matthew Wilcox wrote:
> > On Thu, Feb 23, 2023 at 04:31:56PM +0800, Yin Fengwei wrote:
> > > +
> > > +	if (folio_test_hwpoison(folio) && !(flags & TTU_IGNORE_HWPOISON)) {
> > > +		pteval = swp_entry_to_pte(make_hwpoison_entry(&folio->page));
> > > +		set_huge_pte_at(mm, address, pvmw.pte, pteval);
> > > +	}
> > > +
> > > +	/*** try_to_unmap_one() called dec_mm_counter for
> > > +	 * (folio_test_hwpoison(folio) && !(flags & TTU_IGNORE_HWPOISON)) not
> > > +	 * true case, looks incorrect. Change it to hugetlb_count_sub() here.
> > > +	 */
> > > +	hugetlb_count_sub(folio_nr_pages(folio), mm);
> 
> I have no objection to this change (moving hugetlb_count_sub() outside the
> if), but I have a question related to this.
> 
> Generally TTU_IGNORE_HWPOISON is used to control the pte-conversion based
> on page dirtiness.  But actually what it depends on is whether data lost
> happens when the page is forcibly dropped.  And for hugetlb pages, that's
> true regardless of PageDirty flag on it.
> So I think we can assume that try_to_unmap_one_hugetlb() is called with
> TTU_IGNORE_HWPOISON clear.  So maybe we don't need the if-check?

Thanks for looking at this Naoya!

In another reply I asked about the possibility of that if statement ever
being false for hugetlb pages.  Looks like that can never happen.  I went
back and looked at the memory failure/poison code just to be sure.

Yin,
Since we NEVER took went down the (folio_test_hwpoison(folio) &&
!(flags & TTU_IGNORE_HWPOISON)) not true path in the current code,
perhaps we not need the comment possibly calling dec_mm_counter. 
-- 
Mike Kravetz