This diff uses an open source tool include-what-you-use (IWYU) to modify
the include list, changing indirect includes to direct includes. IWYU is
implemented using the IWYUScripts github repository which is a tool that
is currently undergoing development. These changes seek to improve build
times.
This change to entry/syscall_32.c resulted in a preprocessed size of
entry/syscall_32.i from 64002 lines to 47986 lines (-25%) for the x86
defconfig.
Signed-off-by: Tanzir Hasan <[email protected]>
---
arch/x86/entry/syscall_32.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
index 8cfc9bc73e7f..66db11fe8a1c 100644
--- a/arch/x86/entry/syscall_32.c
+++ b/arch/x86/entry/syscall_32.c
@@ -4,7 +4,7 @@
#include <linux/linkage.h>
#include <linux/sys.h>
#include <linux/cache.h>
-#include <linux/syscalls.h>
+#include <linux/ptrace.h>
#include <asm/syscall.h>
#ifdef CONFIG_IA32_EMULATION
@@ -16,6 +16,7 @@
#define __SYSCALL(nr, sym) extern long __ia32_##sym(const struct pt_regs *);
#include <asm/syscalls_32.h>
+
#undef __SYSCALL
#define __SYSCALL(nr, sym) __ia32_##sym,
---
base-commit: fbafc3e621c3f4ded43720fdb1d6ce1728ec664e
change-id: 20231227-syscall32-2c6d62fe51c9
Best regards,
--
Tanzir Hasan <[email protected]>
On Wed, Dec 27, 2023 at 10:38:41PM +0000, Tanzir Hasan wrote:
> This diff uses an open source tool include-what-you-use (IWYU) to modify
> the include list, changing indirect includes to direct includes. IWYU is
> implemented using the IWYUScripts github repository which is a tool that
> is currently undergoing development. These changes seek to improve build
> times.
>
> This change to entry/syscall_32.c resulted in a preprocessed size of
> entry/syscall_32.i from 64002 lines to 47986 lines (-25%) for the x86
> defconfig.
>
> Signed-off-by: Tanzir Hasan <[email protected]>
> ---
> arch/x86/entry/syscall_32.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
> index 8cfc9bc73e7f..66db11fe8a1c 100644
> --- a/arch/x86/entry/syscall_32.c
> +++ b/arch/x86/entry/syscall_32.c
> @@ -4,7 +4,7 @@
> #include <linux/linkage.h>
> #include <linux/sys.h>
> #include <linux/cache.h>
> -#include <linux/syscalls.h>
> +#include <linux/ptrace.h>
> #include <asm/syscall.h>
Really? What do we need linux/ptrace.h for? Because if it's
struct pt_regs for the generated externs, we might as well have
just said
struct pt_regs;
and that would be it.
<digs around a bit>
As the matter of fact, all you need out of those includes is this:
struct pt_regs;
typedef long (*sys_call_ptr_t)(const struct pt_regs *);
extern const sys_call_ptr_t sys_call_table[];
#if defined(CONFIG_X86_32)
#define ia32_sys_call_table sys_call_table
#else
/*
* These may not exist, but still put the prototypes in so we
* can use IS_ENABLED().
*/
extern const sys_call_ptr_t ia32_sys_call_table[];
extern const sys_call_ptr_t x32_sys_call_table[];
#endif
That's _it_. The same goes for syscall_64.c and syscall_x32.c.
Oh, and lose the __visible/asmlinkage junk in there - none of that
stuff is used from asm these days. See the patch below -
Untested But Should Work(tm):
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index d813160b14d8..5c470806dd08 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -36,6 +36,8 @@
#include <asm/syscall.h>
#include <asm/irq_stack.h>
+#include "syscall.h"
+
#ifdef CONFIG_X86_64
static __always_inline bool do_syscall_x64(struct pt_regs *regs, int nr)
diff --git a/arch/x86/entry/syscall.h b/arch/x86/entry/syscall.h
new file mode 100644
index 000000000000..7c52df000ae0
--- /dev/null
+++ b/arch/x86/entry/syscall.h
@@ -0,0 +1,13 @@
+struct pt_regs;
+typedef long (*sys_call_ptr_t)(const struct pt_regs *);
+extern const sys_call_ptr_t sys_call_table[];
+#if defined(CONFIG_X86_32)
+#define ia32_sys_call_table sys_call_table
+#else
+/*
+ * These may not exist, but still put the prototypes in so we
+ * can use IS_ENABLED().
+ */
+extern const sys_call_ptr_t ia32_sys_call_table[];
+extern const sys_call_ptr_t x32_sys_call_table[];
+#endif
diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
index 8cfc9bc73e7f..a24a8922692f 100644
--- a/arch/x86/entry/syscall_32.c
+++ b/arch/x86/entry/syscall_32.c
@@ -1,11 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/* System call table for i386. */
-#include <linux/linkage.h>
-#include <linux/sys.h>
-#include <linux/cache.h>
-#include <linux/syscalls.h>
-#include <asm/syscall.h>
+#include "syscall.h"
#ifdef CONFIG_IA32_EMULATION
#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
@@ -20,6 +16,6 @@
#define __SYSCALL(nr, sym) __ia32_##sym,
-__visible const sys_call_ptr_t ia32_sys_call_table[] = {
+const sys_call_ptr_t ia32_sys_call_table[] = {
#include <asm/syscalls_32.h>
};
diff --git a/arch/x86/entry/syscall_64.c b/arch/x86/entry/syscall_64.c
index be120eec1fc9..81bde061037b 100644
--- a/arch/x86/entry/syscall_64.c
+++ b/arch/x86/entry/syscall_64.c
@@ -1,11 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/* System call table for x86-64. */
-#include <linux/linkage.h>
-#include <linux/sys.h>
-#include <linux/cache.h>
-#include <linux/syscalls.h>
-#include <asm/syscall.h>
+#include "syscall.h"
#define __SYSCALL(nr, sym) extern long __x64_##sym(const struct pt_regs *);
#include <asm/syscalls_64.h>
@@ -13,6 +9,6 @@
#define __SYSCALL(nr, sym) __x64_##sym,
-asmlinkage const sys_call_ptr_t sys_call_table[] = {
+const sys_call_ptr_t sys_call_table[] = {
#include <asm/syscalls_64.h>
};
diff --git a/arch/x86/entry/syscall_x32.c b/arch/x86/entry/syscall_x32.c
index bdd0e03a1265..7b5170a99b9a 100644
--- a/arch/x86/entry/syscall_x32.c
+++ b/arch/x86/entry/syscall_x32.c
@@ -1,11 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/* System call table for x32 ABI. */
-#include <linux/linkage.h>
-#include <linux/sys.h>
-#include <linux/cache.h>
-#include <linux/syscalls.h>
-#include <asm/syscall.h>
+#include "syscall.h"
#define __SYSCALL(nr, sym) extern long __x64_##sym(const struct pt_regs *);
#include <asm/syscalls_x32.h>
@@ -13,6 +9,6 @@
#define __SYSCALL(nr, sym) __x64_##sym,
-asmlinkage const sys_call_ptr_t x32_sys_call_table[] = {
+const sys_call_ptr_t x32_sys_call_table[] = {
#include <asm/syscalls_x32.h>
};
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index f44e2f9ab65d..f301919b9187 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -16,20 +16,6 @@
#include <asm/thread_info.h> /* for TS_COMPAT */
#include <asm/unistd.h>
-typedef long (*sys_call_ptr_t)(const struct pt_regs *);
-extern const sys_call_ptr_t sys_call_table[];
-
-#if defined(CONFIG_X86_32)
-#define ia32_sys_call_table sys_call_table
-#else
-/*
- * These may not exist, but still put the prototypes in so we
- * can use IS_ENABLED().
- */
-extern const sys_call_ptr_t ia32_sys_call_table[];
-extern const sys_call_ptr_t x32_sys_call_table[];
-#endif
-
/*
* Only the low 32 bits of orig_ax are meaningful, so we return int.
* This importantly ignores the high bits on 64-bit, so comparisons
On December 27, 2023 3:34:44 PM PST, Al Viro <[email protected]> wrote:
>On Wed, Dec 27, 2023 at 10:38:41PM +0000, Tanzir Hasan wrote:
>> This diff uses an open source tool include-what-you-use (IWYU) to modify
>> the include list, changing indirect includes to direct includes. IWYU is
>> implemented using the IWYUScripts github repository which is a tool that
>> is currently undergoing development. These changes seek to improve build
>> times.
>>
>> This change to entry/syscall_32.c resulted in a preprocessed size of
>> entry/syscall_32.i from 64002 lines to 47986 lines (-25%) for the x86
>> defconfig.
>>
>> Signed-off-by: Tanzir Hasan <[email protected]>
>> ---
>> arch/x86/entry/syscall_32.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
>> index 8cfc9bc73e7f..66db11fe8a1c 100644
>> --- a/arch/x86/entry/syscall_32.c
>> +++ b/arch/x86/entry/syscall_32.c
>> @@ -4,7 +4,7 @@
>> #include <linux/linkage.h>
>> #include <linux/sys.h>
>> #include <linux/cache.h>
>> -#include <linux/syscalls.h>
>> +#include <linux/ptrace.h>
>> #include <asm/syscall.h>
>
>Really? What do we need linux/ptrace.h for? Because if it's
>struct pt_regs for the generated externs, we might as well have
>just said
>struct pt_regs;
>and that would be it.
>
><digs around a bit>
>
>As the matter of fact, all you need out of those includes is this:
>
>struct pt_regs;
>typedef long (*sys_call_ptr_t)(const struct pt_regs *);
>extern const sys_call_ptr_t sys_call_table[];
>#if defined(CONFIG_X86_32)
>#define ia32_sys_call_table sys_call_table
>#else
>/*
> * These may not exist, but still put the prototypes in so we
> * can use IS_ENABLED().
> */
>extern const sys_call_ptr_t ia32_sys_call_table[];
>extern const sys_call_ptr_t x32_sys_call_table[];
>#endif
>
>That's _it_. The same goes for syscall_64.c and syscall_x32.c.
>Oh, and lose the __visible/asmlinkage junk in there - none of that
>stuff is used from asm these days. See the patch below -
>Untested But Should Work(tm):
>
>diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>index d813160b14d8..5c470806dd08 100644
>--- a/arch/x86/entry/common.c
>+++ b/arch/x86/entry/common.c
>@@ -36,6 +36,8 @@
> #include <asm/syscall.h>
> #include <asm/irq_stack.h>
>
>+#include "syscall.h"
>+
> #ifdef CONFIG_X86_64
>
> static __always_inline bool do_syscall_x64(struct pt_regs *regs, int nr)
>diff --git a/arch/x86/entry/syscall.h b/arch/x86/entry/syscall.h
>new file mode 100644
>index 000000000000..7c52df000ae0
>--- /dev/null
>+++ b/arch/x86/entry/syscall.h
>@@ -0,0 +1,13 @@
>+struct pt_regs;
>+typedef long (*sys_call_ptr_t)(const struct pt_regs *);
>+extern const sys_call_ptr_t sys_call_table[];
>+#if defined(CONFIG_X86_32)
>+#define ia32_sys_call_table sys_call_table
>+#else
>+/*
>+ * These may not exist, but still put the prototypes in so we
>+ * can use IS_ENABLED().
>+ */
>+extern const sys_call_ptr_t ia32_sys_call_table[];
>+extern const sys_call_ptr_t x32_sys_call_table[];
>+#endif
>diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
>index 8cfc9bc73e7f..a24a8922692f 100644
>--- a/arch/x86/entry/syscall_32.c
>+++ b/arch/x86/entry/syscall_32.c
>@@ -1,11 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /* System call table for i386. */
>
>-#include <linux/linkage.h>
>-#include <linux/sys.h>
>-#include <linux/cache.h>
>-#include <linux/syscalls.h>
>-#include <asm/syscall.h>
>+#include "syscall.h"
>
> #ifdef CONFIG_IA32_EMULATION
> #define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
>@@ -20,6 +16,6 @@
>
> #define __SYSCALL(nr, sym) __ia32_##sym,
>
>-__visible const sys_call_ptr_t ia32_sys_call_table[] = {
>+const sys_call_ptr_t ia32_sys_call_table[] = {
> #include <asm/syscalls_32.h>
> };
>diff --git a/arch/x86/entry/syscall_64.c b/arch/x86/entry/syscall_64.c
>index be120eec1fc9..81bde061037b 100644
>--- a/arch/x86/entry/syscall_64.c
>+++ b/arch/x86/entry/syscall_64.c
>@@ -1,11 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /* System call table for x86-64. */
>
>-#include <linux/linkage.h>
>-#include <linux/sys.h>
>-#include <linux/cache.h>
>-#include <linux/syscalls.h>
>-#include <asm/syscall.h>
>+#include "syscall.h"
>
> #define __SYSCALL(nr, sym) extern long __x64_##sym(const struct pt_regs *);
> #include <asm/syscalls_64.h>
>@@ -13,6 +9,6 @@
>
> #define __SYSCALL(nr, sym) __x64_##sym,
>
>-asmlinkage const sys_call_ptr_t sys_call_table[] = {
>+const sys_call_ptr_t sys_call_table[] = {
> #include <asm/syscalls_64.h>
> };
>diff --git a/arch/x86/entry/syscall_x32.c b/arch/x86/entry/syscall_x32.c
>index bdd0e03a1265..7b5170a99b9a 100644
>--- a/arch/x86/entry/syscall_x32.c
>+++ b/arch/x86/entry/syscall_x32.c
>@@ -1,11 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /* System call table for x32 ABI. */
>
>-#include <linux/linkage.h>
>-#include <linux/sys.h>
>-#include <linux/cache.h>
>-#include <linux/syscalls.h>
>-#include <asm/syscall.h>
>+#include "syscall.h"
>
> #define __SYSCALL(nr, sym) extern long __x64_##sym(const struct pt_regs *);
> #include <asm/syscalls_x32.h>
>@@ -13,6 +9,6 @@
>
> #define __SYSCALL(nr, sym) __x64_##sym,
>
>-asmlinkage const sys_call_ptr_t x32_sys_call_table[] = {
>+const sys_call_ptr_t x32_sys_call_table[] = {
> #include <asm/syscalls_x32.h>
> };
>diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
>index f44e2f9ab65d..f301919b9187 100644
>--- a/arch/x86/include/asm/syscall.h
>+++ b/arch/x86/include/asm/syscall.h
>@@ -16,20 +16,6 @@
> #include <asm/thread_info.h> /* for TS_COMPAT */
> #include <asm/unistd.h>
>
>-typedef long (*sys_call_ptr_t)(const struct pt_regs *);
>-extern const sys_call_ptr_t sys_call_table[];
>-
>-#if defined(CONFIG_X86_32)
>-#define ia32_sys_call_table sys_call_table
>-#else
>-/*
>- * These may not exist, but still put the prototypes in so we
>- * can use IS_ENABLED().
>- */
>-extern const sys_call_ptr_t ia32_sys_call_table[];
>-extern const sys_call_ptr_t x32_sys_call_table[];
>-#endif
>-
> /*
> * Only the low 32 bits of orig_ax are meaningful, so we return int.
> * This importantly ignores the high bits on 64-bit, so comparisons
__visible is for LTO, no?
On Wed, Dec 27, 2023 at 3:34 PM Al Viro <[email protected]> wrote:
>
> On Wed, Dec 27, 2023 at 10:38:41PM +0000, Tanzir Hasan wrote:
> > This diff uses an open source tool include-what-you-use (IWYU) to modify
> > the include list, changing indirect includes to direct includes. IWYU is
> > implemented using the IWYUScripts github repository which is a tool that
> > is currently undergoing development. These changes seek to improve build
> > times.
> >
> > This change to entry/syscall_32.c resulted in a preprocessed size of
> > entry/syscall_32.i from 64002 lines to 47986 lines (-25%) for the x86
> > defconfig.
> >
> > Signed-off-by: Tanzir Hasan <[email protected]>
> > ---
> > arch/x86/entry/syscall_32.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
> > index 8cfc9bc73e7f..66db11fe8a1c 100644
> > --- a/arch/x86/entry/syscall_32.c
> > +++ b/arch/x86/entry/syscall_32.c
> > @@ -4,7 +4,7 @@
> > #include <linux/linkage.h>
> > #include <linux/sys.h>
> > #include <linux/cache.h>
> > -#include <linux/syscalls.h>
> > +#include <linux/ptrace.h>
> > #include <asm/syscall.h>
>
> Really? What do we need linux/ptrace.h for? Because if it's
> struct pt_regs for the generated externs, we might as well have
> just said
> struct pt_regs;
> and that would be it.
>
> <digs around a bit>
>
> As the matter of fact, all you need out of those includes is this:
>
> struct pt_regs;
> typedef long (*sys_call_ptr_t)(const struct pt_regs *);
> extern const sys_call_ptr_t sys_call_table[];
> #if defined(CONFIG_X86_32)
> #define ia32_sys_call_table sys_call_table
> #else
> /*
> * These may not exist, but still put the prototypes in so we
> * can use IS_ENABLED().
> */
> extern const sys_call_ptr_t ia32_sys_call_table[];
> extern const sys_call_ptr_t x32_sys_call_table[];
> #endif
I see that only pt_regs is necessary and I understand this approach.
I was wondering if using this approach would reduce readability.
Once we add the snippet, the file builds even after removing the
following lines:
#include <linux/linkage.h>
#include <linux/sys.h>
#include <linux/cache.h>
#include <linux/ptrace.h>
#include <asm/syscall.h>
It is possible to remove them all, but do you think any should be kept?
Best,
Tanzir
On Wed, Dec 27, 2023 at 03:50:33PM -0800, H. Peter Anvin wrote:
> > /*
> > * Only the low 32 bits of orig_ax are meaningful, so we return int.
> > * This importantly ignores the high bits on 64-bit, so comparisons
>
> __visible is for LTO, no?
If we need it in cases when array defined in entry/syscall_32.c and
used in entry/common.c, I would respectfully suggest that whatever
we need it for is misguided garbage. I don't think that LTO does
need it, though. How is arch/x86/entry/{syscall_32,common}.c
different from e.g. fs/{namespace,d_path}.c, where we have
fs/namespace.c:100:__cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
and
fs/d_path.c:166: read_seqbegin_or_lock(&mount_lock, &m_seq);
respectively?
On Wed, Dec 27, 2023 at 11:34:44PM +0000, Al Viro wrote:
> That's _it_. The same goes for syscall_64.c and syscall_x32.c.
> Oh, and lose the __visible/asmlinkage junk in there - none of that
> stuff is used from asm these days. See the patch below -
> Untested But Should Work(tm):
Unfortunately, there's this in kernel/trace/trace_syscalls.c:
unsigned long __init __weak arch_syscall_addr(int nr)
{
return (unsigned long)sys_call_table[nr];
}
How is that supposed to work for anything biarch? Including
amd64 with CONFIG_COMPAT enabled?
Confused...
On December 27, 2023 4:26:14 PM PST, Al Viro <[email protected]> wrote:
>On Wed, Dec 27, 2023 at 03:50:33PM -0800, H. Peter Anvin wrote:
>> > /*
>> > * Only the low 32 bits of orig_ax are meaningful, so we return int.
>> > * This importantly ignores the high bits on 64-bit, so comparisons
>>
>> __visible is for LTO, no?
>
>If we need it in cases when array defined in entry/syscall_32.c and
>used in entry/common.c, I would respectfully suggest that whatever
>we need it for is misguided garbage. I don't think that LTO does
>need it, though. How is arch/x86/entry/{syscall_32,common}.c
>different from e.g. fs/{namespace,d_path}.c, where we have
>fs/namespace.c:100:__cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
>and
>fs/d_path.c:166: read_seqbegin_or_lock(&mount_lock, &m_seq);
>respectively?
You're correct of course; __visible for LTO is for functions called from assembly *at the point of definition*, not declaration and certainly not as a type.
On Wed, Dec 27, 2023 at 4:45 PM Al Viro <[email protected]> wrote:
>
> On Wed, Dec 27, 2023 at 11:34:44PM +0000, Al Viro wrote:
>
> > That's _it_. The same goes for syscall_64.c and syscall_x32.c.
> > Oh, and lose the __visible/asmlinkage junk in there - none of that
> > stuff is used from asm these days. See the patch below -
> > Untested But Should Work(tm):
>
> Unfortunately, there's this in kernel/trace/trace_syscalls.c:
>
> unsigned long __init __weak arch_syscall_addr(int nr)
> {
> return (unsigned long)sys_call_table[nr];
> }
>
> How is that supposed to work for anything biarch? Including
> amd64 with CONFIG_COMPAT enabled?
commit f431b634f24d ("tracing/syscalls: Allow archs to ignore tracing
compat syscalls")
added a comment block about ARCH_TRACE_IGNORE_COMPAT_SYSCALLS, which
is defined for x86 in arch/x86/include/asm/ftrace.h.
The implementation of arch_syscall_addr for mips is quite complex;
dependent on quite a few different configs.
--
Thanks,
~Nick Desaulniers