MIME-Version: 1.0
Date:   Fri, 10 Feb 2023 02:33:47 +0000
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
From:   "Yajun Deng" <yajun.deng@linux.dev>
Message-ID: <626a5f4c4996f57631a8e1877c7646e5@linux.dev>
Subject: Re: [PATCH v2] mm/page_alloc: optimize find_suitable_fallback()
 and fallbacks array
To:     "Zi Yan" <ziy@nvidia.com>
Cc:     akpm@linux-foundation.org, mgorman@techsingularity.net,
        david@redhat.com, vbabka@suse.cz, rppt@linux.ibm.com,
        osalvador@suse.de, rppt@kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
In-Reply-To: <494D9F5D-33A4-48B4-911B-9A75CFC9BC67@nvidia.com>
References: <494D9F5D-33A4-48B4-911B-9A75CFC9BC67@nvidia.com>
 <4C196D76-49A9-4B06-A51F-D8A13109DF3B@nvidia.com>
 <20230209101144.496144-1-yajun.deng@linux.dev>
 <ddc2d9001ef9d44651b62869ff9575b6@linux.dev>
Precedence: bulk

February 10, 2023 10:14 AM, "Zi Yan" <ziy@nvidia.com> wrote:=0A=0A> On 9 =
Feb 2023, at 20:57, Yajun Deng wrote:=0A> =0A>> February 9, 2023 11:50 PM=
, "Zi Yan" <ziy@nvidia.com> wrote:=0A>> =0A>>> On 9 Feb 2023, at 5:11, Ya=
jun Deng wrote:=0A>> =0A>> There is no need to execute the next loop if i=
t not return in the first=0A>> loop. So add a break at the end of the loo=
p.=0A>>> Can you explain why? If it is the case, MIGRATE_UNMOVABLE cannot=
 fall back=0A>>> to MIGRATE_MOVABLE? And MIGRATE_MOVABLE cannot fall back=
 to MIGRATE_UNMOVABLE?=0A>>> And MIGRATE_RECLAIMABLE cannot fall back to =
MIGRATE_MOVABLE?=0A>> =0A>> The return in the loop is only related to 'or=
der', 'migratetype' and 'only_stealable'=0A>> variables. Even if it execu=
te the next loop, it can't change the result. So the loop=0A>> can be bro=
ken if the first loop can't return.=0A> =0A> OK. Got it. Would the code b=
elow look better?=0A> =0A> for (i =3D 0; i < MIGRATE_PCPTYPES - 1 ; i++) =
{=0A> fallback_mt =3D fallbacks[migratetype][i];=0A> if (free_area_empty(=
area, fallback_mt))=0A> continue;=0A> }=0A> =0A> if (can_steal_fallback(o=
rder, migratetype))=0A> *can_steal =3D true;=0A> =0A> if (!only_stealable=
 || *can_steal)=0A> return fallback_mt;=0A> =0A> return -1;=0A> =0A=0AYes=
, I'll submit a v3 patch. =0AThanks.=0A=0A>> At the same time, add !migra=
tetype_is_mergeable() before the loop and=0A>> reduce the first index siz=
e from MIGRATE_TYPES to MIGRATE_PCPTYPES in=0A>> fallbacks array.=0A>>> Y=
ou sent a patch: https://lore.kernel.org/all/20230203100132.1627787-1-yaj=
un.deng@linux.dev/T/#u,=0A>>> why not squash this one into that? Why do=
=0A>>> we need two separate small patches working on the same code?=0A>> =
=0A>> Yes, this is better, but I overlooked this one when I sent the firs=
t patch. It is already merged.=0A>> =0A>> As Vlastimil Babka said, reduce=
 the first index from MIGRATE_TYPES to MIGRATE_PCPTYPES may be=0A>> cause=
 out of bounds access of the shrinked fallbacks array If we don't judge t=
he range of=0A>> migratetype. But this doesn't happen with the second ind=
ex.=0A>> =0A>>> Thanks.=0A>> =0A>> Signed-off-by: Yajun Deng <yajun.deng@=
linux.dev>=0A>> Acked-by: Vlastimil Babka <vbabka@suse.cz>=0A>> ---=0A>> =
include/linux/mmzone.h | 2 +-=0A>> mm/page_alloc.c | 11 +++++------=0A>> =
2 files changed, 6 insertions(+), 7 deletions(-)=0A>> =0A>> diff --git a/=
include/linux/mmzone.h b/include/linux/mmzone.h=0A>> index ab94985ee7d9..=
0a817b8c7fb2 100644=0A>> --- a/include/linux/mmzone.h=0A>> +++ b/include/=
linux/mmzone.h=0A>> @@ -85,7 +85,7 @@ static inline bool is_migrate_movab=
le(int mt)=0A>> * Check whether a migratetype can be merged with another =
migratetype.=0A>> *=0A>> * It is only mergeable when it can fall back to =
other migratetypes for=0A>> - * allocation. See fallbacks[MIGRATE_TYPES][=
3] in page_alloc.c.=0A>> + * allocation. See fallbacks[][] array in page_=
alloc.c.=0A>> */=0A>> static inline bool migratetype_is_mergeable(int mt)=
=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>> return -1;=0A=
>> =0A>> *can_steal =3D false;=0A>> @@ -2873,11 +2873,10 @@ int find_suit=
able_fallback(struct free_area *area, unsigned int order,=0A>> if (can_st=
eal_fallback(order, migratetype))=0A>> *can_steal =3D true;=0A>> =0A>> - =
if (!only_stealable)=0A>> - return fallback_mt;=0A>> -=0A>> - if (*can_st=
eal)=0A>> + if (!only_stealable || *can_steal)=0A>> return fallback_mt;=
=0A>> + else=0A>> + break;=0A>> }=0A>> =0A>> return -1;=0A>> --=0A>> 2.25=
.1=0A>>> --=0A>>> Best Regards,=0A>>> Yan, Zi=0A> =0A> --=0A> Best Regard=
s,=0A> Yan, Zi