Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190725184253.21160-1-lpf.vector@gmail.com> <20190725184253.21160-3-lpf.vector@gmail.com>
 <ac59714d-74d6-820c-37ea-5bf62cfc33a8@rasmusvillemoes.dk>
In-Reply-To: <ac59714d-74d6-820c-37ea-5bf62cfc33a8@rasmusvillemoes.dk>
From:   Pengfei Li <lpf.vector@gmail.com>
Date:   Sat, 27 Jul 2019 10:34:04 +0800
Message-ID: <CAD7_sbHTzPrQGEA259HU__-G7dvV_dZ-f3WavPavp-0WQzB4aA@mail.gmail.com>
Subject: Re: [PATCH 02/10] mm/page_alloc: use unsigned int for "order" in __rmqueue_fallback()
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        mgorman@techsingularity.net, mhocko@suse.com, vbabka@suse.cz,
        Qian Cai <cai@lca.pw>, aryabinin@virtuozzo.com,
        osalvador@suse.de, rostedt@goodmis.org, mingo@redhat.com,
        pavel.tatashin@microsoft.com, rppt@linux.ibm.com,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 26, 2019 at 5:36 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 25/07/2019 20.42, Pengfei Li wrote:
> > Because "order" will never be negative in __rmqueue_fallback(),
> > so just make "order" unsigned int.
> > And modify trace_mm_page_alloc_extfrag() accordingly.
> >
>
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 75c18f4fd66a..1432cbcd87cd 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2631,8 +2631,8 @@ static bool unreserve_highatomic_pageblock(const =
struct alloc_context *ac,
> >   * condition simpler.
> >   */
> >  static __always_inline bool
> > -__rmqueue_fallback(struct zone *zone, int order, int start_migratetype=
,
> > -                                             unsigned int alloc_flags)
> > +__rmqueue_fallback(struct zone *zone, unsigned int order,
> > +             int start_migratetype, unsigned int alloc_flags)
> >  {
>
> Please read the last paragraph of the comment above this function, run
> git blame to figure out when that was introduced, and then read the full
> commit description.

Thanks for your comments.

I have read the commit info of commit b002529d2563 ("mm/page_alloc.c:
eliminate unsigned confusion in __rmqueue_fallback").

And I looked at the discussion at https://lkml.org/lkml/2017/6/21/684 in de=
tail.

> Here be dragons. At the very least, this patch is
> wrong in that it makes that comment inaccurate.

I wonder if you noticed the commit 6bb154504f8b ("mm, page_alloc: spread
allocations across zones before introducing fragmentation").

Commit 6bb154504f8b introduces a local variable min_order in
__rmqueue_fallback().

And you can see

        for (current_order =3D MAX_ORDER - 1; current_order >=3D min_order;
                                --current_order) {

The =E2=80=9Ccurrent_order=E2=80=9D and "min_order"  are int, so here is ok=
.

Since __rmqueue_fallback() is only called by __rmqueue() and "order" is uns=
igned
int in __rmqueue(), then I think that making "order" is also unsigned
int is good.

Maybe I should also modify the comments here?

>
> Rasmus

Thank you again for your review.

--
Pengfei