[PATCH] shmem: make shmem default size a define value

[PATCH] shmem: make shmem default size a define value

[Gary B. Genett]

The default size of the shmem filesystem is currently set to 50% of
memory using a magic token.  This change makes it a define value, so
that it is clearly denoted as a global value, and to make it easier to
track down and identify.

No behavior is changed, and no additional processing is created.

Signed-off-by: Gary B. Genett <me@garybgenett.net>
---
 mm/shmem.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index cd570cc79c76..bc758cfb4cb6 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -95,6 +95,9 @@ static struct vfsmount *shm_mnt;
 /* Symlink up to this size is kmalloc'ed instead of using a swappable page */
 #define SHORT_SYMLINK_LEN 128

+/* Default size of shmem filesystem */
+#define SHMEM_SIZE_DEFAULT (totalram_pages() / 2)
+
 /*
  * shmem_fallocate communicates with shmem_fault or shmem_writepage via
  * inode->i_private (with i_mutex making sure that it has only one user at
@@ -125,14 +128,14 @@ struct shmem_options {
 #ifdef CONFIG_TMPFS
 static unsigned long shmem_default_max_blocks(void)
 {
-	return totalram_pages() / 2;
+	return SHMEM_SIZE_DEFAULT;
 }

 static unsigned long shmem_default_max_inodes(void)
 {
 	unsigned long nr_pages = totalram_pages();

-	return min(nr_pages - totalhigh_pages(), nr_pages / 2);
+	return min(nr_pages - totalhigh_pages(), SHMEM_SIZE_DEFAULT);
 }
 #endif

--
2.15.2

Re: [PATCH] shmem: make shmem default size a define value

[Qian Cai]

On Wed, 2019-10-09 at 11:46 -0700, Gary B. Genett wrote:
> The default size of the shmem filesystem is currently set to 50% of
> memory using a magic token.  This change makes it a define value, so
> that it is clearly denoted as a global value, and to make it easier to
> track down and identify.
> 
> No behavior is changed, and no additional processing is created.
> 
> Signed-off-by: Gary B. Genett <me@garybgenett.net>
> ---
>  mm/shmem.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index cd570cc79c76..bc758cfb4cb6 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -95,6 +95,9 @@ static struct vfsmount *shm_mnt;
>  /* Symlink up to this size is kmalloc'ed instead of using a swappable page */
>  #define SHORT_SYMLINK_LEN 128
> 
> +/* Default size of shmem filesystem */
> +#define SHMEM_SIZE_DEFAULT (totalram_pages() / 2)
> +
>  /*
>   * shmem_fallocate communicates with shmem_fault or shmem_writepage via
>   * inode->i_private (with i_mutex making sure that it has only one user at
> @@ -125,14 +128,14 @@ struct shmem_options {
>  #ifdef CONFIG_TMPFS
>  static unsigned long shmem_default_max_blocks(void)
>  {
> -	return totalram_pages() / 2;
> +	return SHMEM_SIZE_DEFAULT;
>  }
> 
>  static unsigned long shmem_default_max_inodes(void)
>  {
>  	unsigned long nr_pages = totalram_pages();

This is now unused.

> 
> -	return min(nr_pages - totalhigh_pages(), nr_pages / 2);
> +	return min(nr_pages - totalhigh_pages(), SHMEM_SIZE_DEFAULT);
>  }
>  #endif
> 
> --
> 2.15.2

Re: [PATCH] shmem: make shmem default size a define value

[-Gary-]

> > @@ -125,14 +128,14 @@ struct shmem_options {
> >  #ifdef CONFIG_TMPFS
> >  static unsigned long shmem_default_max_blocks(void)
> >  {
> > -	return totalram_pages() / 2;
> > +	return SHMEM_SIZE_DEFAULT;
> >  }
> > 
> >  static unsigned long shmem_default_max_inodes(void)
> >  {
> >  	unsigned long nr_pages = totalram_pages();
> 
> This is now unused.

Mind telling me what I missed, Qian?

I pulled from:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

Latest commit is:
2019-10-08 15:36:04 -0700 e3280b54afed870d531571212f1fc375df39b7d2 Merge tag 'led-fixes-for-5.4-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds

My patch is based on that, where the above code still exists, and nr_pages is still referenced below.  Apparently, it is making totalram_pages atomic, according to the commit that changed it:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/mm/shmem.c?id=ca79b0c211af63fa3276f0e3fd7dd9ada2439839

Should I be pulling from somewhere else, or also re-hacking nr_pages somehow?

-- Gary

> > -	return min(nr_pages - totalhigh_pages(), nr_pages / 2);
> > +	return min(nr_pages - totalhigh_pages(), SHMEM_SIZE_DEFAULT);
> >  }
> >  #endif

Re: [PATCH] shmem: make shmem default size a define value

[Qian Cai]

On Wed, 2019-10-09 at 13:17 -0700, -Gary- wrote:
> > > @@ -125,14 +128,14 @@ struct shmem_options {
> > >  #ifdef CONFIG_TMPFS
> > >  static unsigned long shmem_default_max_blocks(void)
> > >  {
> > > -	return totalram_pages() / 2;
> > > +	return SHMEM_SIZE_DEFAULT;
> > >  }
> > > 
> > >  static unsigned long shmem_default_max_inodes(void)
> > >  {
> > >  	unsigned long nr_pages = totalram_pages();
> > 
> > This is now unused.
> 
> Mind telling me what I missed, Qian?
> 
> I pulled from:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 
> Latest commit is:
> 2019-10-08 15:36:04 -0700 e3280b54afed870d531571212f1fc375df39b7d2 Merge tag 'led-fixes-for-5.4-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds
> 
> My patch is based on that, where the above code still exists, and nr_pages is still referenced below.  Apparently, it is making totalram_pages atomic, according to the commit that changed it:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/mm/shmem.c?id=ca79b0c211af63fa3276f0e3fd7dd9ada2439839
> 
> Should I be pulling from somewhere else, or also re-hacking nr_pages somehow?

Ah, you are right that nr_pages is still referenced, but then there could a
potentially out of sync? Would that be a problem? Is the compiler just smart
enough to call totalram_pages() only once?

CPU0:                                       CPU1:
unsigned long nr_pages = totalram_pages();
                                            totalram_pages_inc()
min(nr_pages - totalhigh_pages(), SHMEM_SIZE_DEFAULT);

> 
> -- Gary
> 
> > > -	return min(nr_pages - totalhigh_pages(), nr_pages / 2);
> > > +	return min(nr_pages - totalhigh_pages(), SHMEM_SIZE_DEFAULT);
> > >  }
> > >  #endif

Re: [PATCH] shmem: make shmem default size a define value

[-Gary-]

> > Should I be pulling from somewhere else, or also re-hacking nr_pages somehow?
>
> Ah, you are right that nr_pages is still referenced, but then there could a
> potentially out of sync? Would that be a problem? Is the compiler just smart
> enough to call totalram_pages() only once?
>
> CPU0:                                       CPU1:
> unsigned long nr_pages = totalram_pages();
>                                             totalram_pages_inc()
> min(nr_pages - totalhigh_pages(), SHMEM_SIZE_DEFAULT);

Ah, I see your point, Qian.  Even with my fledgling skills, I should have thought of that.  The whole point of the commit I referenced was to make the operation atomic so the values would be identical.  I failed there.

The only option then, would be to do something like define SHMEM_SIZE_DEFAULT as 50, and call it as *(SHMEM_SIZE_DEFAULT/100), which feels pretty contrived and silly.

Good news is that I finally see how it probably ended up as it is.  While poking around, I've also started to see this type of math throughout the entire kernel, such as the hibernate subsystem, where 2/5 is used.  Since I am not going to take up the task of finding and changing all of them...  best to leave well enough alone.

This has been a very educational experience.  It has been fun to really dig into the MM corner of the kernel, and get to understand things on a deeper level.  A lot going on in there!

You've been so patient and gracious.  Thank you for your time, and walking me through the thinking.  I will apply my efforts in other areas.  <^)

-- Gary