| Message ID | 1313529267-4428-1-git-send-email-wad@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 16 August 2011 16:14:26 Will Drewry wrote: > > asm-exports.c is added instead of reusing asm-offsets.c to avoid a > variety of collisions (VM_EXEC, DMA_*, etc). It is possible to use the > same calls.S mechanism but add NR_syscalls to asm-offsets.c. However, > at inclusion time for generated/asm-offsets.h, conflicting defines will > need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose > of asm-offsets.h is to safely bind C language definitions to assembly > and not the reverse. > > - Is this approach palatable? > - Should I resend only when paired with the other ftrace-needed patches? This seems overly complex, compared to a one-line change adding the symbol to asm/unistd.h. The only other architecture that uses an approach like the one you have posted is x86-64, and it's simpler there because it can easily be done in asm-offsets.c there without the need to create another helper. Arnd
On Tue, Aug 16, 2011 at 4:21 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 16 August 2011 16:14:26 Will Drewry wrote: >> >> asm-exports.c is added instead of reusing asm-offsets.c to avoid a >> variety of collisions (VM_EXEC, DMA_*, etc). It is possible to use the >> same calls.S mechanism but add NR_syscalls to asm-offsets.c. However, >> at inclusion time for generated/asm-offsets.h, conflicting defines will >> need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose >> of asm-offsets.h is to safely bind C language definitions to assembly >> and not the reverse. >> >> - Is this approach palatable? >> - Should I resend only when paired with the other ftrace-needed patches? > > This seems overly complex, compared to a one-line change adding the symbol > to asm/unistd.h. The only other architecture that uses an approach > like the one you have posted is x86-64, and it's simpler there > because it can easily be done in asm-offsets.c there without the need > to create another helper. Agreed! I proposed this approach based solely on prior threads I've seen. E.g., - https://lkml.org/lkml/2007/6/1/427 (don't just #define) - https://lkml.org/lkml/2009/8/27/280 (todo: x86-32 to move to x86-64) If a single line #define is good enough, then it certainly works for me. thanks! will
Will Drewry writes: > On Tue, Aug 16, 2011 at 4:21 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 16 August 2011 16:14:26 Will Drewry wrote: > >> > >> asm-exports.c is added instead of reusing asm-offsets.c to avoid a > >> variety of collisions (VM_EXEC, DMA_*, etc). It is possible to use the > >> same calls.S mechanism but add NR_syscalls to asm-offsets.c. However, > >> at inclusion time for generated/asm-offsets.h, conflicting defines will > >> need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose > >> of asm-offsets.h is to safely bind C language definitions to assembly > >> and not the reverse. > >> > >> - Is this approach palatable? > >> - Should I resend only when paired with the other ftrace-needed patches? > > > > This seems overly complex, compared to a one-line change adding the symbol > > to asm/unistd.h. The only other architecture that uses an approach > > like the one you have posted is x86-64, and it's simpler there > > because it can easily be done in asm-offsets.c there without the need > > to create another helper. > > Agreed! > > I proposed this approach based solely on prior threads I've seen. E.g., > - https://lkml.org/lkml/2007/6/1/427 > (don't just #define) > - https://lkml.org/lkml/2009/8/27/280 > (todo: x86-32 to move to x86-64) > > If a single line #define is good enough, then it certainly works for me. Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate, if not entirely elegant, solution. Adding asm-export.c just for this is waaay overkill. /Mikael
On Wednesday 17 August 2011, Mikael Pettersson wrote: > > I proposed this approach based solely on prior threads I've seen. E.g., > > - https://lkml.org/lkml/2007/6/1/427 > > (don't just #define) > > - https://lkml.org/lkml/2009/8/27/280 > > (todo: x86-32 to move to x86-64) > > > > If a single line #define is good enough, then it certainly works for me. > > Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate, > if not entirely elegant, solution. Adding asm-export.c just for this is > waaay overkill. Right. While the main problem with having the constant in asm/unistd.h (needs to be kept in sync when adding new syscalls) is an annoyance, the suggested approach is adding more complexity than necessary. If you want to have the value automatically computed, I'd suggest moving the format of unistd.h over to a method like the one used by x86-64 and asm-generic, which is to combine the syscall number definitions with the list of syscall pointers that currently reside in arch/arm/kernel/calls.S, for the added benefit that it's easier to keep the two in sync as well. The main question is what Russell's preference is on the alternatives. Arnd
On Wed, Aug 17, 2011 at 11:22:48AM +0200, Arnd Bergmann wrote: > On Wednesday 17 August 2011, Mikael Pettersson wrote: > > > I proposed this approach based solely on prior threads I've seen. E.g., > > > - https://lkml.org/lkml/2007/6/1/427 > > > (don't just #define) > > > - https://lkml.org/lkml/2009/8/27/280 > > > (todo: x86-32 to move to x86-64) > > > > > > If a single line #define is good enough, then it certainly works for me. > > > > Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate, > > if not entirely elegant, solution. Adding asm-export.c just for this is > > waaay overkill. > > Right. While the main problem with having the constant in asm/unistd.h > (needs to be kept in sync when adding new syscalls) is an annoyance, > the suggested approach is adding more complexity than necessary. > > If you want to have the value automatically computed, I'd suggest > moving the format of unistd.h over to a method like the one used > by x86-64 and asm-generic, which is to combine the syscall number > definitions with the list of syscall pointers that currently reside > in arch/arm/kernel/calls.S, for the added benefit that it's easier to > keep the two in sync as well. You obviously haven't looked at calls.S - the table has multiple options depending on whether its being used for EABI or OABI. It's not purely a 1:1 mapping between syscall number name and function name. Adding an additional parameter to the CALL() macro to get around that for the syscall number name starts making the file unweidly, especially if we obey the 80 column limit. Finally, the assembly 'number of syscalls' is different from the real number of syscalls to ensure that we don't overflow the 8-bit constant limit for the compare instruction. Whether that needs to be included in the C __NR_syscalls or not depends on how its used. I personally would prefer C code not to rely on the (unprovided) NR_syscalls due to these kinds of issues. Finally, if its just for ftrace, well I don't have a high regard for that code. It's something I hardly ever use and when I have tried to use it it has been soo dire in terms of overheads that it's just not worth bothering with. When I want timings out of the kernel, I have always (and continue to do so) implement my own gathering mechanisms rather than using the ftrace crap.
On Sun, Aug 21, 2011 at 4:07 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Aug 17, 2011 at 11:22:48AM +0200, Arnd Bergmann wrote: >> On Wednesday 17 August 2011, Mikael Pettersson wrote: >> > > I proposed this approach based solely on prior threads I've seen. E.g., >> > > - https://lkml.org/lkml/2007/6/1/427 >> > > (don't just #define) >> > > - https://lkml.org/lkml/2009/8/27/280 >> > > (todo: x86-32 to move to x86-64) >> > > >> > > If a single line #define is good enough, then it certainly works for me. >> > >> > Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate, >> > if not entirely elegant, solution. Adding asm-export.c just for this is >> > waaay overkill. >> >> Right. While the main problem with having the constant in asm/unistd.h >> (needs to be kept in sync when adding new syscalls) is an annoyance, >> the suggested approach is adding more complexity than necessary. >> >> If you want to have the value automatically computed, I'd suggest >> moving the format of unistd.h over to a method like the one used >> by x86-64 and asm-generic, which is to combine the syscall number >> definitions with the list of syscall pointers that currently reside >> in arch/arm/kernel/calls.S, for the added benefit that it's easier to >> keep the two in sync as well. > > You obviously haven't looked at calls.S - the table has multiple > options depending on whether its being used for EABI or OABI. It's > not purely a 1:1 mapping between syscall number name and function > name. > > Adding an additional parameter to the CALL() macro to get around that > for the syscall number name starts making the file unweidly, especially > if we obey the 80 column limit. > > Finally, the assembly 'number of syscalls' is different from the real > number of syscalls to ensure that we don't overflow the 8-bit constant > limit for the compare instruction. Whether that needs to be included > in the C __NR_syscalls or not depends on how its used. > > I personally would prefer C code not to rely on the (unprovided) > NR_syscalls due to these kinds of issues. Upon continued inspection, I largely agree.I have some out-of-tree code (hopefully not forever) which uses ftrace. It followed its coding approach which statically allocates arrays based on NR_syscalls. While it makes lookups simple, it creates more infrastructure headaches for arches that have a non-zero syscall base and/or differenting numberings based on the active ABI. I've since changed my approach to one that works for all architectures and doesn't require knowing NR_syscalls at compiled-time. > Finally, if its just for ftrace, well I don't have a high regard for that > code. It's something I hardly ever use and when I have tried to use it > it has been soo dire in terms of overheads that it's just not worth > bothering with. When I want timings out of the kernel, I have always > (and continue to do so) implement my own gathering mechanisms rather > than using the ftrace crap. My personal interest is in reusing the shared filter engine along with its awareness of system call metadata. That aside, I have a sneaking suspicion ftrace_syscalls will move away from the current NR_syscalls-based approach in order to support more architecture flavors, but I have no idea for sure. (I'll be sending patches at some point.) thanks for your thoughts and consideration! will
diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 70c424e..62abc9a 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -270,8 +270,9 @@ all: $(KBUILD_IMAGE) boot := arch/arm/boot -archprepare: +archprepare: Kbuild $(Q)$(MAKE) $(build)=arch/arm/tools include/generated/mach-types.h + $(Q)$(MAKE) $(build)=arch/arm/kernel -f $(src)/arch/arm/kernel/Makefile.exports include/generated/asm-exports.h # Convert bzImage to zImage bzImage: zImage diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h index 2c04ed5..77d4928 100644 --- a/arch/arm/include/asm/unistd.h +++ b/arch/arm/include/asm/unistd.h @@ -481,5 +481,10 @@ #define __IGNORE_fadvise64_64 1 #define __IGNORE_migrate_pages 1 +#if !defined(COMPILER_OFFSETS) && !defined(__ASSEMBLY__) +#include <generated/asm-exports.h> +#define NR_syscalls (__NR_syscall_max + 1) +#endif + #endif /* __KERNEL__ */ #endif /* __ASM_ARM_UNISTD_H */ diff --git a/arch/arm/kernel/Makefile.exports b/arch/arm/kernel/Makefile.exports new file mode 100644 index 0000000..5ec6553 --- /dev/null +++ b/arch/arm/kernel/Makefile.exports @@ -0,0 +1,42 @@ +# +# linux/arch/arm/kernel/Makefile.exports +# +# Copyright (C) 2011 Chromium OS Authors <chromium-os-dev@chromium.org> +# +# Generate asm-exports.h based on Kbuild's asm-offsets.h +# + +exports-file := include/generated/asm-exports.h + +# Default sed regexp - multiline due to syntax constraints +define sed-y + "/^->/{s:->#\(.*\):/* \1 */:; \ + s:^->\([^ ]*\) [\$$#]*\([-0-9]*\) \(.*\):#define \1 \2 /* \3 */:; \ + s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; \ + s:->::; p;}" +endef + +quiet_cmd_exports = GEN $@ +define cmd_exports + (set -e; \ + echo "#ifndef __ASM_EXPORTS_H__"; \ + echo "#define __ASM_EXPORTS_H__"; \ + echo "/*"; \ + echo " * DO NOT MODIFY."; \ + echo " *"; \ + echo " * This file was generated by arch/arm/tools/Makefile"; \ + echo " *"; \ + echo " */"; \ + echo ""; \ + sed -ne $(sed-y) $<; \ + echo ""; \ + echo "#endif" ) > $@ +endef + +# We use internal kbuild rules to avoid the "is up to date" message from make +$(src)/../asm-exports.s: $(src)/../kernel/asm-exports.c FORCE + $(Q)mkdir -p $(dir $@) + $(call if_changed_dep,cc_s_c) + +$(exports-file): $(src)/../kernel/asm-exports.s Kbuild + $(call cmd,exports) diff --git a/arch/arm/kernel/asm-exports.c b/arch/arm/kernel/asm-exports.c new file mode 100644 index 0000000..a2aec12 --- /dev/null +++ b/arch/arm/kernel/asm-exports.c @@ -0,0 +1,36 @@ +/* + * Copyright (C) 2011 Chromium OS Authors <chromium-os-dev@chromium.org> + * + * Generate definitions needed by C language modules from data resident + * in assembly language modules. Nothing exported here should be supplied + * by any C language headers in common use. + * + * This code generates raw asm output which is post-processed to extract + * and format the required data. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#define COMPILE_OFFSETS +#include <linux/kbuild.h> + +/* + * Make sure that the compiler and target are compatible. + */ +#if defined(__APCS_26__) +#error Sorry, your compiler targets APCS-26 but this kernel requires APCS-32 +#endif + +#define CALL(x) 1 + +#define syscalls_counted +#define NO_PAD_SYSCALLS 1 +static const long syscall_count = + #include "calls.S" + 0; + +int main(void) +{ + DEFINE(__NR_syscall_max, syscall_count - 1); + return 0; +} diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S index 80f7896..02cdece 100644 --- a/arch/arm/kernel/calls.S +++ b/arch/arm/kernel/calls.S @@ -385,6 +385,7 @@ CALL(sys_syncfs) CALL(sys_sendmmsg) /* 375 */ CALL(sys_setns) +#ifndef NO_PAD_SYSCALLS #ifndef syscalls_counted .equ syscalls_padding, ((NR_syscalls + 3) & ~3) - NR_syscalls #define syscalls_counted @@ -392,3 +393,4 @@ .rept syscalls_padding CALL(sys_ni_syscall) .endr +#endif
At present, there is no way to determine, at compile-time, the number of system calls for arm. This change provides NR_syscalls via unistd.h much like other arches. Historically, there hasn't been a need to pull NR_syscalls into unistd.h for ARM. Prior lkml threads indicate as much. However, NR_syscalls is a prerequisite for enabling CONFIG_FTRACE_SYSCALLS on the platform (along with asm/syscall.h population and arch_syscall_addr). This patch is similar to x86-64 in that the system call count is established using the Kbuild assembly generation trick to extract constant values. It adds three files: - arch/arm/kernel/asm-exports.c - arch/arm/kernel/Makefile.exports The C file provides the location to export assembly resident values, like the number of syscalls. The makefile mimics Kbuild for extracting the values from the partially compiled file. Also included are simple fixups to make calls.S safe to #include in asm-exports.c and exporting of asm-exports.h in unistd.h. In particular, this change yields an NR_syscalls value that matches the number of allocated calls via calls.S and not the aligned, padded syscall table size needed for optimal assembly. asm-exports.c is added instead of reusing asm-offsets.c to avoid a variety of collisions (VM_EXEC, DMA_*, etc). It is possible to use the same calls.S mechanism but add NR_syscalls to asm-offsets.c. However, at inclusion time for generated/asm-offsets.h, conflicting defines will need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose of asm-offsets.h is to safely bind C language definitions to assembly and not the reverse. - Is this approach palatable? - Should I resend only when paired with the other ftrace-needed patches? Thanks! Signed-off-by: Will Drewry <wad@chromium.org> --- arch/arm/Makefile | 3 +- arch/arm/include/asm/unistd.h | 5 ++++ arch/arm/kernel/Makefile.exports | 42 ++++++++++++++++++++++++++++++++++++++ arch/arm/kernel/asm-exports.c | 36 ++++++++++++++++++++++++++++++++ arch/arm/kernel/calls.S | 2 + 5 files changed, 87 insertions(+), 1 deletions(-) create mode 100644 arch/arm/kernel/Makefile.exports create mode 100644 arch/arm/kernel/asm-exports.c