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
