Re: [PATCH V5 1/2] mm: compaction: move compaction sysctl to its own file

[Luis Chamberlain]

On Thu, Mar 23, 2023 at 05:19:15PM +0100, Vlastimil Babka wrote:
> On 3/22/23 09:35, Christoph Hellwig wrote:
> > On Wed, Mar 22, 2023 at 10:46:28AM +0800, ye.xingchen@xxxxxxxxxx wrote:
> >> From: Minghao Chi <chi.minghao@xxxxxxxxxx>
> >> 
> >> This moves all compaction sysctls to its own file.
> > 
> > So there's a whole lot of these 'move sysctrls to their own file'
> > patches, but no actual explanation of why that is desirable.  Please
> 
> I think Luis started this initiative, maybe he can provide the canonical
> reasoning :)

The kernel/sysctl.c is flooded now with commit log entries which
describe a proper rationale, however some folks forget to also include
similar rationale in new patches. I try to remind folks when I can,
thanks for reminding them to continue to do that. That needs to be fixed
in this patch. The summary is its hard to coordiante merge conflicts
with all the syctls in one place, best to just put them where they are
used.

There is a small hidden penalty increase in size to the kernel with the
sysctls moves today though and one for which Matthew Wilcox has recently asked
for us to pause these moves until we can save more memory. The extra memory
is caused by the extra empty struct ctl_table added to the end of the new
array. The way to avoid that penalty is to deprecate all APIs in sysctl
registation which deal with complex array structures. I have some some
of that addressed on my sysctl-next tree (and merged on linux-next) but
much more work is required to deprecate the older APIs. I was ready to
pause the kernel/sysctl.c moves until those APIs are deprecated and we
start having sysctl APIs which allow us to not have the empty array at
the end. But as I thought about this just now, the use cases that move
the sysctls where __init is used could benefit already in size.

For this patch there seems to be a savings of 4 bytes:

$ ./scripts/bloat-o-meter vmlinux.old vmlinux
add/remove: 1/0 grow/shrink: 1/2 up/down: 346/-350 (-4)
Function                                     old     new   delta
vm_compaction                                  -     320    +320
kcompactd_init                               167     193     +26
proc_dointvec_minmax_warn_RT_change          104      10     -94
vm_table                                    2112    1856    -256
Total: Before=19287558, After=19287554, chg -0.00%

So I don't think we need to pause this move or others where are have savings.

Minghao, can you fix the commit log, and explain how you are also saving
4 bytes as per the above bloat-o-meter results?

> > explain why we'd want to split code that is closely related, and now
> > requires marking symbols non-static just to create a new tiny source
> > file.
> 
> Hmm? I can see the opposite, at least in the compaction patch here. Related
> code and variables are moved closer together, made static, declarations
> removed from headers. It looks like an improvement to me.

Glad this helps.

  Luis