MIME-Version: 1.0
Date:   Thu, 09 Feb 2023 08:44:02 +0000
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
From:   "Yajun Deng" <yajun.deng@linux.dev>
Message-ID: <7cc548bc6be5f84e1fb9c6cac07b7451@linux.dev>
Subject: Re: [PATCH] mm/page_alloc: optimize the loop in
 find_suitable_fallback()
To:     "Vlastimil Babka" <vbabka@suse.cz>, akpm@linux-foundation.org
Cc:     ziy@nvidia.com, mgorman@techsingularity.net, david@redhat.com,
        rppt@linux.ibm.com, osalvador@suse.de, rppt@kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
In-Reply-To: <362e1270-8a54-36f5-f7c0-d922b3d12860@suse.cz>
References: <362e1270-8a54-36f5-f7c0-d922b3d12860@suse.cz>
 <20230209024435.3392916-1-yajun.deng@linux.dev>
Precedence: bulk

February 9, 2023 4:12 PM, "Vlastimil Babka" <vbabka@suse.cz> wrote:=0A=0A=
> On 2/9/23 03:44, Yajun Deng wrote:=0A> =0A>> There is no need to execut=
e the next loop if it not return in the first=0A>> loop. So add a break a=
t the end of the loop.=0A>> =0A>> There are only three rows in fallbacks,=
 so reduce the first index size=0A>> from MIGRATE_TYPES to MIGRATE_PCPTYP=
ES.=0A>> =0A>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>=0A> =0A> =
Acked-by: Vlastimil Babka <vbabka@suse.cz>=0A> =0A>> ---=0A>> mm/page_all=
oc.c | 11 +++++------=0A>> 1 file changed, 5 insertions(+), 6 deletions(-=
)=0A>> =0A>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c=0A>> index 11=
13483fa6c5..536e8d838fb5 100644=0A>> --- a/mm/page_alloc.c=0A>> +++ b/mm/=
page_alloc.c=0A>> @@ -2603,7 +2603,7 @@ struct page *__rmqueue_smallest(s=
truct zone *zone, unsigned int order,=0A>> *=0A>> * The other migratetype=
s do not have fallbacks.=0A>> */=0A>> -static int fallbacks[MIGRATE_TYPES=
][MIGRATE_PCPTYPES - 1] =3D {=0A>> +static int fallbacks[MIGRATE_PCPTYPES=
][MIGRATE_PCPTYPES - 1] =3D {=0A>> [MIGRATE_UNMOVABLE] =3D { MIGRATE_RECL=
AIMABLE, MIGRATE_MOVABLE },=0A>> [MIGRATE_MOVABLE] =3D { MIGRATE_RECLAIMA=
BLE, MIGRATE_UNMOVABLE },=0A>> [MIGRATE_RECLAIMABLE] =3D { MIGRATE_UNMOVA=
BLE, MIGRATE_MOVABLE },=0A>> @@ -2861,7 +2861,7 @@ int find_suitable_fall=
back(struct free_area *area, unsigned int order,=0A>> int i;=0A>> int fal=
lback_mt;=0A>> =0A>> - if (area->nr_free =3D=3D 0)=0A>> + if (area->nr_fr=
ee =3D=3D 0 || !migratetype_is_mergeable(migratetype))=0A> =0A> Just curi=
ous, did you the check for extra safety or did you find (by running=0A> o=
r code inspection) that this can be indeed called with a non-mergeable=0A=
> migratetype, and cause out of bounds access of the shrinked fallbacks a=
rray?=0A> =0A=0ANo, I'm not sure if it is called with a non-mergeable mig=
ratetype.=0AIt is just for extra safety.=0A=0A> BTW, I noticed the commme=
nt on migratetype_is_mergeable() contains:=0A> =0A> "See fallbacks[MIGRAT=
E_TYPES][3] in page_alloc.c. "=0A> =0A> Should probably change it to e.g.=
 "See fallbacks[][] array ..." so we don't=0A> have to keep it in exact s=
ync...=0A> =0A=0AYes, this comment should be changed.=0ASo do I need to s=
ubmit a v2 patch?=0A=0A>> return -1;=0A>> =0A>> *can_steal =3D false;=0A>=
> @@ -2873,11 +2873,10 @@ int find_suitable_fallback(struct free_area *ar=
ea, unsigned int order,=0A>> if (can_steal_fallback(order, migratetype))=
=0A>> *can_steal =3D true;=0A>> =0A>> - if (!only_stealable)=0A>> - retur=
n fallback_mt;=0A>> -=0A>> - if (*can_steal)=0A>> + if (!only_stealable |=
| *can_steal)=0A>> return fallback_mt;=0A>> + else=0A>> + break;=0A>> }=
=0A>> =0A>> return -1;