linux-kernel - Re: [PATCH 00/10] make "order" unsigned int

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Jul 2019 07:48:36 +0800
From:   Pengfei Li <lpf.vector@...il.com>
To:     Qian Cai <cai@....pw>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        mgorman@...hsingularity.net, mhocko@...e.com, vbabka@...e.cz,
        aryabinin@...tuozzo.com, osalvador@...e.de, rostedt@...dmis.org,
        mingo@...hat.com, pavel.tatashin@...rosoft.com, rppt@...ux.ibm.com,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 00/10] make "order" unsigned int

On Fri, Jul 26, 2019 at 2:52 AM Qian Cai <cai@....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(-)
> >