Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Huang, Ying" <ying.huang@intel.com>
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Minchan Kim <minchan@kernel.org>,
        David Hildenbrand <david@redhat.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andi Kleen <andi.kleen@intel.com>,
        "Kirill A . Shutemov" <kirill@shutemov.name>
Subject: Re: [PATCH v3 6/7] mm/swap: Cache maximum swapfile size when init swap
References: <20220809220100.20033-1-peterx@redhat.com>
        <20220809220100.20033-7-peterx@redhat.com>
Date:   Wed, 10 Aug 2022 14:33:02 +0800
In-Reply-To: <20220809220100.20033-7-peterx@redhat.com> (Peter Xu's message of
        "Tue, 9 Aug 2022 18:00:59 -0400")
Message-ID: <87y1vwehgh.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii
Precedence: bulk

Peter Xu <peterx@redhat.com> writes:

> We used to have swapfile_maximum_size() fetching a maximum value of
> swapfile size per-arch.
>
> As the caller of max_swapfile_size() grows, this patch introduce a variable
> "swapfile_maximum_size" and cache the value of old max_swapfile_size(), so
> that we don't need to calculate the value every time.
>
> Caching the value in swapfile_init() is safe because when reaching the
> phase we should have initialized all the relevant information.  Here the
> major arch to look after is x86, which defines the max size based on L1TF
> mitigation.
>
> Here both X86_BUG_L1TF or l1tf_mitigation should have been setup properly
> when reaching swapfile_init(). As a reference, the code path looks like
> this for x86:
>
> - start_kernel
>   - setup_arch
>     - early_cpu_init
>       - early_identify_cpu --> setup X86_BUG_L1TF
>   - parse_early_param
>     - l1tf_cmdline --> set l1tf_mitigation
>   - check_bugs
>     - l1tf_select_mitigation --> set l1tf_mitigation
>   - arch_call_rest_init
>     - rest_init
>       - kernel_init
>         - kernel_init_freeable
>           - do_basic_setup
>             - do_initcalls --> calls swapfile_init() (initcall level 4)
>
> The swapfile size only depends on swp pte format on non-x86 archs, so
> caching it is safe too.
>
> Since at it, rename max_swapfile_size() to arch_max_swapfile_size() because
> arch can define its own function, so it's more straightforward to have
> "arch_" as its prefix.  At the meantime, keep the swapfile_maximum_size()
> function to fetch the value from the cache initialized in swapfile_init().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/mm/init.c |  2 +-
>  mm/swapfile.c      | 10 +++++++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index 82a042c03824..9121bc1b9453 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -1054,7 +1054,7 @@ void update_cache_mode_entry(unsigned entry, enum page_cache_mode cache)
>  }
>  
>  #ifdef CONFIG_SWAP
> -unsigned long max_swapfile_size(void)
> +unsigned long arch_max_swapfile_size(void)
>  {
>  	unsigned long pages;
>  
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 1fdccd2f1422..794fa37bd0c3 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -63,6 +63,7 @@ EXPORT_SYMBOL_GPL(nr_swap_pages);
>  /* protected with swap_lock. reading in vm_swap_full() doesn't need lock */
>  long total_swap_pages;
>  static int least_priority = -1;
> +static unsigned long swapfile_maximum_size;
>  
>  static const char Bad_file[] = "Bad swap file entry ";
>  static const char Unused_file[] = "Unused swap file entry ";
> @@ -2816,11 +2817,16 @@ unsigned long generic_max_swapfile_size(void)
>  }
>  
>  /* Can be overridden by an architecture for additional checks. */
> -__weak unsigned long max_swapfile_size(void)
> +__weak unsigned long arch_max_swapfile_size(void)
>  {
>  	return generic_max_swapfile_size();
>  }
>  
> +unsigned long max_swapfile_size(void)
> +{
> +	return swapfile_maximum_size;
> +}
> +

It appears unnecessary to hide a variable with a function.  Why not just
use the variable directly.

Best Regards,
Huang, Ying

>  static unsigned long read_swap_header(struct swap_info_struct *p,
>  					union swap_header *swap_header,
>  					struct inode *inode)
> @@ -3677,6 +3683,8 @@ static int __init swapfile_init(void)
>  	for_each_node(nid)
>  		plist_head_init(&swap_avail_heads[nid]);
>  
> +	swapfile_maximum_size = arch_max_swapfile_size();
> +
>  	return 0;
>  }
>  subsys_initcall(swapfile_init);