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

[Pengfei Li <lpf.vector@gmail.com>]

On Fri, Jul 26, 2019 at 3:26 PM Mel Gorman <mgorman@techsingularity.net> wrote:
>

Thank you for your comments.

> On Fri, Jul 26, 2019 at 02:42:43AM +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.
> >
>
> Why? The series is relatively subtle in parts, particularly patch 5.

Before I sent this series of patches, I took a close look at the
git log for compact.c.

Here is a short history, trouble you to look patiently.

1) At first, "order" is _unsigned int_

The commit 56de7263fcf3 ("mm: compaction: direct compact when a
high-order allocation fails") introduced the "order" in
compact_control and its type is unsigned int.

Besides, you specify that order == -1 is the flag that triggers
compaction via proc.

2) Next, because order equals -1 is special, it causes an error.

The commit 7be62de99adc ("vmscan: kswapd carefully call compaction")
determines if "order" is less than 0.

This condition is always true because the type of "order" is
_unsigned int_.

-               compact_zone(zone, &cc);
+               if (cc->order < 0 || !compaction_deferred(zone))

3) Finally, in order to fix the above error, the type of the order
is modified to _int_

It is done by commit: aad6ec3777bf ("mm: compaction: make
compact_control order signed").

The reason I mention this is because I want to express that the
type of "order" is originally _unsigned int_. And "order" is
modified to _int_ because of the special value of -1.

If the special value of "order" is not a negative number (for
example, -1), but a number greater than MAX_ORDER - 1 (for example,
MAX_ORDER), then the "order" may still be _unsigned int_ now.

> There have been places where by it was important for order to be able to
> go negative due to loop exit conditions.

I think that even if "cc->order" is _unsigned int_, it can be done
with a local temporary variable easily.

Like this,

function(...)
{
    for(int tmp_order = cc->order; tmp_order >= 0; tmp_order--) {
        ...
    }
}

> If there was a gain from this
> or it was a cleanup in the context of another major body of work, I
> could understand the justification but that does not appear to be the
> case here.
>

My final conclusion:

Why "order" is _int_ instead of unsigned int?
  => Because order == -1 is used as the flag.
    => So what about making "order" greater than MAX_ORDER - 1?
      => The "order" can be _unsigned int_ just like in most places.

(Can we only pick -1 as this special value?)

This series of patches makes sense because,

1) It guarantees that "order" remains the same type.

No one likes to see this

__alloc_pages_slowpath(unsigned int order, ...)
 => should_compact_retry(int order, ...)            /* The type changed */
  => compaction_zonelist_suitable(int order, ...)
   => __compaction_suitable(int order, ...)
    => zone_watermark_ok(unsigned int order, ...)   /* The type
changed again! */

2) It eliminates the evil "order == -1".

If "order" is specified as any positive number greater than
MAX_ORDER - 1 in commit 56de7263fcf3, perhaps no int order will
appear in compact.c until now.

> --
> Mel Gorman

Thank you again for your comments, and sincerely thank you for
your patience in reading such a long email.

> SUSE Labs