From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755013Ab2ITMgi (ORCPT ); Thu, 20 Sep 2012 08:36:38 -0400 Received: from mail-oa0-f46.google.com ([209.85.219.46]:44298 "EHLO mail-oa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754593Ab2ITMgg (ORCPT ); Thu, 20 Sep 2012 08:36:36 -0400 MIME-Version: 1.0 In-Reply-To: <20120919235156.GC13234@bbox> References: <20120919235156.GC13234@bbox> From: KOSAKI Motohiro Date: Thu, 20 Sep 2012 08:36:15 -0400 X-Google-Sender-Auth: swFvihfs53GOElpeNfs4iaG-LG0 Message-ID: Subject: Re: [PATCH] mm: fix NR_ISOLATED_[ANON|FILE] mismatch To: Minchan Kim Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Mel Gorman , Christoph Lameter , Johannes Weiner , David Rientjes , Vasiliy Kulikov Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 19, 2012 at 7:51 PM, Minchan Kim wrote: > On Wed, Sep 19, 2012 at 02:28:10PM -0400, Johannes Weiner wrote: >> On Wed, Sep 19, 2012 at 01:04:56PM -0400, KOSAKI Motohiro wrote: >> > On Wed, Sep 19, 2012 at 3:45 AM, Minchan Kim wrote: >> > > When I looked at zone stat mismatch problem, I found >> > > migrate_to_node doesn't decrease NR_ISOLATED_[ANON|FILE] >> > > if check_range fails. >> >> This is a bit misleading. It's not that the stats would be >> inaccurate, it's that the pages would be leaked from the LRU, no? >> >> > > It can make system hang out. >> >> Did you spot this by code review only or did you actually run into >> this? Because... >> >> > > Cc: KOSAKI Motohiro >> > > Cc: Mel Gorman >> > > Cc: Christoph Lameter >> > > Signed-off-by: Minchan Kim >> > > --- >> > > mm/mempolicy.c | 16 ++++++++-------- >> > > 1 file changed, 8 insertions(+), 8 deletions(-) >> > > >> > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c >> > > index 3d64b36..6bf0860 100644 >> > > --- a/mm/mempolicy.c >> > > +++ b/mm/mempolicy.c >> > > @@ -953,16 +953,16 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest, >> > > >> > > vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask, >> > > flags | MPOL_MF_DISCONTIG_OK, &pagelist); >> > > - if (IS_ERR(vma)) >> > > - return PTR_ERR(vma); >> > > - >> > > - if (!list_empty(&pagelist)) { >> > > + if (IS_ERR(vma)) { >> > > + err = PTR_ERR(vma); >> > > + goto out; >> > > + } >> > > + if (!list_empty(&pagelist)) >> > > err = migrate_pages(&pagelist, new_node_page, dest, >> > > false, MIGRATE_SYNC); >> > > - if (err) >> > > - putback_lru_pages(&pagelist); >> > > - } >> > > - >> > > +out: >> > > + if (err) >> > > + putback_lru_pages(&pagelist); >> > >> > Good catch! >> > This is a regression since following commit. So, I doubt we need >> > all or nothing semantics. Can we revert it instead? (and probably >> > we need more kind comment for preventing an accident) >> >> I think it makes sense to revert. Not because of the semantics, but I >> just don't see how check_range() could even fail for this callsite: >> >> 1. we pass mm->mmap->vm_start in there, so we should not fail due to >> find_vma() >> >> 2. we pass MPOL_MF_DISCONTIG_OK, so the discontig checks do not apply >> and so can not fail >> >> 3. we pass MPOL_MF_MOVE | MPOL_MF_MOVE_ALL, the page table loops will >> continue until addr == end, so we never fail with -EIO >> >> > commit 0def08e3acc2c9c934e4671487029aed52202d42 >> > Author: Vasiliy Kulikov >> > Date: Tue Oct 26 14:21:32 2010 -0700 >> > >> > mm/mempolicy.c: check return code of check_range >> >> We don't use this code to "check" the range, we use it to collect >> migrate pages. There is no failure case. >> > > Here it goes. > > From c2c21b551811e034eb0ede6806e0314b201d7e5b Mon Sep 17 00:00:00 2001 > From: Minchan Kim > Date: Thu, 20 Sep 2012 08:39:52 +0900 > Subject: [PATCH] mm: revert 0def08e3, mm/mempolicy.c: check return code of > check_range > > This patch reverts 0def08e3 because check_range can't fail in > migrate_to_node with considering current usecases. > > Quote from Johannes > " > I think it makes sense to revert. Not because of the semantics, but I > just don't see how check_range() could even fail for this callsite: > > 1. we pass mm->mmap->vm_start in there, so we should not fail due to > find_vma() > > 2. we pass MPOL_MF_DISCONTIG_OK, so the discontig checks do not apply > and so can not fail > > 3. we pass MPOL_MF_MOVE | MPOL_MF_MOVE_ALL, the page table loops will > continue until addr == end, so we never fail with -EIO > " > > And I add new VM_BUG_ON for checking migrate_to_node's future usecase > which might pass to MPOL_MF_STRICT. > > Cc: KOSAKI Motohiro > Cc: Mel Gorman > Cc: Christoph Lameter > Cc: David Rientjes > Cc: Vasiliy Kulikov > Suggested-by: Johannes Weiner > Signed-off-by: Minchan Kim > --- > mm/mempolicy.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 3d64b36..9ec87bd 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -946,15 +946,16 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest, > nodemask_t nmask; > LIST_HEAD(pagelist); > int err = 0; > - struct vm_area_struct *vma; > > nodes_clear(nmask); > node_set(source, nmask); > > - vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask, > + /* > + * Collect migrate pages and it shoudn't be failed. > + */ > + VM_BUG_ON(flags & MPOL_MF_STRICT); > + check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask, > flags | MPOL_MF_DISCONTIG_OK, &pagelist); > - if (IS_ERR(vma)) > - return PTR_ERR(vma); > > if (!list_empty(&pagelist)) { > err = migrate_pages(&pagelist, new_node_page, dest, Looks good. thank you. Acked-by: KOSAKI Motohiro