Re: [PATCH] mm/page_alloc.c: eliminate unsigned confusion in __rmqueue_fallback

[Wei Yang <richard.weiyang@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

On Wed, Jun 21, 2017 at 08:55:28PM +0200, Rasmus Villemoes wrote:
>Since current_order starts as MAX_ORDER-1 and is then only
>decremented, the second half of the loop condition seems
>superfluous. However, if order is 0, we may decrement current_order
>past 0, making it UINT_MAX. This is obviously too subtle ([1], [2]).
>
>Since we need to add some comment anyway, change the two variables to
>signed, making the counting-down for loop look more familiar, and
>apparently also making gcc generate slightly smaller code.
>
>[1] https://lkml.org/lkml/2016/6/20/493
>[2] https://lkml.org/lkml/2017/6/19/345
>
>Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>---
>Michal, something like this, perhaps?
>
>mm/page_alloc.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>index 2302f250d6b1..e656f4da9772 100644
>--- a/mm/page_alloc.c
>+++ b/mm/page_alloc.c
>@@ -2204,19 +2204,23 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
>  * list of requested migratetype, possibly along with other pages from the same
>  * block, depending on fragmentation avoidance heuristics. Returns true if
>  * fallback was found so that __rmqueue_smallest() can grab it.
>+ *
>+ * The use of signed ints for order and current_order is a deliberate
>+ * deviation from the rest of this file, to make the for loop
>+ * condition simpler.
>  */
> static inline bool
>-__rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
>+__rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
> {
> 	struct free_area *area;
>-	unsigned int current_order;
>+	int current_order;
> 	struct page *page;
> 	int fallback_mt;
> 	bool can_steal;
> 
> 	/* Find the largest possible block of pages in the other list */
> 	for (current_order = MAX_ORDER-1;
>-				current_order >= order && current_order <= MAX_ORDER-1;
>+				current_order >= order;
> 				--current_order) {
> 		area = &(zone->free_area[current_order]);
> 		fallback_mt = find_suitable_fallback(area, current_order,
>-- 
>2.11.0

Looks nice. Why I didn't come up with this change.

Acked-by: Wei Yang <weiyang@gmail.com>

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]