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

Alexander Duyck
2019-09-09T15:37:56Z

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