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