From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx197.postini.com [74.125.245.197]) by kanga.kvack.org (Postfix) with SMTP id 43EBA6B0068 for ; Thu, 20 Sep 2012 11:41:26 -0400 (EDT) Date: Thu, 20 Sep 2012 11:41:11 -0400 From: Johannes Weiner Subject: Re: [PATCH] mm: fix NR_ISOLATED_[ANON|FILE] mismatch Message-ID: <20120920154111.GT1560@cmpxchg.org> References: <20120919235156.GC13234@bbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120919235156.GC13234@bbox> Sender: owner-linux-mm@kvack.org List-ID: To: Minchan Kim Cc: Andrew Morton , KOSAKI Motohiro , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Mel Gorman , Christoph Lameter , David Rientjes , Vasiliy Kulikov On Thu, Sep 20, 2012 at 08:51:56AM +0900, Minchan Kim wrote: > 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); Adding a check and a comment is a good idea, but I'm not a big fan of checking for MPOL_MF_STRICT in particular because it's one of the invalid inputs, and so you need to extend this check when somebody extends the spectrum of invalid inputs. I would much prefer checking directly for !(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) instead, which would also make the possible inputs apparent without having to chase up the call chain to find out what is usually passed in. And how about /* * This does not "check" the range but isolates all pages that * need migration. Between passing in the full user address * space range and MPOL_MF_DISCONTIG_OK, this call can not fail. */ ? > + 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, > -- > 1.7.9.5 > > -- > Kind regards, > Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org