Messages in this thread | | | From | Naoya Horiguchi <> | Subject | Re: [PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning | Date | Thu, 6 Dec 2012 20:22:42 -0500 |
| |
On Thu, Dec 06, 2012 at 02:40:08PM -0800, Andrew Morton wrote: ... > > On Wed, Dec 05, 2012 at 10:04:50PM +0000, Luck, Tony wrote: > > > if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) { > > > - if (PageAnon(page)) > > > + if (PageHuge(page)) > > > + ; > > > + else if (PageAnon(page)) > > > dec_mm_counter(mm, MM_ANONPAGES); > > > else > > > dec_mm_counter(mm, MM_FILEPAGES); > > > > > > This style minimizes the "diff" ... but wouldn't it be nicer to say: > > > > > > if (!PageHuge(page)) { > > > old code in here > > > } > > > > > > > I think this need more lines in diff because old code should be > > indented without any logical change. > > I do agree with Tony on this. While it is nice to keep the diff > looking simple, it is more important that the resulting code be clean > and idiomatic.
OK, I agree.
> --- a/mm/rmap.c~hwpoison-hugetlbfs-fix-rss-counter-warning-fix > +++ a/mm/rmap.c > @@ -1249,14 +1249,14 @@ int try_to_unmap_one(struct page *page, > update_hiwater_rss(mm); > > if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) { > - if (PageHuge(page)) > - ; > - else if (PageAnon(page)) > - dec_mm_counter(mm, MM_ANONPAGES); > - else > - dec_mm_counter(mm, MM_FILEPAGES); > - set_pte_at(mm, address, pte, > - swp_entry_to_pte(make_hwpoison_entry(page))); > + if (!PageHuge(page)) { > + if (PageAnon(page)) > + dec_mm_counter(mm, MM_ANONPAGES); > + else > + dec_mm_counter(mm, MM_FILEPAGES); > + set_pte_at(mm, address, pte, > + swp_entry_to_pte(make_hwpoison_entry(page))); > + }
This set_pte_at() should come outside the if-block, or error containment does not work.
Thanks, Naoya
> } else if (PageAnon(page)) { > swp_entry_t entry = { .val = page_private(page) }; > > _ >
| |