Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration
On Mon, Jun 22, 2020 at 12:33 AM David Hildenbrand wrote: > > On 20.06.20 03:49, Dan Williams wrote: > > On Fri, Jun 19, 2020 at 5:59 AM David Hildenbrand wrote: > >> > >> Commit e900a918b098 ("mm: shuffle initial free memory to improve > >> memory-side-cache utilization") promised "autodetection of a > >> memory-side-cache (to be added in a follow-on patch)" over a year ago. > >> > >> The original series included patches [1], however, they were dropped > >> during review [2] to be followed-up later. > >> > >> Due to lack of platforms that publish an HMAT, autodetection is currently > >> not implemented. However, manual activation is actively used [3]. Let's > >> simplify for now and re-add when really (ever?) needed. > >> > >> [1] > >> https://lkml.kernel.org/r/154510700291.1941238.817190985966612531.st...@dwillia2-desk3.amr.corp.intel.com > >> [2] > >> https://lkml.kernel.org/r/154690326478.676627.103843791978176914.st...@dwillia2-desk3.amr.corp.intel.com > >> [3] > >> https://lkml.kernel.org/r/CAPcyv4irwGUU2x+c6b4L=kbb1dnasnkaazd6ospyjl9kfsn...@mail.gmail.com > >> > >> Cc: Andrew Morton > >> Cc: Johannes Weiner > >> Cc: Michal Hocko > >> Cc: Minchan Kim > >> Cc: Huang Ying > >> Cc: Wei Yang > >> Cc: Mel Gorman > >> Cc: Dan Williams > >> Signed-off-by: David Hildenbrand > >> --- > >> mm/shuffle.c | 28 ++-- > >> mm/shuffle.h | 17 - > >> 2 files changed, 2 insertions(+), 43 deletions(-) > >> > >> diff --git a/mm/shuffle.c b/mm/shuffle.c > >> index dd13ab851b3ee..9b5cd4b004b0f 100644 > >> --- a/mm/shuffle.c > >> +++ b/mm/shuffle.c > >> @@ -10,33 +10,11 @@ > >> #include "shuffle.h" > >> > >> DEFINE_STATIC_KEY_FALSE(page_alloc_shuffle_key); > >> -static unsigned long shuffle_state __ro_after_init; > >> - > >> -/* > >> - * Depending on the architecture, module parameter parsing may run > >> - * before, or after the cache detection. SHUFFLE_FORCE_DISABLE prevents, > >> - * or reverts the enabling of the shuffle implementation. SHUFFLE_ENABLE > >> - * attempts to turn on the implementation, but aborts if it finds > >> - * SHUFFLE_FORCE_DISABLE already set. > >> - */ > >> -__meminit void page_alloc_shuffle(enum mm_shuffle_ctl ctl) > >> -{ > >> - if (ctl == SHUFFLE_FORCE_DISABLE) > >> - set_bit(SHUFFLE_FORCE_DISABLE, _state); > >> - > >> - if (test_bit(SHUFFLE_FORCE_DISABLE, _state)) { > >> - if (test_and_clear_bit(SHUFFLE_ENABLE, _state)) > >> - static_branch_disable(_alloc_shuffle_key); > >> - } else if (ctl == SHUFFLE_ENABLE > >> - && !test_and_set_bit(SHUFFLE_ENABLE, > >> _state)) > >> - static_branch_enable(_alloc_shuffle_key); > >> -} > >> > >> static bool shuffle_param; > >> static int shuffle_show(char *buffer, const struct kernel_param *kp) > >> { > >> - return sprintf(buffer, "%c\n", test_bit(SHUFFLE_ENABLE, > >> _state) > >> - ? 'Y' : 'N'); > >> + return sprintf(buffer, "%c\n", shuffle_param ? 'Y' : 'N'); > >> } > >> > >> static __meminit int shuffle_store(const char *val, > >> @@ -47,9 +25,7 @@ static __meminit int shuffle_store(const char *val, > >> if (rc < 0) > >> return rc; > >> if (shuffle_param) > >> - page_alloc_shuffle(SHUFFLE_ENABLE); > >> - else > >> - page_alloc_shuffle(SHUFFLE_FORCE_DISABLE); > >> + static_branch_enable(_alloc_shuffle_key); > >> return 0; > >> } > > > > Let's do proper input validation here and require 1 / 'true' to enable > > shuffling and not also allow 0 to be an 'enable' value. > > I don't think that's currently done? > > param_set_bool(val, kp) will only default val==NULL to 'true'. Passing 0 > will properly be handled by strtobool(). Or am I missing something? > No, I misread the patch and thought the conditional was being removed. All good now.
Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration
On Fri, Jun 19, 2020 at 02:59:22PM +0200, David Hildenbrand wrote: >Commit e900a918b098 ("mm: shuffle initial free memory to improve >memory-side-cache utilization") promised "autodetection of a >memory-side-cache (to be added in a follow-on patch)" over a year ago. > >The original series included patches [1], however, they were dropped >during review [2] to be followed-up later. > >Due to lack of platforms that publish an HMAT, autodetection is currently >not implemented. However, manual activation is actively used [3]. Let's >simplify for now and re-add when really (ever?) needed. > >[1] >https://lkml.kernel.org/r/154510700291.1941238.817190985966612531.st...@dwillia2-desk3.amr.corp.intel.com >[2] >https://lkml.kernel.org/r/154690326478.676627.103843791978176914.st...@dwillia2-desk3.amr.corp.intel.com >[3] >https://lkml.kernel.org/r/CAPcyv4irwGUU2x+c6b4L=kbb1dnasnkaazd6ospyjl9kfsn...@mail.gmail.com > >Cc: Andrew Morton >Cc: Johannes Weiner >Cc: Michal Hocko >Cc: Minchan Kim >Cc: Huang Ying >Cc: Wei Yang >Cc: Mel Gorman >Cc: Dan Williams >Signed-off-by: David Hildenbrand Reviewed-by: Wei Yang -- Wei Yang Help you, Help me
Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration
On Fri 19-06-20 14:59:22, David Hildenbrand wrote: > Commit e900a918b098 ("mm: shuffle initial free memory to improve > memory-side-cache utilization") promised "autodetection of a > memory-side-cache (to be added in a follow-on patch)" over a year ago. > > The original series included patches [1], however, they were dropped > during review [2] to be followed-up later. > > Due to lack of platforms that publish an HMAT, autodetection is currently > not implemented. However, manual activation is actively used [3]. Let's > simplify for now and re-add when really (ever?) needed. > > [1] > https://lkml.kernel.org/r/154510700291.1941238.817190985966612531.st...@dwillia2-desk3.amr.corp.intel.com > [2] > https://lkml.kernel.org/r/154690326478.676627.103843791978176914.st...@dwillia2-desk3.amr.corp.intel.com > [3] > https://lkml.kernel.org/r/CAPcyv4irwGUU2x+c6b4L=kbb1dnasnkaazd6ospyjl9kfsn...@mail.gmail.com > > Cc: Andrew Morton > Cc: Johannes Weiner > Cc: Michal Hocko > Cc: Minchan Kim > Cc: Huang Ying > Cc: Wei Yang > Cc: Mel Gorman > Cc: Dan Williams > Signed-off-by: David Hildenbrand Acked-by: Michal Hocko > --- > mm/shuffle.c | 28 ++-- > mm/shuffle.h | 17 - > 2 files changed, 2 insertions(+), 43 deletions(-) > > diff --git a/mm/shuffle.c b/mm/shuffle.c > index dd13ab851b3ee..9b5cd4b004b0f 100644 > --- a/mm/shuffle.c > +++ b/mm/shuffle.c > @@ -10,33 +10,11 @@ > #include "shuffle.h" > > DEFINE_STATIC_KEY_FALSE(page_alloc_shuffle_key); > -static unsigned long shuffle_state __ro_after_init; > - > -/* > - * Depending on the architecture, module parameter parsing may run > - * before, or after the cache detection. SHUFFLE_FORCE_DISABLE prevents, > - * or reverts the enabling of the shuffle implementation. SHUFFLE_ENABLE > - * attempts to turn on the implementation, but aborts if it finds > - * SHUFFLE_FORCE_DISABLE already set. > - */ > -__meminit void page_alloc_shuffle(enum mm_shuffle_ctl ctl) > -{ > - if (ctl == SHUFFLE_FORCE_DISABLE) > - set_bit(SHUFFLE_FORCE_DISABLE, _state); > - > - if (test_bit(SHUFFLE_FORCE_DISABLE, _state)) { > - if (test_and_clear_bit(SHUFFLE_ENABLE, _state)) > - static_branch_disable(_alloc_shuffle_key); > - } else if (ctl == SHUFFLE_ENABLE > - && !test_and_set_bit(SHUFFLE_ENABLE, _state)) > - static_branch_enable(_alloc_shuffle_key); > -} > > static bool shuffle_param; > static int shuffle_show(char *buffer, const struct kernel_param *kp) > { > - return sprintf(buffer, "%c\n", test_bit(SHUFFLE_ENABLE, _state) > - ? 'Y' : 'N'); > + return sprintf(buffer, "%c\n", shuffle_param ? 'Y' : 'N'); > } > > static __meminit int shuffle_store(const char *val, > @@ -47,9 +25,7 @@ static __meminit int shuffle_store(const char *val, > if (rc < 0) > return rc; > if (shuffle_param) > - page_alloc_shuffle(SHUFFLE_ENABLE); > - else > - page_alloc_shuffle(SHUFFLE_FORCE_DISABLE); > + static_branch_enable(_alloc_shuffle_key); > return 0; > } > module_param_call(shuffle, shuffle_store, shuffle_show, _param, > 0400); > diff --git a/mm/shuffle.h b/mm/shuffle.h > index 4d79f03b6658f..71b784f0b7c3e 100644 > --- a/mm/shuffle.h > +++ b/mm/shuffle.h > @@ -4,23 +4,10 @@ > #define _MM_SHUFFLE_H > #include > > -/* > - * SHUFFLE_ENABLE is called from the command line enabling path, or by > - * platform-firmware enabling that indicates the presence of a > - * direct-mapped memory-side-cache. SHUFFLE_FORCE_DISABLE is called from > - * the command line path and overrides any previous or future > - * SHUFFLE_ENABLE. > - */ > -enum mm_shuffle_ctl { > - SHUFFLE_ENABLE, > - SHUFFLE_FORCE_DISABLE, > -}; > - > #define SHUFFLE_ORDER (MAX_ORDER-1) > > #ifdef CONFIG_SHUFFLE_PAGE_ALLOCATOR > 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) > @@ -58,10 +45,6 @@ static inline void shuffle_zone(struct zone *z) > { > } > > -static inline void page_alloc_shuffle(enum mm_shuffle_ctl ctl) > -{ > -} > - > static inline bool is_shuffle_order(int order) > { > return false; > -- > 2.26.2 -- Michal Hocko SUSE Labs
Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration
On Mon, Jun 22, 2020 at 09:33:28AM +0200, David Hildenbrand wrote: >On 20.06.20 03:49, Dan Williams wrote: >> On Fri, Jun 19, 2020 at 5:59 AM David Hildenbrand wrote: >>> >>> Commit e900a918b098 ("mm: shuffle initial free memory to improve >>> memory-side-cache utilization") promised "autodetection of a >>> memory-side-cache (to be added in a follow-on patch)" over a year ago. >>> >>> The original series included patches [1], however, they were dropped >>> during review [2] to be followed-up later. >>> >>> Due to lack of platforms that publish an HMAT, autodetection is currently >>> not implemented. However, manual activation is actively used [3]. Let's >>> simplify for now and re-add when really (ever?) needed. >>> >>> [1] >>> https://lkml.kernel.org/r/154510700291.1941238.817190985966612531.st...@dwillia2-desk3.amr.corp.intel.com >>> [2] >>> https://lkml.kernel.org/r/154690326478.676627.103843791978176914.st...@dwillia2-desk3.amr.corp.intel.com >>> [3] >>> https://lkml.kernel.org/r/CAPcyv4irwGUU2x+c6b4L=kbb1dnasnkaazd6ospyjl9kfsn...@mail.gmail.com >>> >>> Cc: Andrew Morton >>> Cc: Johannes Weiner >>> Cc: Michal Hocko >>> Cc: Minchan Kim >>> Cc: Huang Ying >>> Cc: Wei Yang >>> Cc: Mel Gorman >>> Cc: Dan Williams >>> Signed-off-by: David Hildenbrand >>> --- >>> mm/shuffle.c | 28 ++-- >>> mm/shuffle.h | 17 - >>> 2 files changed, 2 insertions(+), 43 deletions(-) >>> >>> diff --git a/mm/shuffle.c b/mm/shuffle.c >>> index dd13ab851b3ee..9b5cd4b004b0f 100644 >>> --- a/mm/shuffle.c >>> +++ b/mm/shuffle.c >>> @@ -10,33 +10,11 @@ >>> #include "shuffle.h" >>> >>> DEFINE_STATIC_KEY_FALSE(page_alloc_shuffle_key); >>> -static unsigned long shuffle_state __ro_after_init; >>> - >>> -/* >>> - * Depending on the architecture, module parameter parsing may run >>> - * before, or after the cache detection. SHUFFLE_FORCE_DISABLE prevents, >>> - * or reverts the enabling of the shuffle implementation. SHUFFLE_ENABLE >>> - * attempts to turn on the implementation, but aborts if it finds >>> - * SHUFFLE_FORCE_DISABLE already set. >>> - */ >>> -__meminit void page_alloc_shuffle(enum mm_shuffle_ctl ctl) >>> -{ >>> - if (ctl == SHUFFLE_FORCE_DISABLE) >>> - set_bit(SHUFFLE_FORCE_DISABLE, _state); >>> - >>> - if (test_bit(SHUFFLE_FORCE_DISABLE, _state)) { >>> - if (test_and_clear_bit(SHUFFLE_ENABLE, _state)) >>> - static_branch_disable(_alloc_shuffle_key); >>> - } else if (ctl == SHUFFLE_ENABLE >>> - && !test_and_set_bit(SHUFFLE_ENABLE, >>> _state)) >>> - static_branch_enable(_alloc_shuffle_key); >>> -} >>> >>> static bool shuffle_param; >>> static int shuffle_show(char *buffer, const struct kernel_param *kp) >>> { >>> - return sprintf(buffer, "%c\n", test_bit(SHUFFLE_ENABLE, >>> _state) >>> - ? 'Y' : 'N'); >>> + return sprintf(buffer, "%c\n", shuffle_param ? 'Y' : 'N'); >>> } >>> >>> static __meminit int shuffle_store(const char *val, >>> @@ -47,9 +25,7 @@ static __meminit int shuffle_store(const char *val, >>> if (rc < 0) >>> return rc; >>> if (shuffle_param) >>> - page_alloc_shuffle(SHUFFLE_ENABLE); >>> - else >>> - page_alloc_shuffle(SHUFFLE_FORCE_DISABLE); >>> + static_branch_enable(_alloc_shuffle_key); >>> return 0; >>> } >> >> Let's do proper input validation here and require 1 / 'true' to enable >> shuffling and not also allow 0 to be an 'enable' value. > >I don't think that's currently done? > >param_set_bool(val, kp) will only default val==NULL to 'true'. Passing 0 >will properly be handled by strtobool(). Or am I missing something? > Agree with this statement. >Thanks! > >-- >Thanks, > >David / dhildenb -- Wei Yang Help you, Help me
Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration
On 20.06.20 03:49, Dan Williams wrote: > On Fri, Jun 19, 2020 at 5:59 AM David Hildenbrand wrote: >> >> Commit e900a918b098 ("mm: shuffle initial free memory to improve >> memory-side-cache utilization") promised "autodetection of a >> memory-side-cache (to be added in a follow-on patch)" over a year ago. >> >> The original series included patches [1], however, they were dropped >> during review [2] to be followed-up later. >> >> Due to lack of platforms that publish an HMAT, autodetection is currently >> not implemented. However, manual activation is actively used [3]. Let's >> simplify for now and re-add when really (ever?) needed. >> >> [1] >> https://lkml.kernel.org/r/154510700291.1941238.817190985966612531.st...@dwillia2-desk3.amr.corp.intel.com >> [2] >> https://lkml.kernel.org/r/154690326478.676627.103843791978176914.st...@dwillia2-desk3.amr.corp.intel.com >> [3] >> https://lkml.kernel.org/r/CAPcyv4irwGUU2x+c6b4L=kbb1dnasnkaazd6ospyjl9kfsn...@mail.gmail.com >> >> Cc: Andrew Morton >> Cc: Johannes Weiner >> Cc: Michal Hocko >> Cc: Minchan Kim >> Cc: Huang Ying >> Cc: Wei Yang >> Cc: Mel Gorman >> Cc: Dan Williams >> Signed-off-by: David Hildenbrand >> --- >> mm/shuffle.c | 28 ++-- >> mm/shuffle.h | 17 - >> 2 files changed, 2 insertions(+), 43 deletions(-) >> >> diff --git a/mm/shuffle.c b/mm/shuffle.c >> index dd13ab851b3ee..9b5cd4b004b0f 100644 >> --- a/mm/shuffle.c >> +++ b/mm/shuffle.c >> @@ -10,33 +10,11 @@ >> #include "shuffle.h" >> >> DEFINE_STATIC_KEY_FALSE(page_alloc_shuffle_key); >> -static unsigned long shuffle_state __ro_after_init; >> - >> -/* >> - * Depending on the architecture, module parameter parsing may run >> - * before, or after the cache detection. SHUFFLE_FORCE_DISABLE prevents, >> - * or reverts the enabling of the shuffle implementation. SHUFFLE_ENABLE >> - * attempts to turn on the implementation, but aborts if it finds >> - * SHUFFLE_FORCE_DISABLE already set. >> - */ >> -__meminit void page_alloc_shuffle(enum mm_shuffle_ctl ctl) >> -{ >> - if (ctl == SHUFFLE_FORCE_DISABLE) >> - set_bit(SHUFFLE_FORCE_DISABLE, _state); >> - >> - if (test_bit(SHUFFLE_FORCE_DISABLE, _state)) { >> - if (test_and_clear_bit(SHUFFLE_ENABLE, _state)) >> - static_branch_disable(_alloc_shuffle_key); >> - } else if (ctl == SHUFFLE_ENABLE >> - && !test_and_set_bit(SHUFFLE_ENABLE, _state)) >> - static_branch_enable(_alloc_shuffle_key); >> -} >> >> static bool shuffle_param; >> static int shuffle_show(char *buffer, const struct kernel_param *kp) >> { >> - return sprintf(buffer, "%c\n", test_bit(SHUFFLE_ENABLE, >> _state) >> - ? 'Y' : 'N'); >> + return sprintf(buffer, "%c\n", shuffle_param ? 'Y' : 'N'); >> } >> >> static __meminit int shuffle_store(const char *val, >> @@ -47,9 +25,7 @@ static __meminit int shuffle_store(const char *val, >> if (rc < 0) >> return rc; >> if (shuffle_param) >> - page_alloc_shuffle(SHUFFLE_ENABLE); >> - else >> - page_alloc_shuffle(SHUFFLE_FORCE_DISABLE); >> + static_branch_enable(_alloc_shuffle_key); >> return 0; >> } > > Let's do proper input validation here and require 1 / 'true' to enable > shuffling and not also allow 0 to be an 'enable' value. I don't think that's currently done? param_set_bool(val, kp) will only default val==NULL to 'true'. Passing 0 will properly be handled by strtobool(). Or am I missing something? Thanks! -- Thanks, David / dhildenb
Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration
On Fri, Jun 19, 2020 at 5:59 AM David Hildenbrand wrote: > > Commit e900a918b098 ("mm: shuffle initial free memory to improve > memory-side-cache utilization") promised "autodetection of a > memory-side-cache (to be added in a follow-on patch)" over a year ago. > > The original series included patches [1], however, they were dropped > during review [2] to be followed-up later. > > Due to lack of platforms that publish an HMAT, autodetection is currently > not implemented. However, manual activation is actively used [3]. Let's > simplify for now and re-add when really (ever?) needed. > > [1] > https://lkml.kernel.org/r/154510700291.1941238.817190985966612531.st...@dwillia2-desk3.amr.corp.intel.com > [2] > https://lkml.kernel.org/r/154690326478.676627.103843791978176914.st...@dwillia2-desk3.amr.corp.intel.com > [3] > https://lkml.kernel.org/r/CAPcyv4irwGUU2x+c6b4L=kbb1dnasnkaazd6ospyjl9kfsn...@mail.gmail.com > > Cc: Andrew Morton > Cc: Johannes Weiner > Cc: Michal Hocko > Cc: Minchan Kim > Cc: Huang Ying > Cc: Wei Yang > Cc: Mel Gorman > Cc: Dan Williams > Signed-off-by: David Hildenbrand > --- > mm/shuffle.c | 28 ++-- > mm/shuffle.h | 17 - > 2 files changed, 2 insertions(+), 43 deletions(-) > > diff --git a/mm/shuffle.c b/mm/shuffle.c > index dd13ab851b3ee..9b5cd4b004b0f 100644 > --- a/mm/shuffle.c > +++ b/mm/shuffle.c > @@ -10,33 +10,11 @@ > #include "shuffle.h" > > DEFINE_STATIC_KEY_FALSE(page_alloc_shuffle_key); > -static unsigned long shuffle_state __ro_after_init; > - > -/* > - * Depending on the architecture, module parameter parsing may run > - * before, or after the cache detection. SHUFFLE_FORCE_DISABLE prevents, > - * or reverts the enabling of the shuffle implementation. SHUFFLE_ENABLE > - * attempts to turn on the implementation, but aborts if it finds > - * SHUFFLE_FORCE_DISABLE already set. > - */ > -__meminit void page_alloc_shuffle(enum mm_shuffle_ctl ctl) > -{ > - if (ctl == SHUFFLE_FORCE_DISABLE) > - set_bit(SHUFFLE_FORCE_DISABLE, _state); > - > - if (test_bit(SHUFFLE_FORCE_DISABLE, _state)) { > - if (test_and_clear_bit(SHUFFLE_ENABLE, _state)) > - static_branch_disable(_alloc_shuffle_key); > - } else if (ctl == SHUFFLE_ENABLE > - && !test_and_set_bit(SHUFFLE_ENABLE, _state)) > - static_branch_enable(_alloc_shuffle_key); > -} > > static bool shuffle_param; > static int shuffle_show(char *buffer, const struct kernel_param *kp) > { > - return sprintf(buffer, "%c\n", test_bit(SHUFFLE_ENABLE, > _state) > - ? 'Y' : 'N'); > + return sprintf(buffer, "%c\n", shuffle_param ? 'Y' : 'N'); > } > > static __meminit int shuffle_store(const char *val, > @@ -47,9 +25,7 @@ static __meminit int shuffle_store(const char *val, > if (rc < 0) > return rc; > if (shuffle_param) > - page_alloc_shuffle(SHUFFLE_ENABLE); > - else > - page_alloc_shuffle(SHUFFLE_FORCE_DISABLE); > + static_branch_enable(_alloc_shuffle_key); > return 0; > } Let's do proper input validation here and require 1 / 'true' to enable shuffling and not also allow 0 to be an 'enable' value. Other than that looks like the right move to me until end users or distros start asking for the kernel to do this by default, I'm not aware of any of those requests to date. People seem fine to set the boot option. After the above fixups you can add: Acked-by: Dan Williams