From: Helge Deller <[email protected]>
An alignment of 4 bytes is wrong for 64-bit platforms which don't define
CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (which then store 64-bit pointers).
Fix their alignment to 8 bytes.
Signed-off-by: Helge Deller <[email protected]>
---
include/linux/export-internal.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/export-internal.h b/include/linux/export-internal.h
index 69501e0ec239..cd253eb51d6c 100644
--- a/include/linux/export-internal.h
+++ b/include/linux/export-internal.h
@@ -16,10 +16,13 @@
* and eliminates the need for absolute relocations that require runtime
* processing on relocatable kernels.
*/
+#define __KSYM_ALIGN ".balign 4"
#define __KSYM_REF(sym) ".long " #sym "- ."
#elif defined(CONFIG_64BIT)
+#define __KSYM_ALIGN ".balign 8"
#define __KSYM_REF(sym) ".quad " #sym
#else
+#define __KSYM_ALIGN ".balign 4"
#define __KSYM_REF(sym) ".long " #sym
#endif
@@ -42,7 +45,7 @@
" .asciz \"" ns "\"" "\n" \
" .previous" "\n" \
" .section \"___ksymtab" sec "+" #name "\", \"a\"" "\n" \
- " .balign 4" "\n" \
+ __KSYM_ALIGN "\n" \
"__ksymtab_" #name ":" "\n" \
__KSYM_REF(sym) "\n" \
__KSYM_REF(__kstrtab_ ##name) "\n" \
--
2.41.0
On Thu, Nov 23, 2023 at 7:18 AM <[email protected]> wrote:
>
> From: Helge Deller <[email protected]>
>
> An alignment of 4 bytes is wrong for 64-bit platforms which don't define
> CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (which then store 64-bit pointers).
> Fix their alignment to 8 bytes.
>
> Signed-off-by: Helge Deller <[email protected]>
This is correct.
Acked-by: Masahiro Yamada <[email protected]>
Please add
Fixes: ddb5cdbafaaa ("kbuild: generate KSYMTAB entries by modpost")
> ---
> include/linux/export-internal.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/export-internal.h b/include/linux/export-internal.h
> index 69501e0ec239..cd253eb51d6c 100644
> --- a/include/linux/export-internal.h
> +++ b/include/linux/export-internal.h
> @@ -16,10 +16,13 @@
> * and eliminates the need for absolute relocations that require runtime
> * processing on relocatable kernels.
> */
> +#define __KSYM_ALIGN ".balign 4"
> #define __KSYM_REF(sym) ".long " #sym "- ."
> #elif defined(CONFIG_64BIT)
> +#define __KSYM_ALIGN ".balign 8"
> #define __KSYM_REF(sym) ".quad " #sym
> #else
> +#define __KSYM_ALIGN ".balign 4"
> #define __KSYM_REF(sym) ".long " #sym
> #endif
>
> @@ -42,7 +45,7 @@
> " .asciz \"" ns "\"" "\n" \
> " .previous" "\n" \
> " .section \"___ksymtab" sec "+" #name "\", \"a\"" "\n" \
> - " .balign 4" "\n" \
> + __KSYM_ALIGN "\n" \
> "__ksymtab_" #name ":" "\n" \
> __KSYM_REF(sym) "\n" \
> __KSYM_REF(__kstrtab_ ##name) "\n" \
> --
> 2.41.0
>
--
Best Regards
Masahiro Yamada
On Thu, Dec 21, 2023 at 7:22 PM Masahiro Yamada <[email protected]> wrote:
>
> On Thu, Nov 23, 2023 at 7:18 AM <[email protected]> wrote:
> >
> > From: Helge Deller <[email protected]>
> >
> > An alignment of 4 bytes is wrong for 64-bit platforms which don't define
> > CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (which then store 64-bit pointers).
> > Fix their alignment to 8 bytes.
> >
> > Signed-off-by: Helge Deller <[email protected]>
>
>
> This is correct.
>
> Acked-by: Masahiro Yamada <[email protected]>
>
> Please add
>
>
> Fixes: ddb5cdbafaaa ("kbuild: generate KSYMTAB entries by modpost")
>
>
If there is no objection, I will pick this up
to linux-kbuild/fixes.
Thanks.
--
Best Regards
Masahiro Yamada
On Fri, Dec 22, 2023 at 01:01:23AM +0900, Masahiro Yamada wrote:
> On Thu, Dec 21, 2023 at 7:22 PM Masahiro Yamada <[email protected]> wrote:
> >
> > On Thu, Nov 23, 2023 at 7:18 AM <[email protected]> wrote:
> > >
> > > From: Helge Deller <[email protected]>
> > >
> > > An alignment of 4 bytes is wrong for 64-bit platforms which don't define
> > > CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (which then store 64-bit pointers).
> > > Fix their alignment to 8 bytes.
> > >
> > > Signed-off-by: Helge Deller <[email protected]>
> >
> >
> > This is correct.
> >
> > Acked-by: Masahiro Yamada <[email protected]>
> >
> > Please add
> >
> >
> > Fixes: ddb5cdbafaaa ("kbuild: generate KSYMTAB entries by modpost")
> >
> >
>
>
> If there is no objection, I will pick this up
> to linux-kbuild/fixes.
The new selftests I've suggested should help get perf data to cover both
modules and built-in kernel symbols given find_symbol() will first hit
built-in symbols first before modules with one caveat: we'd want to extend
the selftest with a part which builds a module built-in with also tons
of other symbols.
So I'm all for you taking this but I don't think we need to rush for the
same reasons I mentioned in my reply to Helge.
I think it would be nice to get real perf data with perf stat as I
suggested, and include that in the commit logs. I think it would also be
useful to include a description about the fact that there is no real fix
and that the performance hit is all that happens as the architecture
just emulates the aligment. In the worst case, if exception handlers
are broken we could crash but that is rare although it does happen.
If we want to go bananas we could even get a graph of size of modules
Vs cost on misaligment as a relationship with time. Without this, frankly
cost on "performance" is artificial.
Thoughts?
Luis
On Thu, Dec 21, 2023 at 10:07:13PM -0800, Luis Chamberlain wrote:
>
> If we want to go bananas we could even get a graph of size of modules
Sorry I meant size of number of symbols Vs cost.
Luis
On Fri, Dec 22, 2023 at 3:08 PM Luis Chamberlain <[email protected]> wrote:
>
> On Thu, Dec 21, 2023 at 10:07:13PM -0800, Luis Chamberlain wrote:
> >
> > If we want to go bananas we could even get a graph of size of modules
>
> Sorry I meant size of number of symbols Vs cost.
>
> Luis
But, 1/4 is really a bug-fix, isn't it?
ksymtab was previously 8-byte aligned for CONFIG_64BIT,
but now is only 4-byte aligned.
$ git show ddb5cdbafaaa^:include/linux/export.h |
head -n66 | tail -n5
struct kernel_symbol {
unsigned long value;
const char *name;
const char *namespace;
};
$ git show ddb5cdbafaaa^:include/asm-generic/export.h |
head -23 | tail -8
#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
#define KSYM_ALIGN 4
#elif defined(CONFIG_64BIT)
#define KSYM_ALIGN 8
#else
#define KSYM_ALIGN 4
#endif
In the old behavior, <linux/export.h> used C code
for producing ksymtab, hence it was naturally
aligned by the compiler. (unsigned long and pointer
require 8-byte alignment for CONFIG_64BIT)
<asm-generic/export.h> explicitly required
8-byte alignment for CONFIG_64BIT.
In the current behavior, <linux/export-internal.h>
produces all ksymtab by using inline assembler,
but it hard-codes ".balign 4".
--
Best Regards
Masahiro Yamada
On Fri, Dec 22, 2023 at 04:01:30PM +0900, Masahiro Yamada wrote:
> On Fri, Dec 22, 2023 at 3:08 PM Luis Chamberlain <[email protected]> wrote:
> >
> > On Thu, Dec 21, 2023 at 10:07:13PM -0800, Luis Chamberlain wrote:
> > >
> > > If we want to go bananas we could even get a graph of size of modules
> >
> > Sorry I meant size of number of symbols Vs cost.
> >
> > Luis
>
>
>
> But, 1/4 is really a bug-fix, isn't it?
Ah you mean a regression fix, yeah sure, thanks I see !
Luis
On Sat, Dec 23, 2023 at 5:11 AM Luis Chamberlain <[email protected]> wrote:
>
> On Fri, Dec 22, 2023 at 04:01:30PM +0900, Masahiro Yamada wrote:
> > On Fri, Dec 22, 2023 at 3:08 PM Luis Chamberlain <[email protected]> wrote:
> > >
> > > On Thu, Dec 21, 2023 at 10:07:13PM -0800, Luis Chamberlain wrote:
> > > >
> > > > If we want to go bananas we could even get a graph of size of modules
> > >
> > > Sorry I meant size of number of symbols Vs cost.
> > >
> > > Luis
> >
> >
> >
> > But, 1/4 is really a bug-fix, isn't it?
>
> Ah you mean a regression fix, yeah sure, thanks I see !
>
> Luis
Now, I applied 1/4 to linux-kbuild/fixes.
Thanks.
--
Best Regards
Masahiro Yamada