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

[tangmeng <tangmeng@uniontech.com>]

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.