On Fri, Jul 26, 2019 at 3:26 PM Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> 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
