| Message ID | 20230808015831.608454-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: remove unneeded __meminit annotation | expand |
On Tue, Aug 08, 2023 at 09:58:31AM +0800, Miaohe Lin wrote: > kswapd_stop() and kcompactd_stop() are only called when MEMORY_HOTREMOVE > is enabled. So wrap them under CONFIG_MEMORY_HOTREMOVE and further remove > __meminit annotation. No functional change intended. I don't understand why this is an improvement. If CONFIG_MEMORY_HOTREMOVE is disabled, the linker drops this section (... right?) If it's enabled, then it gets shunted off into a cold section. So it seems like this patch strictly makes things worse. But maybe I misunderstood.
On 2023/8/8 10:31, Matthew Wilcox wrote: > On Tue, Aug 08, 2023 at 09:58:31AM +0800, Miaohe Lin wrote: >> kswapd_stop() and kcompactd_stop() are only called when MEMORY_HOTREMOVE >> is enabled. So wrap them under CONFIG_MEMORY_HOTREMOVE and further remove >> __meminit annotation. No functional change intended. > > I don't understand why this is an improvement. If CONFIG_MEMORY_HOTREMOVE > is disabled, the linker drops this section (... right?) If it's enabled, When CONFIG_MEMORY_HOTREMOVE is disabled, without this patch: size mm/compaction.o text data bss dec hex filename 103164 30873 0 134037 20b95 mm/compaction.o size mm/vmscan.o text data bss dec hex filename 158775 49612 64 208451 32e43 mm/vmscan.o while with this patch: size mm/compaction.o text data bss dec hex filename 102915 30865 0 133780 20a94 mm/compaction.o size mm/vmscan.o text data bss dec hex filename 158534 49604 64 208202 32d4a mm/vmscan.o We can reduce each .o by ~250 bytes. > then it gets shunted off into a cold section. So it seems like this > patch strictly makes things worse. But maybe I misunderstood. What about add __cold annotation? Something like: diff --git a/mm/vmscan.c b/mm/vmscan.c index eb4db273bf7e..e27ffa22c70f 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -7902,7 +7902,7 @@ void __meminit kswapd_run(int nid) * Called by memory hotplug when all memory in a node is offlined. Caller must * be holding mem_hotplug_begin/done(). */ -void kswapd_stop(int nid) +void __cold kswapd_stop(int nid) { pg_data_t *pgdat = NODE_DATA(nid); struct task_struct *kswapd; Thanks.
On Tue, Aug 08, 2023 at 11:08:36AM +0800, Miaohe Lin wrote: > On 2023/8/8 10:31, Matthew Wilcox wrote: > > On Tue, Aug 08, 2023 at 09:58:31AM +0800, Miaohe Lin wrote: > >> kswapd_stop() and kcompactd_stop() are only called when MEMORY_HOTREMOVE > >> is enabled. So wrap them under CONFIG_MEMORY_HOTREMOVE and further remove > >> __meminit annotation. No functional change intended. > > > > I don't understand why this is an improvement. If CONFIG_MEMORY_HOTREMOVE > > is disabled, the linker drops this section (... right?) If it's enabled, > > When CONFIG_MEMORY_HOTREMOVE is disabled, without this patch: > > size mm/compaction.o > text data bss dec hex filename > 103164 30873 0 134037 20b95 mm/compaction.o > > size mm/vmscan.o > text data bss dec hex filename > 158775 49612 64 208451 32e43 mm/vmscan.o > > while with this patch: > > size mm/compaction.o > text data bss dec hex filename > 102915 30865 0 133780 20a94 mm/compaction.o > > size mm/vmscan.o > text data bss dec hex filename > 158534 49604 64 208202 32d4a mm/vmscan.o > > We can reduce each .o by ~250 bytes. But this is before the linker step! That will be where the meminit sections get dropped. Assuming they are; I haven't verified. You need to compare before/after of the vmlinux, not the individual .o files.
On Tue, Aug 08, 2023 at 04:16:57AM +0100, Matthew Wilcox wrote: > On Tue, Aug 08, 2023 at 11:08:36AM +0800, Miaohe Lin wrote: > > On 2023/8/8 10:31, Matthew Wilcox wrote: > > > On Tue, Aug 08, 2023 at 09:58:31AM +0800, Miaohe Lin wrote: > > >> kswapd_stop() and kcompactd_stop() are only called when MEMORY_HOTREMOVE > > >> is enabled. So wrap them under CONFIG_MEMORY_HOTREMOVE and further remove > > >> __meminit annotation. No functional change intended. > > > > > > I don't understand why this is an improvement. If CONFIG_MEMORY_HOTREMOVE > > > is disabled, the linker drops this section (... right?) If it's enabled, > > > > When CONFIG_MEMORY_HOTREMOVE is disabled, without this patch: > > > > size mm/compaction.o > > text data bss dec hex filename > > 103164 30873 0 134037 20b95 mm/compaction.o > > > > size mm/vmscan.o > > text data bss dec hex filename > > 158775 49612 64 208451 32e43 mm/vmscan.o > > > > while with this patch: > > > > size mm/compaction.o > > text data bss dec hex filename > > 102915 30865 0 133780 20a94 mm/compaction.o > > > > size mm/vmscan.o > > text data bss dec hex filename > > 158534 49604 64 208202 32d4a mm/vmscan.o > > > > We can reduce each .o by ~250 bytes. > > But this is before the linker step! That will be where the meminit > sections get dropped. Assuming they are; I haven't verified. You need > to compare before/after of the vmlinux, not the individual .o files. Ah, found it: #if defined(CONFIG_MEMORY_HOTPLUG) #define MEM_KEEP(sec) *(.mem##sec) #define MEM_DISCARD(sec) #else #define MEM_KEEP(sec) #define MEM_DISCARD(sec) *(.mem##sec) #endif in include/asm-generic/vmlinux.lds.h So yeah, I think this patch is garbage.
On 2023/8/8 11:26, Matthew Wilcox wrote: > On Tue, Aug 08, 2023 at 04:16:57AM +0100, Matthew Wilcox wrote: >> On Tue, Aug 08, 2023 at 11:08:36AM +0800, Miaohe Lin wrote: >>> On 2023/8/8 10:31, Matthew Wilcox wrote: >>>> On Tue, Aug 08, 2023 at 09:58:31AM +0800, Miaohe Lin wrote: >>>>> kswapd_stop() and kcompactd_stop() are only called when MEMORY_HOTREMOVE >>>>> is enabled. So wrap them under CONFIG_MEMORY_HOTREMOVE and further remove >>>>> __meminit annotation. No functional change intended. >>>> >>>> I don't understand why this is an improvement. If CONFIG_MEMORY_HOTREMOVE >>>> is disabled, the linker drops this section (... right?) If it's enabled, >>> >>> When CONFIG_MEMORY_HOTREMOVE is disabled, without this patch: >>> >>> size mm/compaction.o >>> text data bss dec hex filename >>> 103164 30873 0 134037 20b95 mm/compaction.o >>> >>> size mm/vmscan.o >>> text data bss dec hex filename >>> 158775 49612 64 208451 32e43 mm/vmscan.o >>> >>> while with this patch: >>> >>> size mm/compaction.o >>> text data bss dec hex filename >>> 102915 30865 0 133780 20a94 mm/compaction.o >>> >>> size mm/vmscan.o >>> text data bss dec hex filename >>> 158534 49604 64 208202 32d4a mm/vmscan.o >>> >>> We can reduce each .o by ~250 bytes. >> >> But this is before the linker step! That will be where the meminit >> sections get dropped. Assuming they are; I haven't verified. You need >> to compare before/after of the vmlinux, not the individual .o files. When CONFIG_MEMORY_HOTREMOVE is disabled, without patch: size vmlinux text data bss dec hex filename 70648396 21668938 17985540 110302874 693169a vmlinux while with patch: size vmlinux text data bss dec hex filename 70648128 21668938 17985540 110302606 693158e vmlinux vmlinux is still reduced by 268 bytes. > > Ah, found it: > > #if defined(CONFIG_MEMORY_HOTPLUG) > #define MEM_KEEP(sec) *(.mem##sec) > #define MEM_DISCARD(sec) > #else > #define MEM_KEEP(sec) > #define MEM_DISCARD(sec) *(.mem##sec) > #endif > > in include/asm-generic/vmlinux.lds.h When CONFIG_MEMORY_HOTREMOVE is disabled, without this patch, kswapd_stop() and kcompactd_stop() are dropped after system init. But no insist in this patch. Thanks for your comment.
diff --git a/include/linux/compaction.h b/include/linux/compaction.h index e94776496049..3dbb7eea96eb 100644 --- a/include/linux/compaction.h +++ b/include/linux/compaction.h @@ -99,7 +99,7 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order, int alloc_flags); extern void __meminit kcompactd_run(int nid); -extern void __meminit kcompactd_stop(int nid); +extern void kcompactd_stop(int nid); extern void wakeup_kcompactd(pg_data_t *pgdat, int order, int highest_zoneidx); #else diff --git a/include/linux/swap.h b/include/linux/swap.h index bb5adc604144..c102587b3a86 100644 --- a/include/linux/swap.h +++ b/include/linux/swap.h @@ -437,7 +437,7 @@ static inline bool node_reclaim_enabled(void) void check_move_unevictable_folios(struct folio_batch *fbatch); extern void __meminit kswapd_run(int nid); -extern void __meminit kswapd_stop(int nid); +extern void kswapd_stop(int nid); #ifdef CONFIG_SWAP diff --git a/mm/compaction.c b/mm/compaction.c index ea61922a1619..c59244d2ed5a 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -3132,11 +3132,12 @@ void __meminit kcompactd_run(int nid) } } +#ifdef CONFIG_MEMORY_HOTREMOVE /* * Called by memory hotplug when all memory in a node is offlined. Caller must * be holding mem_hotplug_begin/done(). */ -void __meminit kcompactd_stop(int nid) +void kcompactd_stop(int nid) { struct task_struct *kcompactd = NODE_DATA(nid)->kcompactd; @@ -3145,6 +3146,7 @@ void __meminit kcompactd_stop(int nid) NODE_DATA(nid)->kcompactd = NULL; } } +#endif /* * It's optimal to keep kcompactd on the same CPUs as their memory, but diff --git a/mm/vmscan.c b/mm/vmscan.c index 80e9a222e522..eb4db273bf7e 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -7897,11 +7897,12 @@ void __meminit kswapd_run(int nid) pgdat_kswapd_unlock(pgdat); } +#ifdef CONFIG_MEMORY_HOTREMOVE /* * Called by memory hotplug when all memory in a node is offlined. Caller must * be holding mem_hotplug_begin/done(). */ -void __meminit kswapd_stop(int nid) +void kswapd_stop(int nid) { pg_data_t *pgdat = NODE_DATA(nid); struct task_struct *kswapd; @@ -7914,6 +7915,7 @@ void __meminit kswapd_stop(int nid) } pgdat_kswapd_unlock(pgdat); } +#endif static int __init kswapd_init(void) {
kswapd_stop() and kcompactd_stop() are only called when MEMORY_HOTREMOVE is enabled. So wrap them under CONFIG_MEMORY_HOTREMOVE and further remove __meminit annotation. No functional change intended. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- include/linux/compaction.h | 2 +- include/linux/swap.h | 2 +- mm/compaction.c | 4 +++- mm/vmscan.c | 4 +++- 4 files changed, 8 insertions(+), 4 deletions(-)