linux-kernel - Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Jun 2020 18:49:59 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...e.com>,
        Minchan Kim <minchan@...nel.org>,
        Huang Ying <ying.huang@...el.com>,
        Wei Yang <richard.weiyang@...il.com>,
        Mel Gorman <mgorman@...hsingularity.net>
Subject: Re: [PATCH v2 3/3] mm/shuffle: remove dynamic reconfiguration

On Fri, Jun 19, 2020 at 5:59 AM David Hildenbrand <david@...hat.com> 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.stgit@dwillia2-desk3.amr.corp.intel.com
> [2] https://lkml.kernel.org/r/154690326478.676627.103843791978176914.stgit@dwillia2-desk3.amr.corp.intel.com
> [3] https://lkml.kernel.org/r/CAPcyv4irwGUU2x+c6b4L=KbB1dnasNKaaZd6oSpYjL9kfsnROQ@mail.gmail.com
>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Minchan Kim <minchan@...nel.org>
> Cc: Huang Ying <ying.huang@...el.com>
> Cc: Wei Yang <richard.weiyang@...il.com>
> Cc: Mel Gorman <mgorman@...hsingularity.net>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>  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, &shuffle_state);
> -
> -       if (test_bit(SHUFFLE_FORCE_DISABLE, &shuffle_state)) {
> -               if (test_and_clear_bit(SHUFFLE_ENABLE, &shuffle_state))
> -                       static_branch_disable(&page_alloc_shuffle_key);
> -       } else if (ctl == SHUFFLE_ENABLE
> -                       && !test_and_set_bit(SHUFFLE_ENABLE, &shuffle_state))
> -               static_branch_enable(&page_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, &shuffle_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(&page_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 <dan.j.williams@...el.com>