Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Alexey Dobriyan]

> > +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...

It is better to keep all sysctls in one preallocated structure
for memory reasons:

	header = kzalloc(sizeof(struct ctl_table_header) +
                         sizeof(struct ctl_node)*nr_entries, GFP_KERNEL_ACCOUNT);

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Luis Chamberlain]

On Tue, Mar 21, 2023 at 05:37:10PM +0300, Alexey Dobriyan 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,
> > > +	},
> > > +	{}
> > > +};
> > > +
> > > +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...
> 
> It is better to keep all sysctls in one preallocated structure
> for memory reasons:
> 
> 	header = kzalloc(sizeof(struct ctl_table_header) +
>                          sizeof(struct ctl_node)*nr_entries, GFP_KERNEL_ACCOUNT);

For memory reasons we are actually phasing out the older APIs which
required an empty array at the end, which we then kmalloc for, and in
the future will just use ARRAY_SIZE(). In the end that will save us
an entry per each sysctl registered. The rationate for this commit log
sucks. It should be fixed to take into consideration other moves as can
be seen in older git log kernel/sysclt.c moves.

  Luis

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Luis Chamberlain]

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

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Christian Brauner]

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.

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Matthew Wilcox]

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.

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Luis Chamberlain]

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

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Matthew Wilcox]

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.

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Luis Chamberlain]

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

Re: [PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Christian Brauner]

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...

[PATCH] fs/drop_caches: move drop_caches sysctls into its own file

[Yangtao Li]

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);
+	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",
-- 
2.35.1