modpost: vmlinux: section mismatch in reference: __list_add (section: .text.unlikely.) -> dir_list (section: .init.data)

[nickdesaulniers]

Both Android and ChromeOS are hitting a modpost warning when enabling AutoFDO. They appear subtly different, but seem to be referring to the same (or similar) callers/callees:

Android:

WARNING: modpost: vmlinux: section mismatch in reference: list_add+0x0 (section: .text.hot.list_add) -> dir_list (section: .init.data)

ChromeOS

WARNING: modpost: vmlinux.o: section mismatch in reference: __list_add (section: .text.unlikely.) -> dir_list (section: .init.data)

Note: one is list_add (no __ prefix), one is __list_add. Both refer to dir_list. I recall there's some what color is your function stuff going on with __init, but forget how we've fixed these previously. It looks like dir_list is defined in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/init/initramfs.c#n155.

I suspect that the call to list_add from dir_list is being specialized for &dir_list, since dir_list is static __initdata, but the specialization isn't being placed in __init.

Thanks to @ajordanr-google for the report. Jordan can provide AutoFDO profiles (if necessary, but I think we can come up with a fix without them) and help test prospective fixes.

[nathanchance]

Do these go away if list_add() and __list_add() are marked as __always_inline instead of just inline?

[ajordanr-google]

I've confirmed this can be fixed with:

diff --git a/include/linux/list.h b/include/linux/list.h
index 164b4d0e9d2a..272ce46ff289 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -143,9 +143,9 @@ static inline bool __list_del_entry_valid(struct list_head *entry)
  * This is only for internal list manipulation where we know
  * the prev/next entries already!
  */
-static inline void __list_add(struct list_head *new,
-                             struct list_head *prev,
-                             struct list_head *next)
+static __always_inline void __list_add(struct list_head *new,
+                                     struct list_head *prev,
+                                     struct list_head *next)
 {
        if (!__list_add_valid(new, prev, next))
                return;
@@ -164,7 +164,7 @@ static inline void __list_add(struct list_head *new,
  * Insert a new entry after the specified head.
  * This is good for implementing stacks.
  */
-static inline void list_add(struct list_head *new, struct list_head *head)
+static __always_inline void list_add(struct list_head *new, struct list_head *head)
 {
        __list_add(new, head, head->next);
 }

But this is overly aggressive. It seems however that the kernel expects this to always be inlined? the modpost check can never work if it's not inlined.

Do we just want to do that?

[nickdesaulniers]

the modpost check can never work if it's not inlined.

Note: list_add is called throughout the kernel, from __init and non-__init functions alike. This specific issue occurs when constant propagation specializes the call to list_add in init/initramfs.c (because there are no other call sites of this static inline functions, and one of the parameters is a constant, so we can constant propagate it into list_add then delete the corresponding parameter and call frame setup.

The downside to marking list_add and __list_add always_inline is I would expect non-trivial code size increases of .text in vmlinux; but perhaps it's negligible. Would be good to measure via bloaty.

[nathanchance]

Eric Dumazet happened to notice list_add() not being inlined and did some measurements, which seemed to indicate it would be more beneficial to force inlining but I am not sure how many different configurations he tested with:

https://lore.kernel.org/CANn89iJVQe=wedLheJmjZjOTJsWHijT0jZs=iRxKssJZbjAxHw@mail.gmail.com/

[nickdesaulniers]

I suspect maybe you'd want to do something differently for CONFIG_LIST_HARDENED; the always_inline on __list_add_valid looks concerning. I suspect maybe list_add, and __list_add should be always_inline, while the always_inline on __list_add_valid (when CONFIG_LIST_HARDENED is set) should be made no_inline). (probably same for __list_del_entry_valid wrt the same config).

[nickdesaulniers]

@ajordanr-google is ChromeOS setting CONFIG_LIST_HARDENED or CONFIG_DEBUG_LIST in its .config for these kernel builds, by chance?

[ajordanr-google]

CONFIG_LIST_HARDENED

This is set in our newer kernels (e.g. 6.10, 6.12) but not 6.1, at least that I can see. Only 6.1 has this issue.

CONFIG_DEBUG_LIST

Not that I can see.

[ajordanr-google]

Eric Dumazet happened to notice list_add() not being inlined and did some measurements, which seemed to indicate it would be more beneficial to force inlining but I am not sure how many different configurations he tested with:

https://lore.kernel.org/CANn89iJVQe=wedLheJmjZjOTJsWHijT0jZs=iRxKssJZbjAxHw@mail.gmail.com/

This seems actually pretty strongly suggesting the simplest and perhaps best solution would be to mark this as __always_inline, at least for 6.1, which would likely definitively solve this issue at least for list_add. We quite like our up-to-date Kernel AFDO, so trying to balance both downstream urgency and also making sure we at least have an upstream record of this first.

What's our best path forward here?

[nickdesaulniers]

at least for 6.1

The linux kernel doesn't generally do LTS branch specific patches like that. We'd want a general fix upstream; then once we had some confidence in it we could request it be backported to stable tree branches.

so trying to balance both downstream urgency

If you want the smallest hack you can ship immediately then move on, I'd suggest removing __initdata from dir_list defined in init/initramfs.c. This will leak one struct list_head that could have been cleaned up after init, but it doesn't make widespread changes that modifying list_add+__list_add would.

If you don't have time pressure, then we can probably do a little more due diligence here, which would help (IMO) land a larger change upstream (such as making list_add/__list_add always inline).

For example, if we do:

diff --git a/init/Makefile b/init/Makefile
index d6f75d8907e0..3f909f171fb3 100644
--- a/init/Makefile
+++ b/init/Makefile
@@ -60,3 +60,5 @@ include/generated/utsversion.h: FORCE
 
 $(obj)/version-timestamp.o: include/generated/utsversion.h
 CFLAGS_version-timestamp.o := -include include/generated/utsversion.h
+
+CFLAGS_initramfs.o := -Rpass-missed=inline

$ make LLVM=1 -j$(nproc) init/initramfs.o

We get a flood of optimization remark emitter diagnostics.

There's then a handful of flags and utils here I'd love to play with, time permitting:

• -fdiagnostics-show-hotness: Appends the profile execution count to remark diagnostics.
• -fdiagnostics-hotness-threshold=<N|auto>: Suppresses remarks for callsites that execute fewer than N times in
  the profile (or use auto to use the cutoff from the profile summary).
• -fsave-optimization-record[=yaml|bitstream]: Serializes all remarks to disk.
• -foptimization-record-passes=<regex>: Restricts saved remarks only to passes matching the regex (e.g., inline).
• -foptimization-record-file=<file>: Specifies output file location.
• llvm-remarkutil filter --function=my_critical_function remarks.opt.yaml
• llvm-remarkutil summary --inline-callees remarks.opt.yaml
• llvm/tools/opt-viewer/opt-viewer.py
• llvm-opt-report

So for example, we can do:

diff --git a/init/Makefile b/init/Makefile
index d6f75d8907e0..9e51cc5eb344 100644
--- a/init/Makefile
+++ b/init/Makefile
@@ -60,3 +60,5 @@ include/generated/utsversion.h: FORCE
 
 $(obj)/version-timestamp.o: include/generated/utsversion.h
 CFLAGS_version-timestamp.o := -include include/generated/utsversion.h
+
+CFLAGS_initramfs.o := -Rpass-missed=inline -fsave-optimization-record=yaml

$ make LLVM=1 -j$(nproc) init/initramfs.o
$ ~/llvm-project/llvm/tools/opt-viewer/opt-viewer.py init/initramfs.opt.yaml
$ xdg-open html/index.html

and get some nice UI about inlining decisions.

---

---

Of course, if it comes down to cost vs threshold for inlining, answering "why does the cost have that value?" does become time consuming.

[nickdesaulniers]

cc @melver @kees since b16c42c looks relevant.

[ajordanr-google]

I'll add some additional context with our AFDO profile. While list_add is correctly inlined in our case for dir_add, __list_add is not. We can see this here with our applied profile:

(as per the instructions provided by Nick above). But basically, yeah, it's a question of "why is the cost so high" and "why is the threshold low"? Evidently these 8 samples were sufficient to sway it.

[nathanchance]

But basically, yeah, it's a question of "why is the cost so high" and "why is the threshold low"? Evidently these 8 samples were sufficient to sway it.

@nickdesaulniers, @arndb, and I talked about this in the ClangBuiltLinux meeting last week. Arnd and I have noticed that clang sometimes refuses to inline functions from .text into other sections (like .init.text in this case). Another contributing factor could be that __init includes __attribute__((__cold__)), which might make clang less inclined to inline.