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> <1564080768.11067.22.camel@lca.pw>
In-Reply-To: <1564080768.11067.22.camel@lca.pw>
From:   Pengfei Li <lpf.vector@gmail.com>
Date:   Fri, 26 Jul 2019 07:48:36 +0800
Message-ID: <CAD7_sbEXQt0oHuD01BXdW2_=G4h8U8ogHVt0N1Yez2ajFJkShw@mail.gmail.com>
Subject: Re: [PATCH 00/10] make "order" unsigned int
To:     Qian Cai <cai@lca.pw>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        mgorman@techsingularity.net, mhocko@suse.com, vbabka@suse.cz,
        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"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 26, 2019 at 2:52 AM Qian Cai <cai@lca.pw> wrote:
>
> On Fri, 2019-07-26 at 02:42 +0800, Pengfei Li wrote:
> > Objective
> > ----
> > The motivation for this series of patches is use unsigned int for
> > "order" in compaction.c, just like in other memory subsystems.
>
> I suppose you will need more justification for this change. Right now, I don't

Thanks for your comments.

> see much real benefit apart from possibly introducing more regressions in those

As you can see, except for patch [05/10], other commits only modify the type
of "order". So the change is not big.

For the benefit, "order" may be negative, which is confusing and weird.
There is no good reason not to do this since it can be avoided.

> tricky areas of the code. Also, your testing seems quite lightweight.
>

Yes, you are right.
I use "stress" for stress testing, and made some small code coverage testing.

As you said, I need more ideas and comments about testing.
Any suggestions for testing?

Thanks again.

--
Pengfei

> >
> > In addition, did some cleanup about "order" in page_alloc
> > and vmscan.
> >
> >
> > Description
> > ----
> > Directly modifying the type of "order" to unsigned int is ok in most
> > places, because "order" is always non-negative.
> >
> > But there are two places that are special, one is next_search_order()
> > and the other is compact_node().
> >
> > For next_search_order(), order may be negative. It can be avoided by
> > some modifications.
> >
> > For compact_node(), order = -1 means performing manual compaction.
> > It can be avoided by specifying order = MAX_ORDER.
> >
> > Key changes in [PATCH 05/10] mm/compaction: make "order" and
> > "search_order" unsigned.
> >
> > More information can be obtained from commit messages.
> >
> >
> > Test
> > ----
> > I have done some stress testing locally and have not found any problems.
> >
> > In addition, local tests indicate no performance impact.
> >
> >
> > Pengfei Li (10):
> >   mm/page_alloc: use unsigned int for "order" in should_compact_retry()
> >   mm/page_alloc: use unsigned int for "order" in __rmqueue_fallback()
> >   mm/page_alloc: use unsigned int for "order" in should_compact_retry()
> >   mm/page_alloc: remove never used "order" in alloc_contig_range()
> >   mm/compaction: make "order" and "search_order" unsigned int in struct
> >     compact_control
> >   mm/compaction: make "order" unsigned int in compaction.c
> >   trace/events/compaction: make "order" unsigned int
> >   mm/compaction: use unsigned int for "compact_order_failed" in struct
> >     zone
> >   mm/compaction: use unsigned int for "kcompactd_max_order" in struct
> >     pglist_data
> >   mm/vmscan: use unsigned int for "kswapd_order" in struct pglist_data
> >
> >  include/linux/compaction.h        |  30 +++----
> >  include/linux/mmzone.h            |   8 +-
> >  include/trace/events/compaction.h |  40 +++++-----
> >  include/trace/events/kmem.h       |   6 +-
> >  include/trace/events/oom.h        |   6 +-
> >  include/trace/events/vmscan.h     |   4 +-
> >  mm/compaction.c                   | 127 +++++++++++++++---------------
> >  mm/internal.h                     |   6 +-
> >  mm/page_alloc.c                   |  16 ++--
> >  mm/vmscan.c                       |   6 +-
> >  10 files changed, 126 insertions(+), 123 deletions(-)
> >