On 2/25/2023 3:21 AM, Mike Kravetz wrote:
> 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.
Sure. I am going to remove this line and the comment of mm counter
also:
if (folio_test_hwpoison(folio) && !(flags & TTU_IGNORE_HWPOISON)) {
Will send out new version after few more days to see whether there
are comments to other patches in this series.
Thanks all for sharing the comments.
Regards
Yin, Fengwei
