Re: [PATCH v9 2/8] mm: Adjust shuffle code to allow for future coalescing

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Mon, Sep 09, 2019 at 08:22:11AM -0700, Alexander Duyck wrote:
> > > +	area = &zone->free_area[order];
> > > +	if (is_shuffle_order(order) ? shuffle_pick_tail() :
> > > +	    buddy_merge_likely(pfn, buddy_pfn, page, order))
> > 
> > Too loaded condition to my taste. Maybe
> > 
> > 	bool to_tail;
> > 	...
> > 	if (is_shuffle_order(order))
> > 		to_tail = shuffle_pick_tail();
> > 	else if (buddy_merge_likely(pfn, buddy_pfn, page, order))
> > 		to_tail = true;
> > 	else
> > 		to_tail = false;
> 
> I can do that, although I would tweak this slightly and do something more
> like:
>         if (is_shuffle_order(order))
>                 to_tail = shuffle_pick_tail();
>         else
>                 to_tail = buddy+_merge_likely(pfn, buddy_pfn, page, order);

Okay. Looks fine.

> > 	if (to_tail)
> > 		add_to_free_area_tail(page, area, migratetype);
> > 	else
> > 		add_to_free_area(page, area, migratetype);
> > 
> > > +		add_to_free_area_tail(page, area, migratetype);
> > >  	else
> > > -		add_to_free_area(page, &zone->free_area[order], migratetype);
> > > -
> > > +		add_to_free_area(page, area, migratetype);
> > >  }
> > >  
> > >  /*
> > > diff --git a/mm/shuffle.c b/mm/shuffle.c
> > > index 9ba542ecf335..345cb4347455 100644
> > > --- a/mm/shuffle.c
> > > +++ b/mm/shuffle.c
> > > @@ -4,7 +4,6 @@
> > >  #include <linux/mm.h>
> > >  #include <linux/init.h>
> > >  #include <linux/mmzone.h>
> > > -#include <linux/random.h>
> > >  #include <linux/moduleparam.h>
> > >  #include "internal.h"
> > >  #include "shuffle.h"
> > 
> > Why do you move #include <linux/random.h> from .c to .h?
> > It's not obvious to me.
> 
> Because I had originally put the shuffle logic in an inline function. I
> can undo that now as I when back to doing the randomness in the .c
> sometime v5 I believe.

Yes, please. It's needless change now.

> 
> > > @@ -190,8 +189,7 @@ struct batched_bit_entropy {
> > >  
> > >  static DEFINE_PER_CPU(struct batched_bit_entropy, batched_entropy_bool);
> > >  
> > > -void add_to_free_area_random(struct page *page, struct free_area *area,
> > > -		int migratetype)
> > > +bool __shuffle_pick_tail(void)
> > >  {
> > >  	struct batched_bit_entropy *batch;
> > >  	unsigned long entropy;
> > > @@ -213,8 +211,5 @@ void add_to_free_area_random(struct page *page, struct free_area *area,
> > >  	batch->position = position;
> > >  	entropy = batch->entropy_bool;
> > >  
> > > -	if (1ul & (entropy >> position))
> > > -		add_to_free_area(page, area, migratetype);
> > > -	else
> > > -		add_to_free_area_tail(page, area, migratetype);
> > > +	return 1ul & (entropy >> position);
> > >  }
> > > diff --git a/mm/shuffle.h b/mm/shuffle.h
> > > index 777a257a0d2f..0723eb97f22f 100644
> > > --- a/mm/shuffle.h
> > > +++ b/mm/shuffle.h
> > > @@ -3,6 +3,7 @@
> > >  #ifndef _MM_SHUFFLE_H
> > >  #define _MM_SHUFFLE_H
> > >  #include <linux/jump_label.h>
> > > +#include <linux/random.h>
> > >  
> > >  /*
> > >   * SHUFFLE_ENABLE is called from the command line enabling path, or by
> > > @@ -22,6 +23,7 @@ enum mm_shuffle_ctl {
> > >  DECLARE_STATIC_KEY_FALSE(page_alloc_shuffle_key);
> > >  extern void page_alloc_shuffle(enum mm_shuffle_ctl ctl);
> > >  extern void __shuffle_free_memory(pg_data_t *pgdat);
> > > +extern bool __shuffle_pick_tail(void);
> > >  static inline void shuffle_free_memory(pg_data_t *pgdat)
> > >  {
> > >  	if (!static_branch_unlikely(&page_alloc_shuffle_key))
> > > @@ -43,6 +45,11 @@ static inline bool is_shuffle_order(int order)
> > >  		return false;
> > >  	return order >= SHUFFLE_ORDER;
> > >  }
> > > +
> > > +static inline bool shuffle_pick_tail(void)
> > > +{
> > > +	return __shuffle_pick_tail();
> > > +}
> > 
> > I don't see a reason in __shuffle_pick_tail() existing if you call it
> > unconditionally.
> 
> That is for compilation purposes. The function is not used in the
> shuffle_pick_tail below that always returns false.

Wouldn't it be the same if you rename __shuffle_pick_tail() to
shuffle_pick_tail() and put its declaration under the same #ifdef?

-- 
 Kirill A. Shutemov