[PATCH 10/11] fs/drop_caches: move drop_caches sysctls to its own file

[PATCH 10/11] fs/drop_caches: move drop_caches sysctls to its own file

[tangmeng]

kernel/sysctl.c is a kitchen sink where everyone leaves their dirty
dishes, this makes it very difficult to maintain.

To help with this maintenance let's start by moving sysctls to places
where they actually belong.  The proc sysctl maintainers do not want to
know what sysctl knobs you wish to add for your own piece of code, we
just care about the core logic.

All filesystem syctls now get reviewed by fs folks. This commit
follows the commit of fs, move the drop_caches sysctls to its own file,
fs/drop_caches.c.

Signed-off-by: tangmeng <tangmeng@uniontech.com>
---
 fs/drop_caches.c   | 26 ++++++++++++++++++++++++--
 include/linux/mm.h |  6 ------
 kernel/sysctl.c    |  9 ---------
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/fs/drop_caches.c b/fs/drop_caches.c
index e619c31b6bd9..5e044a2d75ef 100644
--- a/fs/drop_caches.c
+++ b/fs/drop_caches.c
@@ -13,7 +13,7 @@
 #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 +47,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 +75,25 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,
 	}
 	return 0;
 }
+
+#ifdef CONFIG_SYSCTL
+static struct ctl_table vm_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 __init int vm_drop_caches_sysctls_init(void)
+{
+	register_sysctl_init("vm", vm_drop_caches_table);
+	return 0;
+}
+late_initcall(vm_drop_caches_sysctls_init);
+#endif /* CONFIG_SYSCTL */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c3c7cb58c847..775befb2786b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3126,12 +3126,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 b51b0b92fdc1..657b7bfe38f6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2182,15 +2182,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.20.1

Re: [PATCH 10/11] fs/drop_caches: move drop_caches sysctls to its own file

[Matthew Wilcox]

On Sun, Feb 20, 2022 at 02:06:26PM +0800, tangmeng wrote:
> diff --git a/fs/drop_caches.c b/fs/drop_caches.c
> @@ -75,3 +75,25 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,
>  	}
>  	return 0;
>  }
> +
> +#ifdef CONFIG_SYSCTL

fs/Makefile has:
obj-$(CONFIG_SYSCTL)            += drop_caches.o

so we don't need this ifdef.

> +static struct ctl_table vm_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,
> +	},
> +	{ }
> +};

Something which slightly concerns me about this sysctl splitup (which
is obviously the right thing to do) is that ctl_table is quite large
(64 bytes per entry) and every array is terminated with an empty one.
In this example, we've gone from 64 bytes to 128 bytes.

Would we be better off having a register_sysctl_one() which
registers exactly one ctl_table, rather than an array?  And/or a
register_sysctl_array() which takes an ARRAY_SIZE() of its argument
instead of looking for the NULL terminator?

Re: [PATCH 10/11] fs/drop_caches: move drop_caches sysctls to its own file

[tangmeng]

On 2022/2/20 19:40, Matthew Wilcox wrote:
> On Sun, Feb 20, 2022 at 02:06:26PM +0800, tangmeng wrote:
>> diff --git a/fs/drop_caches.c b/fs/drop_caches.c
>> @@ -75,3 +75,25 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,
>>   	}
>>   	return 0;
>>   }
>> +
>> +#ifdef CONFIG_SYSCTL
> 
> fs/Makefile has:
> obj-$(CONFIG_SYSCTL)            += drop_caches.o
> 
> so we don't need this ifdef.
Thanks for the heads up! I will delete this ifdef.

> 
>> +static struct ctl_table vm_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,
>> +	},
>> +	{ }
>> +};
> 
> Something which slightly concerns me about this sysctl splitup (which
> is obviously the right thing to do) is that ctl_table is quite large
> (64 bytes per entry) and every array is terminated with an empty one.
> In this example, we've gone from 64 bytes to 128 bytes.
> 
> Would we be better off having a register_sysctl_one() which
> registers exactly one ctl_table, rather than an array?  And/or a
> register_sysctl_array() which takes an ARRAY_SIZE() of its argument
> instead of looking for the NULL terminator?
> 
I think it is obviously the right thing that we need to do.
However, many submissions have been commited which registers an array 
before, I think that having a register_sysctl_one() which registers 
exactly one ctl_table should submit in a separate submission, rather 
than modify it this time.

Re: [PATCH 10/11] fs/drop_caches: move drop_caches sysctls to its own file

[Luis Chamberlain]

On Mon, Feb 21, 2022 at 09:55:18AM +0800, tangmeng wrote:
> I think it is obviously the right thing that we need to do.

Since you are following up on more changes, can you work on this?
Brownie points if you show size results to refelct no size difference
based on a new build with an example new user.

> However, many submissions have been commited which registers an array
> before, I think that having a register_sysctl_one() which registers exactly
> one ctl_table should submit in a separate submission, rather than modify it
> this time.

We can optimize this later and fix those.

  Luis

Re: [PATCH 10/11] fs/drop_caches: move drop_caches sysctls to its own file

[tangmeng]

On 2022/2/23 08:39, Luis Chamberlain wrote:
> 
> Since you are following up on more changes, can you work on this?
> Brownie points if you show size results to refelct no size difference
> based on a new build with an example new user.
> 

I'm going work on this, but I think it may take some times to get it done.

Before ctl_table optimization, I optimized ctl_path first. If the 
optimization method for ctl_path is reasonable, I will apply the 
optimization method to the optimization of ctl_table.

The optimization of ctl_path refers to the subject which is as follows:

[PATCH] fs/proc: Optimize arrays defined by struct ctl_path

Best regards!

Meng Tang

Re: [PATCH 10/11] fs/drop_caches: move drop_caches sysctls to its own file

[Luis Chamberlain]

On Wed, Feb 23, 2022 at 05:51:33PM +0800, tangmeng wrote:
> On 2022/2/23 08:39, Luis Chamberlain wrote:
> > 
> > Since you are following up on more changes, can you work on this?
> > Brownie points if you show size results to refelct no size difference
> > based on a new build with an example new user.
> > 
> 
> I'm going work on this, but I think it may take some times to get it done.

Thanks and good stuff! Can you please then make that the focus of your
first set of patches, and then respin this series by collecting all
ACKs, Reviwed-bys? Yes please send a new v3 series for it. While at it
please Cc the other folks doing sysctls changes so they are aware of
this.

Thanks!

  Luis