diff mbox series

fs/drop_caches: move drop_caches sysctls into its own file

Message ID 20230321130908.6972-1-frank.li@vivo.com
State New
Headers
Series fs/drop_caches: move drop_caches sysctls into its own file |

Commit Message

李扬韬 March 21, 2023, 1:09 p.m. UTC 
  This moves the fs/drop_caches.c respective sysctls to its own file.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/drop_caches.c   | 25 ++++++++++++++++++++++---
 include/linux/mm.h |  6 ------
 kernel/sysctl.c    |  9 ---------
 3 files changed, 22 insertions(+), 18 deletions(-)

Comments

Christian Brauner March 21, 2023, 2:28 p.m. UTC | #1 
On Tue, Mar 21, 2023 at 09:09:07PM +0800, Yangtao Li wrote:
> This moves the fs/drop_caches.c respective sysctls to its own file.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/drop_caches.c   | 25 ++++++++++++++++++++++---
>  include/linux/mm.h |  6 ------
>  kernel/sysctl.c    |  9 ---------
>  3 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/drop_caches.c b/fs/drop_caches.c
> index e619c31b6bd9..3032b83ce6f2 100644
> --- a/fs/drop_caches.c
> +++ b/fs/drop_caches.c
> @@ -12,8 +12,7 @@
>  #include <linux/gfp.h>
>  #include "internal.h"
>  
> -/* A global variable is a bit ugly, but it keeps the code simple */
> -int sysctl_drop_caches;
> +static int sysctl_drop_caches;
>  
>  static void drop_pagecache_sb(struct super_block *sb, void *unused)
>  {
> @@ -47,7 +46,7 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused)
>  	iput(toput_inode);
>  }
>  
> -int drop_caches_sysctl_handler(struct ctl_table *table, int write,
> +static int drop_caches_sysctl_handler(struct ctl_table *table, int write,
>  		void *buffer, size_t *length, loff_t *ppos)
>  {
>  	int ret;
> @@ -75,3 +74,23 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,
>  	}
>  	return 0;
>  }
> +
> +static struct ctl_table drop_caches_table[] = {
> +	{
> +		.procname	= "drop_caches",
> +		.data		= &sysctl_drop_caches,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0200,
> +		.proc_handler	= drop_caches_sysctl_handler,
> +		.extra1		= SYSCTL_ONE,
> +		.extra2		= SYSCTL_FOUR,
> +	},
> +	{}
> +};
> +
> +static int __init drop_cache_init(void)
> +{
> +	register_sysctl_init("vm", drop_caches_table);

Does this belong under mm/ or fs/?
And is it intended to be moved into a completely separate file?
Feels abit wasteful for 20 lines of code...
Luis Chamberlain March 21, 2023, 4:39 p.m. UTC | #2 
On Tue, Mar 21, 2023 at 03:28:36PM +0100, Christian Brauner wrote:
> On Tue, Mar 21, 2023 at 09:09:07PM +0800, Yangtao Li wrote:
> > This moves the fs/drop_caches.c respective sysctls to its own file.
> > 
> > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > ---
> >  fs/drop_caches.c   | 25 ++++++++++++++++++++++---
> >  include/linux/mm.h |  6 ------
> >  kernel/sysctl.c    |  9 ---------
> >  3 files changed, 22 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/drop_caches.c b/fs/drop_caches.c
> > index e619c31b6bd9..3032b83ce6f2 100644
> > --- a/fs/drop_caches.c
> > +++ b/fs/drop_caches.c
> > @@ -12,8 +12,7 @@
> >  #include <linux/gfp.h>
> >  #include "internal.h"
> >  
> > -/* A global variable is a bit ugly, but it keeps the code simple */
> > -int sysctl_drop_caches;
> > +static int sysctl_drop_caches;
> >  
> >  static void drop_pagecache_sb(struct super_block *sb, void *unused)
> >  {
> > @@ -47,7 +46,7 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused)
> >  	iput(toput_inode);
> >  }
> >  
> > -int drop_caches_sysctl_handler(struct ctl_table *table, int write,
> > +static int drop_caches_sysctl_handler(struct ctl_table *table, int write,
> >  		void *buffer, size_t *length, loff_t *ppos)
> >  {
> >  	int ret;
> > @@ -75,3 +74,23 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,
> >  	}
> >  	return 0;
> >  }
> > +
> > +static struct ctl_table drop_caches_table[] = {
> > +	{
> > +		.procname	= "drop_caches",
> > +		.data		= &sysctl_drop_caches,
> > +		.maxlen		= sizeof(int),
> > +		.mode		= 0200,
> > +		.proc_handler	= drop_caches_sysctl_handler,
> > +		.extra1		= SYSCTL_ONE,
> > +		.extra2		= SYSCTL_FOUR,
> > +	},
> > +	{}
> > +};
> > +
> > +static int __init drop_cache_init(void)
> > +{
> > +	register_sysctl_init("vm", drop_caches_table);
> 
> Does this belong under mm/ or fs/?

To not break old userspace it must be kept under "vm" because the
patch author is moving it from the kernel/sysctl.c table which used
the "vm" table.

Moving it to "fs" would be a highly functional change which should
require review from maintainers it would not break existing userspace
expecations.

> And is it intended to be moved into a completely separate file?

What do you mean by this?

> Feels abit wasteful for 20 lines of code...

Not sure what you mean by this either. The commit log sucks, please
review got log kernel/sysclt.c for much better commit logs for the
rationale of moving sysctls out out kernel/sysctl.c to their own
respective places.

  Luis
Matthew Wilcox March 21, 2023, 4:42 p.m. UTC | #3 
On Tue, Mar 21, 2023 at 09:09:07PM +0800, Yangtao Li wrote:
> +static struct ctl_table drop_caches_table[] = {
> +	{
> +		.procname	= "drop_caches",
> +		.data		= &sysctl_drop_caches,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0200,
> +		.proc_handler	= drop_caches_sysctl_handler,
> +		.extra1		= SYSCTL_ONE,
> +		.extra2		= SYSCTL_FOUR,
> +	},
> +	{}
> +};

Could we avoid doing this until we no longer need an entire zero entry
after the last one?  Also, please post scripts/bloat-o-meter results
for before-and-after.
Luis Chamberlain March 21, 2023, 4:56 p.m. UTC | #4 
On Tue, Mar 21, 2023 at 04:42:32PM +0000, Matthew Wilcox wrote:
> On Tue, Mar 21, 2023 at 09:09:07PM +0800, Yangtao Li wrote:
> > +static struct ctl_table drop_caches_table[] = {
> > +	{
> > +		.procname	= "drop_caches",
> > +		.data		= &sysctl_drop_caches,
> > +		.maxlen		= sizeof(int),
> > +		.mode		= 0200,
> > +		.proc_handler	= drop_caches_sysctl_handler,
> > +		.extra1		= SYSCTL_ONE,
> > +		.extra2		= SYSCTL_FOUR,
> > +	},
> > +	{}
> > +};
> 
> Could we avoid doing this until we no longer need an entire zero entry
> after the last one? 

That may be 2-3 kernel release from now. The way to use ARRAY_SIZE()
really is to deprecate the crap APIs that allow messy directory sysctl
structures.

> Also, please post scripts/bloat-o-meter results
> for before-and-after.

It should be one extry ~ ctl_table per move.

  Luis
Matthew Wilcox March 21, 2023, 5:19 p.m. UTC | #5 
On Tue, Mar 21, 2023 at 09:56:03AM -0700, Luis Chamberlain wrote:
> On Tue, Mar 21, 2023 at 04:42:32PM +0000, Matthew Wilcox wrote:
> > On Tue, Mar 21, 2023 at 09:09:07PM +0800, Yangtao Li wrote:
> > > +static struct ctl_table drop_caches_table[] = {
> > > +	{
> > > +		.procname	= "drop_caches",
> > > +		.data		= &sysctl_drop_caches,
> > > +		.maxlen		= sizeof(int),
> > > +		.mode		= 0200,
> > > +		.proc_handler	= drop_caches_sysctl_handler,
> > > +		.extra1		= SYSCTL_ONE,
> > > +		.extra2		= SYSCTL_FOUR,
> > > +	},
> > > +	{}
> > > +};
> > 
> > Could we avoid doing this until we no longer need an entire zero entry
> > after the last one? 
> 
> That may be 2-3 kernel release from now. The way to use ARRAY_SIZE()
> really is to deprecate the crap APIs that allow messy directory sysctl
> structures.

I'm OK with waiting another year to commence this cleanup.  We've lived
with the giant tables for decades already.  Better to get the new API
right than split the tables now, then have to touch all the places
again.
Christian Brauner March 21, 2023, 5:19 p.m. UTC | #6 
On Tue, Mar 21, 2023 at 09:39:41AM -0700, Luis Chamberlain wrote:
> On Tue, Mar 21, 2023 at 03:28:36PM +0100, Christian Brauner wrote:
> > On Tue, Mar 21, 2023 at 09:09:07PM +0800, Yangtao Li wrote:
> > > This moves the fs/drop_caches.c respective sysctls to its own file.
> > > 
> > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > ---
> > >  fs/drop_caches.c   | 25 ++++++++++++++++++++++---
> > >  include/linux/mm.h |  6 ------
> > >  kernel/sysctl.c    |  9 ---------
> > >  3 files changed, 22 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/fs/drop_caches.c b/fs/drop_caches.c
> > > index e619c31b6bd9..3032b83ce6f2 100644
> > > --- a/fs/drop_caches.c
> > > +++ b/fs/drop_caches.c
> > > @@ -12,8 +12,7 @@
> > >  #include <linux/gfp.h>
> > >  #include "internal.h"
> > >  
> > > -/* A global variable is a bit ugly, but it keeps the code simple */
> > > -int sysctl_drop_caches;
> > > +static int sysctl_drop_caches;
> > >  
> > >  static void drop_pagecache_sb(struct super_block *sb, void *unused)
> > >  {
> > > @@ -47,7 +46,7 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused)
> > >  	iput(toput_inode);
> > >  }
> > >  
> > > -int drop_caches_sysctl_handler(struct ctl_table *table, int write,
> > > +static int drop_caches_sysctl_handler(struct ctl_table *table, int write,
> > >  		void *buffer, size_t *length, loff_t *ppos)
> > >  {
> > >  	int ret;
> > > @@ -75,3 +74,23 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,
> > >  	}
> > >  	return 0;
> > >  }
> > > +
> > > +static struct ctl_table drop_caches_table[] = {
> > > +	{
> > > +		.procname	= "drop_caches",
> > > +		.data		= &sysctl_drop_caches,
> > > +		.maxlen		= sizeof(int),
> > > +		.mode		= 0200,
> > > +		.proc_handler	= drop_caches_sysctl_handler,
> > > +		.extra1		= SYSCTL_ONE,
> > > +		.extra2		= SYSCTL_FOUR,
> > > +	},
> > > +	{}
> > > +};
> > > +
> > > +static int __init drop_cache_init(void)
> > > +{
> > > +	register_sysctl_init("vm", drop_caches_table);
> > 
> > Does this belong under mm/ or fs/?
> 
> To not break old userspace it must be kept under "vm" because the
> patch author is moving it from the kernel/sysctl.c table which used
> the "vm" table.
> 
> Moving it to "fs" would be a highly functional change which should
> require review from maintainers it would not break existing userspace
> expecations.

No, I was asking whether this belongs under the fs/ or mm/ directory.
But I misread the patch. I thought at first, that the patch moved the
file from the mm/ to the fs/ directory. But that's obviously not the
case... So ignore me.
Luis Chamberlain March 21, 2023, 6:10 p.m. UTC | #7 
On Tue, Mar 21, 2023 at 05:19:18PM +0000, Matthew Wilcox wrote:
> On Tue, Mar 21, 2023 at 09:56:03AM -0700, Luis Chamberlain wrote:
> > On Tue, Mar 21, 2023 at 04:42:32PM +0000, Matthew Wilcox wrote:
> > > On Tue, Mar 21, 2023 at 09:09:07PM +0800, Yangtao Li wrote:
> > > > +static struct ctl_table drop_caches_table[] = {
> > > > +	{
> > > > +		.procname	= "drop_caches",
> > > > +		.data		= &sysctl_drop_caches,
> > > > +		.maxlen		= sizeof(int),
> > > > +		.mode		= 0200,
> > > > +		.proc_handler	= drop_caches_sysctl_handler,
> > > > +		.extra1		= SYSCTL_ONE,
> > > > +		.extra2		= SYSCTL_FOUR,
> > > > +	},
> > > > +	{}
> > > > +};
> > > 
> > > Could we avoid doing this until we no longer need an entire zero entry
> > > after the last one? 
> > 
> > That may be 2-3 kernel release from now. The way to use ARRAY_SIZE()
> > really is to deprecate the crap APIs that allow messy directory sysctl
> > structures.
> 
> I'm OK with waiting another year to commence this cleanup.  We've lived
> with the giant tables for decades already.  Better to get the new API
> right than split the tables now, then have to touch all the places
> again.

We can do that sure.

  Luis
diff mbox series

Patch

diff --git a/fs/drop_caches.c b/fs/drop_caches.c
index e619c31b6bd9..3032b83ce6f2 100644
--- a/fs/drop_caches.c
+++ b/fs/drop_caches.c
@@ -12,8 +12,7 @@ 
 #include <linux/gfp.h>
 #include "internal.h"
 
-/* A global variable is a bit ugly, but it keeps the code simple */
-int sysctl_drop_caches;
+static int sysctl_drop_caches;
 
 static void drop_pagecache_sb(struct super_block *sb, void *unused)
 {
@@ -47,7 +46,7 @@  static void drop_pagecache_sb(struct super_block *sb, void *unused)
 	iput(toput_inode);
 }
 
-int drop_caches_sysctl_handler(struct ctl_table *table, int write,
+static int drop_caches_sysctl_handler(struct ctl_table *table, int write,
 		void *buffer, size_t *length, loff_t *ppos)
 {
 	int ret;
@@ -75,3 +74,23 @@  int drop_caches_sysctl_handler(struct ctl_table *table, int write,
 	}
 	return 0;
 }
+
+static struct ctl_table drop_caches_table[] = {
+	{
+		.procname	= "drop_caches",
+		.data		= &sysctl_drop_caches,
+		.maxlen		= sizeof(int),
+		.mode		= 0200,
+		.proc_handler	= drop_caches_sysctl_handler,
+		.extra1		= SYSCTL_ONE,
+		.extra2		= SYSCTL_FOUR,
+	},
+	{}
+};
+
+static int __init drop_cache_init(void)
+{
+	register_sysctl_init("vm", drop_caches_table);
+	return 0;
+}
+fs_initcall(drop_cache_init);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ee755bb4e1c1..1a5d9e8a41b5 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3513,12 +3513,6 @@  static inline int in_gate_area(struct mm_struct *mm, unsigned long addr)
 
 extern bool process_shares_mm(struct task_struct *p, struct mm_struct *mm);
 
-#ifdef CONFIG_SYSCTL
-extern int sysctl_drop_caches;
-int drop_caches_sysctl_handler(struct ctl_table *, int, void *, size_t *,
-		loff_t *);
-#endif
-
 void drop_slab(void);
 
 #ifndef CONFIG_MMU
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ce0297acf97c..6cbae0f7d50f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2148,15 +2148,6 @@  static struct ctl_table vm_table[] = {
 		.mode		= 0644,
 		.proc_handler	= lowmem_reserve_ratio_sysctl_handler,
 	},
-	{
-		.procname	= "drop_caches",
-		.data		= &sysctl_drop_caches,
-		.maxlen		= sizeof(int),
-		.mode		= 0200,
-		.proc_handler	= drop_caches_sysctl_handler,
-		.extra1		= SYSCTL_ONE,
-		.extra2		= SYSCTL_FOUR,
-	},
 #ifdef CONFIG_COMPACTION
 	{
 		.procname	= "compact_memory",