| Message ID | 20230321130908.6972-1-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | fs/drop_caches: move drop_caches sysctls into its own file | expand |
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...
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
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.
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
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.
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.
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 --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",
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(-)