Re: [PATCH v3 6/7] mm/swap: Cache maximum swapfile size when init swap

[Peter Xu <peterx@redhat.com>]

On Wed, Aug 10, 2022 at 02:33:02PM +0800, Huang, Ying wrote:
> 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.

Sure.

-- 
Peter Xu