From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa0-f46.google.com (mail-oa0-f46.google.com [209.85.219.46]) by kanga.kvack.org (Postfix) with ESMTP id 139436B0031 for ; Thu, 6 Mar 2014 21:06:55 -0500 (EST) Received: by mail-oa0-f46.google.com with SMTP id i7so3543190oag.19 for ; Thu, 06 Mar 2014 18:06:54 -0800 (PST) Received: from snt0-omc3-s51.snt0.hotmail.com (snt0-omc3-s51.snt0.hotmail.com. [65.54.51.88]) by mx.google.com with ESMTP id kb5si7282969obb.109.2014.03.06.18.06.54 for ; Thu, 06 Mar 2014 18:06:54 -0800 (PST) Message-ID: Date: Thu, 6 Mar 2014 20:06:54 -0600 From: TB Boxer In-Reply-To: References: <742FF125-8DCE-41BB-932F-6A2F8FDF3583@outlook.com> Subject: Re: [PATCHv2] mm/compaction: Break out of loop on !PageBuddy in isolate_freepages_block MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="531929be_66ef438d_99a" Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: Laura Abbott , linux-kernel@vger.kernel.org, Joonsoo Kim , Vlastimil Babka , Mel Gorman , linux-mm@kvack.org --531929be_66ef438d_99a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable TB Boxer liked your message with Boxer. On March 6, 2014 at 7:39:17 PM CS= T, TB Boxer wrote:TB Boxer liked your message = with Boxer. On March 6, 2014 at 7:18:40 PM CST, TB Boxer wrote:TB Boxer liked your message with Boxer. On March 6, 2014= at 6:33:49 PM CST, Andrew Morton wrote: = On Thu,=C2=A0 6 Mar 2014 10:21:32 -0800 Laura Abbott wrote: > We received several reports of bad page state when fre= eing CMA pages > previously allocated with alloc=5Fcontig=5Frange: > > <= 1>=5B 1258.084111=5D BUG: Bad page state in process Binder=5FA=C2=A0 pfn:= 63202 > <1>=5B 1258.089763=5D page:d21130b0 count:0 mapcount:1 mapping:=C2= =A0 (null) index:0x7dfbf > <1>=5B 1258.096109=5D page flags: 0x40080068(u= ptodate=7Clru=7Cactive=7Cswapbacked) > > Based on the page state, it loo= ks like the page was still in use. The page > flags do not make sense for= the use case though. =46urther debugging showed > that despite alloc=5Fc= ontig=5Frange returning success, at least one page in the > range still r= emained in the buddy allocator. > > There is an issue with isolate=5Ffre= epages=5Fblock. In strict mode (which CMA > uses), if any pages in the ra= nge cannot be isolated, > isolate=5Ffreepages=5Fblock should return failu= re 0. The current check keeps > track of the total number of isolated pag= es and compares against the size > of the range: > >=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 if (strict && nr=5Fstrict=5Frequired > total=5F= isolated) >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 total=5Fisolated =3D 0; > > After taki= ng the zone lock, if one of the pages in the range is not > in the buddy = allocator, we continue through the loop and do not > increment total=5Fis= olated. If in the last iteration of the loop we isolate > more than one p= age (e.g. last page needed is a higher order page), the > check for total= =5Fisolated may pass and we fail to detect that a page was > skipped. The= fix is to bail out if the loop immediately if we are in > strict mode. T= here's no benfit to continuing anyway since we need all > pages to be iso= lated. Additionally, drop the error checking based on > nr=5Fstrict=5Freq= uired and just check the pfn ranges. This matches with > what isolate=5Ff= reepages=5Frange does. > > --- a/mm/compaction.c > +++ b/mm/compaction.c= > =40=40 -242,7 +242,6 =40=40 static unsigned long isolate=5Ffreepages=5F= block(struct compact=5Fcontrol *cc, >=C2=A0 =7B >=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 int nr=5Fscanned =3D 0, total=5Fisolated =3D 0; >=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct page *cursor, *valid=5Fpage =3D= NULL; > -=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long nr=5Fstrict=5Frequired =3D= end=5Fpfn - blockpfn; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsign= ed long flags; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool locked =3D= false; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool checked=5Fpagebl= ock =3D false; > =40=40 -256,11 +255,12 =40=40 static unsigned long isola= te=5Ffreepages=5Fblock(struct compact=5Fcontrol *cc, >=C2=A0 >=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 nr=5Fscanned++; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (=21pfn=5Fvalid=5Fwithin(blockpfn= )) > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; > +=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto isolate=5Ffail; > + >=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 if (=21valid=5Fpage) >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 valid=5Fpage =3D page; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (=21PageBudd= y(page)) > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; > +=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto isolate=5Ffail; >=C2=A0 = >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 /* >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * The zone lock must be hel= d to isolate freepages. > =40=40 -289,12 +289,10 =40=40 static unsigned l= ong isolate=5Ffreepages=5Fblock(struct compact=5Fcontrol *cc, >=C2=A0 >=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 /* Recheck this is a buddy page under lock */ >=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= if (=21PageBuddy(page)) > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= continue; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto isolate= =5Ffail; >=C2=A0 >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* =46ound a free page, break it int= o order-0 pages */ >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 isolated =3D split=5Ffree=5Fpage(pag= e); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 if (=21isolated && strict) > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 break; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 total=5Fisolated +=3D isolated; >=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 for (i =3D 0; i < isolated; i++) =7B >=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 list=5Fadd(&page->lru, freelist); > = =40=40 -305,7 +303,15 =40=40 static unsigned long isolate=5Ffreepages=5Fb= lock(struct compact=5Fcontrol *cc, >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (isolated) =7B >=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 blockpfn +=3D= isolated - 1; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 cursor +=3D isolated - 1; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 continue; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =7D We can make the code a little more= efficient and (I think) clearer by moving that =60if (isolated)' test. = > + > +isolate=5Ffail: > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 if (strict) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 break; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 else > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= continue; > + And I don't think this =60continue' has any benefit. --= - a/mm/compaction.c=7Emm-compaction-break-out-of-loop-on-pagebuddy-in-iso= late=5Ffreepages=5Fblock-fix +++ a/mm/compaction.c =40=40 -293,14 +293,14= =40=40 static unsigned long isolate=5Ffreepages=5Fb =C2=A0 =C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* =46ound a free page, break it into order-0 pages */ =C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 isolated =3D split=5Ffree=5Fpage(page); -=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 total=5Fisol= ated +=3D isolated; -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for (i =3D 0; i < isolated; i++) =7B -=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 list=5Fadd(&page->lru= , freelist); -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = page++; -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 =7D - -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* If a page was split, advance to t= he end of it */ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (isolated) =7B +=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 total=5Fisolated +=3D isolated= ; +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for (i =3D 0= ; i < isolated; i++) =7B +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 list=5Fadd(&pag= e->lru, freelist); +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page++; +=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =7D + +=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* If a page was split, advance to t= he end of it */ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 blockpfn +=3D isolated - 1; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 cursor +=3D isolated - 1; =C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; =40=40= -309,9 +309,6 =40=40 static unsigned long isolate=5Ffreepages=5Fb =C2=A0= isolate=5Ffail: =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (strict) =C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break; -=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else -= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; - =C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =7D =C2=A0 =C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 trace=5Fmm=5Fcompaction=5Fisolate=5Ffreepa= ges(nr=5Fscanned, total=5Fisolated); Problem is, I can't be bothered te= sting this. -- To unsubscribe from this list: send the line =22unsubscri= be linux-kernel=22 in the body of a message to majordomo=40vger.kernel.or= g More majordomo info at=C2=A0 http://vger.kernel.org/majordomo-info.html= Please read the =46AQ at=C2=A0 http://www.tux.org/lkml/ = --531929be_66ef438d_99a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
TB Boxer liked your message with Boxer.


On March 6, 2014 at 7:39:17 PM CST, TB Boxer <boxerspam1=40ou= tlook.com> wrote:
<= html>
TB Boxer liked your message with Boxer.


On March 6, 2014 at 7:18:40 PM CST, TB Boxer <boxerspam1=40ou= tlook.com> wrote:
<= html>
TB Boxer liked your message with Boxer.


On March 6, 2014 at 6:33:49 PM CST, Andrew Morton <akpm=40lin= ux-foundation.org> wrote:
<=21-- converted from text -->
On Thu,  6 Mar 2014 10:21:32 -0800 Laur= a Abbott <lauraa=40codeaurora.org> wrote:

> We received several reports of bad page state when freeing CMA pages=
> previously allocated with alloc=5Fcontig=5Frange:
>
> <1>=5B 1258.084111=5D BUG: Bad page state in process Binder=5F= A  pfn:63202
> <1>=5B 1258.089763=5D page:d21130b0 count:0 mapcount:1 mapping= :  (null) index:0x7dfbf
> <1>=5B 1258.096109=5D page flags: 0x40080068(uptodate=7Clru=7C= active=7Cswapbacked)
>
> Based on the page state, it looks like the page was still in use. Th= e page
> flags do not make sense for the use case though. =46urther debugging= showed
> that despite alloc=5Fcontig=5Frange returning success, at least one = page in the
> range still remained in the buddy allocator.
>
> There is an issue with isolate=5Ffreepages=5Fblock. In strict mode (= which CMA
> uses), if any pages in the range cannot be isolated,
> isolate=5Ffreepages=5Fblock should return failure 0. The current che= ck keeps
> track of the total number of isolated pages and compares against the= size
> of the range:
>
>         if (strict &&= ; nr=5Fstrict=5Frequired > total=5Fisolated)
>           &nb= sp;     total=5Fisolated =3D 0;
>
> After taking the zone lock, if one of the pages in the range is not<= br> > in the buddy allocator, we continue through the loop and do not
= > increment total=5Fisolated. If in the last iteration of the loop we = isolate
> more than one page (e.g. last page needed is a higher order page), t= he
> check for total=5Fisolated may pass and we fail to detect that a pag= e was
> skipped. The fix is to bail out if the loop immediately if we are in=
> strict mode. There's no benfit to continuing anyway since we need al= l
> pages to be isolated. Additionally, drop the error checking based on=
> nr=5Fstrict=5Frequired and just check the pfn ranges. This matches w= ith
> what isolate=5Ffreepages=5Frange does.
>
> --- a/mm/compaction.c
> &=2343;&=2343;&=2343; b/mm/compaction.c
> =40=40 -242,7 &=2343;242,6 =40=40 static unsigned long isolate=5Ffre= epages=5Fblock(struct compact=5Fcontrol *cc,
>  =7B
>        int nr=5Fscanned =3D 0, to= tal=5Fisolated =3D 0;
>        struct page *cursor, *vali= d=5Fpage =3D NULL;
> -     unsigned long nr=5Fstrict=5Frequired =3D e= nd=5Fpfn - blockpfn;
>        unsigned long flags;
>        bool locked =3D false;
= >        bool checked=5Fpageblock =3D= false;
> =40=40 -256,11 &=2343;255,12 =40=40 static unsigned long isolate=5Ff= reepages=5Fblock(struct compact=5Fcontrol *cc,
> 
>           &nb= sp;    nr=5Fscanned&=2343;&=2343;;
>           &nb= sp;    if (=21pfn=5Fvalid=5Fwithin(blockpfn))
> -           &= nbsp;         continue;
> &=2343;          &= nbsp;          goto isolate=5F= fail;
> &=2343;
>           &nb= sp;    if (=21valid=5Fpage)
>           &nb= sp;            val= id=5Fpage =3D page;
>           &nb= sp;    if (=21PageBuddy(page))
> -           &= nbsp;         continue;
> &=2343;          &= nbsp;          goto isolate=5F= fail;
> 
>           &nb= sp;    /*
>           &nb= sp;     * The zone lock must be held to isolate freep= ages.
> =40=40 -289,12 &=2343;289,10 =40=40 static unsigned long isolate=5Ff= reepages=5Fblock(struct compact=5Fcontrol *cc,
> 
>           &nb= sp;    /* Recheck this is a buddy page under lock */
>           &nb= sp;    if (=21PageBuddy(page))
> -           &= nbsp;         continue;
> &=2343;          &= nbsp;          goto isolate=5F= fail;
> 
>           &nb= sp;    /* =46ound a free page, break it into order-0 pages= */
>           &nb= sp;    isolated =3D split=5Ffree=5Fpage(page);
> -           &= nbsp; if (=21isolated && strict)
> -           &= nbsp;         break;
>           &nb= sp;    total=5Fisolated &=2343;=3D isolated;
>           &nb= sp;    for (i =3D 0; i < isolated; i&=2343;&=2343;) =7B=
>           &nb= sp;            lis= t=5Fadd(&page->lru, freelist);
> =40=40 -305,7 &=2343;303,15 =40=40 static unsigned long isolate=5Ffr= eepages=5Fblock(struct compact=5Fcontrol *cc,
>           &nb= sp;    if (isolated) =7B
>           &nb= sp;            blo= ckpfn &=2343;=3D isolated - 1;
>           &nb= sp;            cur= sor &=2343;=3D isolated - 1;
> &=2343;          &= nbsp;          continue;
= >           &nb= sp;    =7D

We can make the code a little more efficient and (I think) clearer by
= moving that =60if (isolated)' test.

> &=2343;
> &=2343;isolate=5Ffail:
> &=2343;          &= nbsp;  if (strict)
> &=2343;          &= nbsp;          break;
> &=2343;          &= nbsp;  else
> &=2343;          &= nbsp;          continue;
= > &=2343;

And I don't think this =60continue' has any benefit.


--- a/mm/compaction.c=7Emm-compaction-break-out-of-loop-on-pagebuddy-in-i= solate=5Ffreepages=5Fblock-fix
&=2343;&=2343;&=2343; a/mm/compaction.c
=40=40 -293,14 &=2343;293,14 =40=40 static unsigned long isolate=5Ffreepa= ges=5Fb
 
            &= nbsp;    /* =46ound a free page, break it into order-0 pag= es */
            &= nbsp;    isolated =3D split=5Ffree=5Fpage(page);
-            =    total=5Fisolated &=2343;=3D isolated;
-            =    for (i =3D 0; i < isolated; i&=2343;&=2343;) =7B
-            =            list=5Fadd(&= amp;page->lru, freelist);
-            =            page&=2343;&= =2343;;
-            =    =7D
-
-            =    /* If a page was split, advance to the end of it */
            &= nbsp;    if (isolated) =7B
&=2343;           =             total=5F= isolated &=2343;=3D isolated;
&=2343;           =             for (i= =3D 0; i < isolated; i&=2343;&=2343;) =7B
&=2343;           =             &= nbsp;       list=5Fadd(&page->lru, f= reelist);
&=2343;           =             &= nbsp;       page&=2343;&=2343;;
&=2343;           =             =7D &=2343;
&=2343;           =             /* If = a page was split, advance to the end of it */
            &= nbsp;            b= lockpfn &=2343;=3D isolated - 1;
            &= nbsp;            c= ursor &=2343;=3D isolated - 1;
            &= nbsp;            c= ontinue;
=40=40 -309,9 &=2343;309,6 =40=40 static unsigned long isolate=5Ffreepage= s=5Fb
 isolate=5Ffail:
            &= nbsp;    if (strict)
            &= nbsp;            b= reak;
-            =    else
-            =            continue; -
         =7D
 
         trace=5Fmm=5Fcompaction=5F= isolate=5Ffreepages(nr=5Fscanned, total=5Fisolated);


Problem is, I can't be bothered testing this.

--
To unsubscribe from this list: send the line "unsubscribe linux-kern= el" in
the body of a message to majordomo=40vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the =46AQ at  h= ttp://www.tux.org/lkml/
--531929be_66ef438d_99a-- -- 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