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

[Alexander Duyck]

On Mon, 2019-09-09 at 18:35 +0300, Kirill A. Shutemov wrote:
> 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?
> 

Yeah I guess I can do that. I'll update that for v10.

Thanks.

- Alex