2020-12-28 02:42:01

by Hans-Peter Nilsson

[permalink] [raw]
Subject: [PATCH] perf arm64: Fix mksyscalltbl, don't lose syscalls due to sort -nu

When using "sort -nu", arm64 syscalls were lost. That is, the
io_setup syscall (number 0) and all but one (typically
ftruncate; 64) of the syscalls that are defined symbolically
(like "#define __NR_ftruncate __NR3264_ftruncate") at the point
where "sort" is applied.

This creation-of-syscalls.c-scheme is, judging from comments,
copy-pasted from powerpc, and worked there because at the time,
its tools/arch/powerpc/include/uapi/asm/unistd.h had *literals*,
like "#define __NR_ftruncate 93".

With sort being numeric and the non-numeric key effectively
evaluating to 0, the sort option "-u" means these "duplicates"
are removed. There's no need to remove syscall lines with
duplicate numbers for arm64 because there are none, so let's fix
that by just losing the "-u". Having the table numerically
sorted on syscall-number for the rest of the syscalls looks
nice, so keep the "-n".

Signed-off-by: Hans-Peter Nilsson <[email protected]>
Cc: John Garry <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Kim Phillips <[email protected]>
---
tools/perf/arch/arm64/entry/syscalls/mksyscalltbl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
index 459469b7222c..a7ca48d1e37b 100755
--- a/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
+++ b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
@@ -58,5 +58,5 @@ create_table()

$gcc -E -dM -x c -I $incpath/include/uapi $input \
|sed -ne 's/^#define __NR_//p' \
- |sort -t' ' -k2 -nu \
+ |sort -t' ' -k2 -n \
|create_table
--
2.11.0

brgds, H-P


2020-12-29 03:15:19

by Leo Yan

[permalink] [raw]
Subject: Re: [PATCH] perf arm64: Fix mksyscalltbl, don't lose syscalls due to sort -nu

Hi Hans-Peter,

On Mon, Dec 28, 2020 at 03:39:41AM +0100, Hans-Peter Nilsson wrote:
> When using "sort -nu", arm64 syscalls were lost. That is, the
> io_setup syscall (number 0) and all but one (typically
> ftruncate; 64) of the syscalls that are defined symbolically
> (like "#define __NR_ftruncate __NR3264_ftruncate") at the point
> where "sort" is applied.
>
> This creation-of-syscalls.c-scheme is, judging from comments,
> copy-pasted from powerpc, and worked there because at the time,
> its tools/arch/powerpc/include/uapi/asm/unistd.h had *literals*,
> like "#define __NR_ftruncate 93".
>
> With sort being numeric and the non-numeric key effectively
> evaluating to 0, the sort option "-u" means these "duplicates"
> are removed. There's no need to remove syscall lines with
> duplicate numbers for arm64 because there are none, so let's fix
> that by just losing the "-u". Having the table numerically
> sorted on syscall-number for the rest of the syscalls looks
> nice, so keep the "-n".
>
> Signed-off-by: Hans-Peter Nilsson <[email protected]>

Very good catching! I tested this patch with the commands:

$ cd $LINUX_KERN
$ tools/perf/arch/arm64/entry/syscalls/mksyscalltbl \
$ARM64_TOOLCHAIN_PATH/aarch64-linux-gnu-gcc \
gcc tools tools/include/uapi/asm-generic/unistd.h

It gives out complete syscall tables:

$ diff /tmp/mksyscall_before.txt /tmp/mksyscall_after.txt
1a2,4
> [223] = "fadvise64",
> [25] = "fcntl",
> [44] = "fstatfs",
2a6,11
> [0] = "io_setup",
> [62] = "lseek",
> [222] = "mmap",
> [71] = "sendfile",
> [43] = "statfs",
> [45] = "truncate",

Rather than dropping option "-u" for sort command, I googled and read
the manual of "sort", but cannot find other better method. So this
patch looks good for me:

Reviewed-by: Leo Yan <[email protected]>
Tested-by: Leo Yan <[email protected]>


> Cc: John Garry <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Kim Phillips <[email protected]>
> ---
> tools/perf/arch/arm64/entry/syscalls/mksyscalltbl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
> index 459469b7222c..a7ca48d1e37b 100755
> --- a/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
> +++ b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
> @@ -58,5 +58,5 @@ create_table()
>
> $gcc -E -dM -x c -I $incpath/include/uapi $input \
> |sed -ne 's/^#define __NR_//p' \
> - |sort -t' ' -k2 -nu \
> + |sort -t' ' -k2 -n \
> |create_table
> --
> 2.11.0
>
> brgds, H-P

2022-11-25 12:06:35

by Vincent Whitchurch

[permalink] [raw]
Subject: Re: [PATCH] perf arm64: Fix mksyscalltbl, don't lose syscalls due to sort -nu

On Tue, Dec 29, 2020 at 11:09:33AM +0800, Leo Yan wrote:
> On Mon, Dec 28, 2020 at 03:39:41AM +0100, Hans-Peter Nilsson wrote:
> > When using "sort -nu", arm64 syscalls were lost. That is, the
> > io_setup syscall (number 0) and all but one (typically
> > ftruncate; 64) of the syscalls that are defined symbolically
> > (like "#define __NR_ftruncate __NR3264_ftruncate") at the point
> > where "sort" is applied.
> >
> > This creation-of-syscalls.c-scheme is, judging from comments,
> > copy-pasted from powerpc, and worked there because at the time,
> > its tools/arch/powerpc/include/uapi/asm/unistd.h had *literals*,
> > like "#define __NR_ftruncate 93".
> >
> > With sort being numeric and the non-numeric key effectively
> > evaluating to 0, the sort option "-u" means these "duplicates"
> > are removed. There's no need to remove syscall lines with
> > duplicate numbers for arm64 because there are none, so let's fix
> > that by just losing the "-u". Having the table numerically
> > sorted on syscall-number for the rest of the syscalls looks
> > nice, so keep the "-n".
> >
> > Signed-off-by: Hans-Peter Nilsson <[email protected]>
>
> Very good catching! I tested this patch with the commands:
>
> $ cd $LINUX_KERN
> $ tools/perf/arch/arm64/entry/syscalls/mksyscalltbl \
> $ARM64_TOOLCHAIN_PATH/aarch64-linux-gnu-gcc \
> gcc tools tools/include/uapi/asm-generic/unistd.h
>
> It gives out complete syscall tables:
>
> $ diff /tmp/mksyscall_before.txt /tmp/mksyscall_after.txt
> 1a2,4
> > [223] = "fadvise64",
> > [25] = "fcntl",
> > [44] = "fstatfs",
> 2a6,11
> > [0] = "io_setup",
> > [62] = "lseek",
> > [222] = "mmap",
> > [71] = "sendfile",
> > [43] = "statfs",
> > [45] = "truncate",
>
> Rather than dropping option "-u" for sort command, I googled and read
> the manual of "sort", but cannot find other better method. So this
> patch looks good for me:
>
> Reviewed-by: Leo Yan <[email protected]>
> Tested-by: Leo Yan <[email protected]>

It looks like this patch was never applied? AFAICS it is still needed
on current HEAD and it still applies cleanly.

2022-11-25 13:07:33

by Leo Yan

[permalink] [raw]
Subject: Re: [PATCH] perf arm64: Fix mksyscalltbl, don't lose syscalls due to sort -nu

Hi Vincent,

[ + Arnd ]

On Fri, Nov 25, 2022 at 12:53:10PM +0100, Vincent Whitchurch wrote:
> On Tue, Dec 29, 2020 at 11:09:33AM +0800, Leo Yan wrote:
> > On Mon, Dec 28, 2020 at 03:39:41AM +0100, Hans-Peter Nilsson wrote:
> > > When using "sort -nu", arm64 syscalls were lost. That is, the
> > > io_setup syscall (number 0) and all but one (typically
> > > ftruncate; 64) of the syscalls that are defined symbolically
> > > (like "#define __NR_ftruncate __NR3264_ftruncate") at the point
> > > where "sort" is applied.
> > >
> > > This creation-of-syscalls.c-scheme is, judging from comments,
> > > copy-pasted from powerpc, and worked there because at the time,
> > > its tools/arch/powerpc/include/uapi/asm/unistd.h had *literals*,
> > > like "#define __NR_ftruncate 93".
> > >
> > > With sort being numeric and the non-numeric key effectively
> > > evaluating to 0, the sort option "-u" means these "duplicates"
> > > are removed. There's no need to remove syscall lines with
> > > duplicate numbers for arm64 because there are none, so let's fix
> > > that by just losing the "-u". Having the table numerically
> > > sorted on syscall-number for the rest of the syscalls looks
> > > nice, so keep the "-n".
> > >
> > > Signed-off-by: Hans-Peter Nilsson <[email protected]>
> >
> > Very good catching! I tested this patch with the commands:
> >
> > $ cd $LINUX_KERN
> > $ tools/perf/arch/arm64/entry/syscalls/mksyscalltbl \
> > $ARM64_TOOLCHAIN_PATH/aarch64-linux-gnu-gcc \
> > gcc tools tools/include/uapi/asm-generic/unistd.h
> >
> > It gives out complete syscall tables:
> >
> > $ diff /tmp/mksyscall_before.txt /tmp/mksyscall_after.txt
> > 1a2,4
> > > [223] = "fadvise64",
> > > [25] = "fcntl",
> > > [44] = "fstatfs",
> > 2a6,11
> > > [0] = "io_setup",
> > > [62] = "lseek",
> > > [222] = "mmap",
> > > [71] = "sendfile",
> > > [43] = "statfs",
> > > [45] = "truncate",
> >
> > Rather than dropping option "-u" for sort command, I googled and read
> > the manual of "sort", but cannot find other better method. So this
> > patch looks good for me:
> >
> > Reviewed-by: Leo Yan <[email protected]>
> > Tested-by: Leo Yan <[email protected]>
>
> It looks like this patch was never applied? AFAICS it is still needed
> on current HEAD and it still applies cleanly.

Thanks a lot for bringing up this.

Before there have a discussion [1] for refactoring Arm64 system call
table but it didn't really happen. I think it's the right thing to merge
this patch, @Arnaldo, could you pick up this patch?

Thanks,
Leo

[1] https://lore.kernel.org/lkml/[email protected]om/

2022-11-25 14:28:06

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] perf arm64: Fix mksyscalltbl, don't lose syscalls due to sort -nu

On Fri, Nov 25, 2022, at 13:54, Leo Yan wrote:
> On Fri, Nov 25, 2022 at 12:53:10PM +0100, Vincent Whitchurch wrote:

>> It looks like this patch was never applied? AFAICS it is still needed
>> on current HEAD and it still applies cleanly.
>
> Thanks a lot for bringing up this.
>
> Before there have a discussion [1] for refactoring Arm64 system call
> table but it didn't really happen.

I actually worked on this last week and did a new series to convert
the old asm-generic/unistd.h header into the syscall.tbl format,
and change arm64 to use that.

You can find my work in the 'syscall-tbl' branch of my asm-generic
tree [1]. This has only seen light build testing so far, and is
probably still buggy, but most of the work is there. The missing
bits are the Makefiles for the other seven architectures using
asm-generic/unistd.h, and checking the output to ensure the
contents are still the same.

> I think it's the right thing to merge
> this patch, @Arnaldo, could you pick up this patch?

That sounds fine to me.

Arnd

[1] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git/

2022-12-02 19:48:18

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH] perf arm64: Fix mksyscalltbl, don't lose syscalls due to sort -nu

Em Fri, Nov 25, 2022 at 02:56:31PM +0100, Arnd Bergmann escreveu:
> On Fri, Nov 25, 2022, at 13:54, Leo Yan wrote:
> > On Fri, Nov 25, 2022 at 12:53:10PM +0100, Vincent Whitchurch wrote:
>
> >> It looks like this patch was never applied? AFAICS it is still needed
> >> on current HEAD and it still applies cleanly.
> >
> > Thanks a lot for bringing up this.
> >
> > Before there have a discussion [1] for refactoring Arm64 system call
> > table but it didn't really happen.
>
> I actually worked on this last week and did a new series to convert
> the old asm-generic/unistd.h header into the syscall.tbl format,
> and change arm64 to use that.
>
> You can find my work in the 'syscall-tbl' branch of my asm-generic
> tree [1]. This has only seen light build testing so far, and is
> probably still buggy, but most of the work is there. The missing
> bits are the Makefiles for the other seven architectures using
> asm-generic/unistd.h, and checking the output to ensure the
> contents are still the same.
>
> > I think it's the right thing to merge
> > this patch, @Arnaldo, could you pick up this patch?
>
> That sounds fine to me.

Sure, and adding an Acked-by: Arnd

> Arnd
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git/

--

- Arnaldo