* [PATCH v2 01/11] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2023-01-11 21:27 ` Carlos O'Donell
2022-11-04 19:01 ` [PATCH v2 02/11] linux: Extend internal clone3 documentation Adhemerval Zanella
` (10 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
There is no need to issue another sigaction is the disposition is
already SIG_DFL.
Checked on x86_64-linux-gnu.
---
sysdeps/unix/sysv/linux/spawni.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index ee843a2247..65ee03c804 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -129,7 +129,7 @@ __spawni_child (void *arguments)
else
{
__libc_sigaction (sig, 0, &sa);
- if (sa.sa_handler == SIG_IGN)
+ if (sa.sa_handler == SIG_IGN || sa.sa_handler == SIG_DFL)
continue;
sa.sa_handler = SIG_DFL;
}
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 01/11] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL
2022-11-04 19:01 ` [PATCH v2 01/11] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL Adhemerval Zanella
@ 2023-01-11 21:27 ` Carlos O'Donell
0 siblings, 0 replies; 22+ messages in thread
From: Carlos O'Donell @ 2023-01-11 21:27 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha, Szabolcs Nagy, H . J . Lu
Cc: Christian Brauner
On 11/4/22 15:01, Adhemerval Zanella via Libc-alpha wrote:
> There is no need to issue another sigaction is the disposition is
> already SIG_DFL.
>
> Checked on x86_64-linux-gnu.
Changes requested per original v2. See other review. Please post v4.
> ---
> sysdeps/unix/sysv/linux/spawni.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index ee843a2247..65ee03c804 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -129,7 +129,7 @@ __spawni_child (void *arguments)
> else
> {
> __libc_sigaction (sig, 0, &sa);
> - if (sa.sa_handler == SIG_IGN)
> + if (sa.sa_handler == SIG_IGN || sa.sa_handler == SIG_DFL)
> continue;
> sa.sa_handler = SIG_DFL;
> }
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 02/11] linux: Extend internal clone3 documentation
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
2022-11-04 19:01 ` [PATCH v2 01/11] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2022-11-07 16:40 ` Szabolcs Nagy
2023-01-11 21:35 ` Carlos O'Donell
2022-11-04 19:01 ` [PATCH v2 03/11] Linux: Do not align the stack for __clone3 Adhemerval Zanella
` (9 subsequent siblings)
11 siblings, 2 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
Different than kernel, clone3 returns EINVAL for NULL struct
clone_args or function pointer. This is similar to clone
interface that return EINVAL for NULL function argument.
It also clean up the Linux clone3.h interface, since it not
currently exported.
Checked on x86_64-linux-gnu.
---
include/clone_internal.h | 24 +++++++++++++++++++-----
sysdeps/unix/sysv/linux/clone3.h | 10 +---------
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/include/clone_internal.h b/include/clone_internal.h
index 4b23ef33ce..73b8114df4 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -1,10 +1,24 @@
-#ifndef _CLONE3_H
-#include_next <clone3.h>
+#ifndef _CLONE_INTERNAL_H
+#define _CLONE_INTERNAL_H
-extern __typeof (clone3) __clone3;
+#include <clone3.h>
-/* The internal wrapper of clone/clone2 and clone3. If __clone3 returns
- -1 with ENOSYS, fall back to clone or clone2. */
+/* The clone3 syscall provides a superset of the functionality of the clone
+ interface. The kernel might extend __CL_ARGS struct in the future, with
+ each version with a diffent __SIZE. If the child is created, it will
+ start __FUNC function with __ARG arguments.
+
+ Different than kernel, the implementation also returns EINVAL for an
+ invalid NULL __CL_ARGS or __FUNC (similar to __clone).
+
+ This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
+*/
+extern int __clone3 (struct clone_args *__cl_args, size_t __size,
+ int (*__func) (void *__arg), void *__arg);
+
+/* The internal wrapper of clone/clone2 and clone3. Different than __clone3,
+ it will align the stack if required. If __clone3 returns -1 with ENOSYS,
+ fall back to clone or clone2. */
extern int __clone_internal (struct clone_args *__cl_args,
int (*__func) (void *__arg), void *__arg);
diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
index 889014a6a9..7c7e2cdcb0 100644
--- a/sysdeps/unix/sysv/linux/clone3.h
+++ b/sysdeps/unix/sysv/linux/clone3.h
@@ -1,4 +1,4 @@
-/* The wrapper of clone3.
+/* The clone3 kernel interface definitions.
Copyright (C) 2021-2022 Free Software Foundation, Inc.
This file is part of the GNU C Library.
@@ -23,8 +23,6 @@
#include <stddef.h>
#include <bits/types.h>
-__BEGIN_DECLS
-
/* The unsigned 64-bit and 8-byte aligned integer type. */
typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
@@ -58,10 +56,4 @@ struct clone_args
__aligned_uint64_t cgroup;
};
-/* The wrapper of clone3. */
-extern int clone3 (struct clone_args *__cl_args, size_t __size,
- int (*__func) (void *__arg), void *__arg);
-
-__END_DECLS
-
#endif /* clone3.h */
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 02/11] linux: Extend internal clone3 documentation
2022-11-04 19:01 ` [PATCH v2 02/11] linux: Extend internal clone3 documentation Adhemerval Zanella
@ 2022-11-07 16:40 ` Szabolcs Nagy
2023-01-11 21:35 ` Carlos O'Donell
1 sibling, 0 replies; 22+ messages in thread
From: Szabolcs Nagy @ 2022-11-07 16:40 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, H . J . Lu, Christian Brauner
The 11/04/2022 16:01, Adhemerval Zanella wrote:
> Different than kernel, clone3 returns EINVAL for NULL struct
> clone_args or function pointer. This is similar to clone
> interface that return EINVAL for NULL function argument.
>
> It also clean up the Linux clone3.h interface, since it not
> currently exported.
>
> Checked on x86_64-linux-gnu.
this looks reasonable.
Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
> ---
> include/clone_internal.h | 24 +++++++++++++++++++-----
> sysdeps/unix/sysv/linux/clone3.h | 10 +---------
> 2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 4b23ef33ce..73b8114df4 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -1,10 +1,24 @@
> -#ifndef _CLONE3_H
> -#include_next <clone3.h>
> +#ifndef _CLONE_INTERNAL_H
> +#define _CLONE_INTERNAL_H
>
> -extern __typeof (clone3) __clone3;
> +#include <clone3.h>
>
> -/* The internal wrapper of clone/clone2 and clone3. If __clone3 returns
> - -1 with ENOSYS, fall back to clone or clone2. */
> +/* The clone3 syscall provides a superset of the functionality of the clone
> + interface. The kernel might extend __CL_ARGS struct in the future, with
> + each version with a diffent __SIZE. If the child is created, it will
> + start __FUNC function with __ARG arguments.
> +
> + Different than kernel, the implementation also returns EINVAL for an
> + invalid NULL __CL_ARGS or __FUNC (similar to __clone).
> +
> + This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
> +*/
> +extern int __clone3 (struct clone_args *__cl_args, size_t __size,
> + int (*__func) (void *__arg), void *__arg);
> +
> +/* The internal wrapper of clone/clone2 and clone3. Different than __clone3,
> + it will align the stack if required. If __clone3 returns -1 with ENOSYS,
> + fall back to clone or clone2. */
> extern int __clone_internal (struct clone_args *__cl_args,
> int (*__func) (void *__arg), void *__arg);
>
> diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> index 889014a6a9..7c7e2cdcb0 100644
> --- a/sysdeps/unix/sysv/linux/clone3.h
> +++ b/sysdeps/unix/sysv/linux/clone3.h
> @@ -1,4 +1,4 @@
> -/* The wrapper of clone3.
> +/* The clone3 kernel interface definitions.
> Copyright (C) 2021-2022 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
>
> @@ -23,8 +23,6 @@
> #include <stddef.h>
> #include <bits/types.h>
>
> -__BEGIN_DECLS
> -
> /* The unsigned 64-bit and 8-byte aligned integer type. */
> typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
>
> @@ -58,10 +56,4 @@ struct clone_args
> __aligned_uint64_t cgroup;
> };
>
> -/* The wrapper of clone3. */
> -extern int clone3 (struct clone_args *__cl_args, size_t __size,
> - int (*__func) (void *__arg), void *__arg);
> -
> -__END_DECLS
> -
> #endif /* clone3.h */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 02/11] linux: Extend internal clone3 documentation
2022-11-04 19:01 ` [PATCH v2 02/11] linux: Extend internal clone3 documentation Adhemerval Zanella
2022-11-07 16:40 ` Szabolcs Nagy
@ 2023-01-11 21:35 ` Carlos O'Donell
1 sibling, 0 replies; 22+ messages in thread
From: Carlos O'Donell @ 2023-01-11 21:35 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha, Szabolcs Nagy, H . J . Lu
Cc: Christian Brauner
On 11/4/22 15:01, Adhemerval Zanella via Libc-alpha wrote:
> Different than kernel, clone3 returns EINVAL for NULL struct
> clone_args or function pointer. This is similar to clone
> interface that return EINVAL for NULL function argument.
Please post v4.
Suggest:
Document that the internal __clone3 returns EINVAL for a NULL struct
clone_args or function pointer and that this differs from the kernel.
This behavior is similar to the clone interface that returns EINVAL
for a NULL function argument.
> It also clean up the Linux clone3.h interface, since it not
> currently exported.
Suggest:
Additionally cleanup the Linux clone3.h interface (not currently exported).
>
> Checked on x86_64-linux-gnu.
> ---
> include/clone_internal.h | 24 +++++++++++++++++++-----
> sysdeps/unix/sysv/linux/clone3.h | 10 +---------
> 2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 4b23ef33ce..73b8114df4 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -1,10 +1,24 @@
> -#ifndef _CLONE3_H
> -#include_next <clone3.h>
> +#ifndef _CLONE_INTERNAL_H
> +#define _CLONE_INTERNAL_H
>
> -extern __typeof (clone3) __clone3;
> +#include <clone3.h>
>
> -/* The internal wrapper of clone/clone2 and clone3. If __clone3 returns
> - -1 with ENOSYS, fall back to clone or clone2. */
> +/* The clone3 syscall provides a superset of the functionality of the clone
> + interface. The kernel might extend __CL_ARGS struct in the future, with
> + each version with a diffent __SIZE. If the child is created, it will
> + start __FUNC function with __ARG arguments.
OK.
> +
> + Different than kernel, the implementation also returns EINVAL for an
> + invalid NULL __CL_ARGS or __FUNC (similar to __clone).
Suggest:
Unlike the kernel the implementation returns EINVAL for an invalid NULL __CL_ARGS
or __FUNC (similar to __clone).
> +
This has nothing to do with the ABI in glibc so I'd avoid "ABI" here.
Suggest:
This function is only implemented if the machine defines HAVE_CLONE3_WRAPPER.
> + This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
> +*/
> +extern int __clone3 (struct clone_args *__cl_args, size_t __size,
> + int (*__func) (void *__arg), void *__arg);
> +
> +/* The internal wrapper of clone/clone2 and clone3. Different than __clone3,
> + it will align the stack if required. If __clone3 returns -1 with ENOSYS,
> + fall back to clone or clone2. */
> extern int __clone_internal (struct clone_args *__cl_args,
> int (*__func) (void *__arg), void *__arg);
>
> diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> index 889014a6a9..7c7e2cdcb0 100644
> --- a/sysdeps/unix/sysv/linux/clone3.h
> +++ b/sysdeps/unix/sysv/linux/clone3.h
> @@ -1,4 +1,4 @@
> -/* The wrapper of clone3.
> +/* The clone3 kernel interface definitions.
> Copyright (C) 2021-2022 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
>
> @@ -23,8 +23,6 @@
> #include <stddef.h>
> #include <bits/types.h>
>
> -__BEGIN_DECLS
> -
> /* The unsigned 64-bit and 8-byte aligned integer type. */
> typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
>
> @@ -58,10 +56,4 @@ struct clone_args
> __aligned_uint64_t cgroup;
> };
>
> -/* The wrapper of clone3. */
> -extern int clone3 (struct clone_args *__cl_args, size_t __size,
> - int (*__func) (void *__arg), void *__arg);
> -
> -__END_DECLS
> -
OK.
> #endif /* clone3.h */
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 03/11] Linux: Do not align the stack for __clone3
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
2022-11-04 19:01 ` [PATCH v2 01/11] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL Adhemerval Zanella
2022-11-04 19:01 ` [PATCH v2 02/11] linux: Extend internal clone3 documentation Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2023-01-11 21:43 ` Carlos O'Donell
2022-11-04 19:01 ` [PATCH v2 04/11] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn Adhemerval Zanella
` (8 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
Adjusting the stack on child thread is not async-signal safe, since
kernel does not enforce stack alignmnet. Although, all the internal
usage of __clone_internal are done with all signal masked, removing
stack alignment of __clone3 is still a net gain: is simplifies the
function contract detail (mask/unmarsk signals) and simplifies the
arch-specific code.
The internal users are responsible for correct stack alignment. If
glibc ever exports a clone3 wrapper, it should adjust stack alignment.
Checked on x86_64-linux-gnu.
---
include/clone_internal.h | 5 ++
.../sysv/linux/tst-misalign-clone-internal.c | 74 -------------------
sysdeps/unix/sysv/linux/x86_64/clone3.S | 3 -
3 files changed, 5 insertions(+), 77 deletions(-)
delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
diff --git a/include/clone_internal.h b/include/clone_internal.h
index 73b8114df4..5a48c7caa5 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -11,6 +11,11 @@
Different than kernel, the implementation also returns EINVAL for an
invalid NULL __CL_ARGS or __FUNC (similar to __clone).
+ The stack is not aligned prior the syscall, different than exported
+ __clone. Adjusting the stack on child is not async-signal safe and since
+ it is used only internally the caller is responsible for proper stack
+ alignment.
+
This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
*/
extern int __clone3 (struct clone_args *__cl_args, size_t __size,
diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c b/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
deleted file mode 100644
index e039ca5a3a..0000000000
--- a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
+++ /dev/null
@@ -1,74 +0,0 @@
-/* Verify that __clone_internal properly aligns the child stack.
- Copyright (C) 2021-2022 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
-
- The GNU C Library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
-
- The GNU C Library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with the GNU C Library; if not, see
- <https://www.gnu.org/licenses/>. */
-
-#include <sched.h>
-#include <stdbool.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <string.h>
-#include <sys/wait.h>
-#include <unistd.h>
-#include <libc-pointer-arith.h>
-#include <tst-stack-align.h>
-#include <clone_internal.h>
-#include <support/xunistd.h>
-#include <support/check.h>
-
-static int
-check_stack_alignment (void *arg)
-{
- puts ("in f");
-
- return TEST_STACK_ALIGN () ? 1 : 0;
-}
-
-static int
-do_test (void)
-{
- puts ("in do_test");
-
- if (TEST_STACK_ALIGN ())
- FAIL_EXIT1 ("stack isn't aligned\n");
-
-#ifdef __ia64__
-# define STACK_SIZE (256 * 1024)
-#else
-# define STACK_SIZE (128 * 1024)
-#endif
- char st[STACK_SIZE + 1];
- /* NB: Align child stack to 1 byte. */
- char *stack = PTR_ALIGN_UP (&st[0], 2) + 1;
- struct clone_args clone_args =
- {
- .stack = (uintptr_t) stack,
- .stack_size = STACK_SIZE,
- };
- pid_t p = __clone_internal (&clone_args, check_stack_alignment, 0);
-
- /* Clone must not fail. */
- TEST_VERIFY_EXIT (p != -1);
-
- int e;
- xwaitpid (p, &e, __WCLONE);
- TEST_VERIFY (WIFEXITED (e));
- TEST_COMPARE (WEXITSTATUS (e), 0);
-
- return 0;
-}
-
-#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/x86_64/clone3.S b/sysdeps/unix/sysv/linux/x86_64/clone3.S
index 18fc2ee966..989b62bdf6 100644
--- a/sysdeps/unix/sysv/linux/x86_64/clone3.S
+++ b/sysdeps/unix/sysv/linux/x86_64/clone3.S
@@ -73,9 +73,6 @@ L(thread_start):
the outermost frame obviously. */
xorl %ebp, %ebp
- /* Align stack to 16 bytes per the x86-64 psABI. */
- and $-16, %RSP_LP
-
/* Set up arguments for the function call. */
mov %R8_LP, %RDI_LP /* Argument. */
call *%rdx /* Call function. */
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 03/11] Linux: Do not align the stack for __clone3
2022-11-04 19:01 ` [PATCH v2 03/11] Linux: Do not align the stack for __clone3 Adhemerval Zanella
@ 2023-01-11 21:43 ` Carlos O'Donell
0 siblings, 0 replies; 22+ messages in thread
From: Carlos O'Donell @ 2023-01-11 21:43 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha, Szabolcs Nagy, H . J . Lu
Cc: Christian Brauner
On 11/4/22 15:01, Adhemerval Zanella via Libc-alpha wrote:
> Adjusting the stack on child thread is not async-signal safe, since
> kernel does not enforce stack alignmnet. Although, all the internal
> usage of __clone_internal are done with all signal masked, removing
> stack alignment of __clone3 is still a net gain: is simplifies the
> function contract detail (mask/unmarsk signals) and simplifies the
> arch-specific code.
Please post v4.
Suggest:
All internal callers of __clone3 should provide an already aligned stack.
Removing the stack alignment in __clone3 is a net gain: it simplifies the
internal function contract (mask/unmask signals) along with the arch-specific
code.
>
> The internal users are responsible for correct stack alignment. If
> glibc ever exports a clone3 wrapper, it should adjust stack alignment.
>
> Checked on x86_64-linux-gnu.
> ---
> include/clone_internal.h | 5 ++
> .../sysv/linux/tst-misalign-clone-internal.c | 74 -------------------
> sysdeps/unix/sysv/linux/x86_64/clone3.S | 3 -
> 3 files changed, 5 insertions(+), 77 deletions(-)
> delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
>
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 73b8114df4..5a48c7caa5 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -11,6 +11,11 @@
> Different than kernel, the implementation also returns EINVAL for an
> invalid NULL __CL_ARGS or __FUNC (similar to __clone).
>
> + The stack is not aligned prior the syscall, different than exported
> + __clone. Adjusting the stack on child is not async-signal safe and since
> + it is used only internally the caller is responsible for proper stack
> + alignment.
Suggest:
All callers are responsible for correctly aligning the stack. The stack is not aligned
prior to the syscall (this differs from the exported __clone).
> +
> This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
> */
> extern int __clone3 (struct clone_args *__cl_args, size_t __size,
> diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c b/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
> deleted file mode 100644
> index e039ca5a3a..0000000000
> --- a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
> +++ /dev/null
> @@ -1,74 +0,0 @@
> -/* Verify that __clone_internal properly aligns the child stack.
> - Copyright (C) 2021-2022 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <https://www.gnu.org/licenses/>. */
> -
> -#include <sched.h>
> -#include <stdbool.h>
> -#include <stdint.h>
> -#include <stdio.h>
> -#include <string.h>
> -#include <sys/wait.h>
> -#include <unistd.h>
> -#include <libc-pointer-arith.h>
> -#include <tst-stack-align.h>
> -#include <clone_internal.h>
> -#include <support/xunistd.h>
> -#include <support/check.h>
> -
> -static int
> -check_stack_alignment (void *arg)
> -{
> - puts ("in f");
> -
> - return TEST_STACK_ALIGN () ? 1 : 0;
> -}
> -
> -static int
> -do_test (void)
> -{
> - puts ("in do_test");
> -
> - if (TEST_STACK_ALIGN ())
> - FAIL_EXIT1 ("stack isn't aligned\n");
> -
> -#ifdef __ia64__
> -# define STACK_SIZE (256 * 1024)
> -#else
> -# define STACK_SIZE (128 * 1024)
> -#endif
> - char st[STACK_SIZE + 1];
> - /* NB: Align child stack to 1 byte. */
> - char *stack = PTR_ALIGN_UP (&st[0], 2) + 1;
> - struct clone_args clone_args =
> - {
> - .stack = (uintptr_t) stack,
> - .stack_size = STACK_SIZE,
> - };
> - pid_t p = __clone_internal (&clone_args, check_stack_alignment, 0);
> -
> - /* Clone must not fail. */
> - TEST_VERIFY_EXIT (p != -1);
> -
> - int e;
> - xwaitpid (p, &e, __WCLONE);
> - TEST_VERIFY (WIFEXITED (e));
> - TEST_COMPARE (WEXITSTATUS (e), 0);
> -
> - return 0;
> -}
> -
> -#include <support/test-driver.c>
OK.
> diff --git a/sysdeps/unix/sysv/linux/x86_64/clone3.S b/sysdeps/unix/sysv/linux/x86_64/clone3.S
> index 18fc2ee966..989b62bdf6 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/clone3.S
> +++ b/sysdeps/unix/sysv/linux/x86_64/clone3.S
> @@ -73,9 +73,6 @@ L(thread_start):
> the outermost frame obviously. */
> xorl %ebp, %ebp
>
> - /* Align stack to 16 bytes per the x86-64 psABI. */
> - and $-16, %RSP_LP
OK.
> -
> /* Set up arguments for the function call. */
> mov %R8_LP, %RDI_LP /* Argument. */
> call *%rdx /* Call function. */
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 04/11] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (2 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 03/11] Linux: Do not align the stack for __clone3 Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2023-01-11 21:44 ` Carlos O'Donell
2022-11-04 19:01 ` [PATCH v2 05/11] powerpc64: Add the clone3 wrapper Adhemerval Zanella
` (7 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
The clone3 flag resets all signal handlers of the child not set to
SIG_IGN to SIG_DFL. It allows to skip most of the sigaction calls
to setup child signal handling, where previously a posix_spawn
has to issue 2 times NSIG sigaction calls (one to obtain the current
disposition and another to set either SIG_DFL or SIG_IGN).
The expection is POSIX_SPAWN_SETSIGDEF the child still setup the
signal for the case the disposition is SIG_IGN.
It also need to handle the fallback where clone3 is not available,
to set the fallback in child. This is done by splitting of
__clone_internal_fallback from __clone_internal.
Checked on x86_64-linux-gnu.
---
include/clone_internal.h | 5 +
posix/Makefile | 3 +-
posix/tst-spawn7.c | 179 +++++++++++++++++++++++
sysdeps/unix/sysv/linux/Makefile | 3 +-
sysdeps/unix/sysv/linux/clone-internal.c | 37 +++--
sysdeps/unix/sysv/linux/clone3.h | 7 +
sysdeps/unix/sysv/linux/spawni.c | 31 ++--
7 files changed, 238 insertions(+), 27 deletions(-)
create mode 100644 posix/tst-spawn7.c
diff --git a/include/clone_internal.h b/include/clone_internal.h
index 5a48c7caa5..81292b3eb1 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -26,6 +26,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
fall back to clone or clone2. */
extern int __clone_internal (struct clone_args *__cl_args,
int (*__func) (void *__arg), void *__arg);
+/* The fallback code which calls clone/clone2 based on clone3 arguments. */
+extern int __clone_internal_fallback (struct clone_args *__cl_args,
+ int (*__func) (void *__arg),
+ void *__arg)
+ attribute_hidden;
#ifndef _ISOMAC
libc_hidden_proto (__clone3)
diff --git a/posix/Makefile b/posix/Makefile
index 4d60449426..6e61ce50f2 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -109,7 +109,7 @@ tests := test-errno tstgetopt testfnm runtests runptests \
tst-glob-tilde test-ssize-max tst-spawn4 bug-regex37 \
bug-regex38 tst-regcomp-truncated tst-spawn-chdir \
tst-wordexp-nocmd tst-execveat tst-spawn5 \
- tst-sched_getaffinity tst-spawn6
+ tst-sched_getaffinity tst-spawn6 tst-spawn7
# Test for the glob symbol version that was replaced in glibc 2.27.
ifeq ($(have-GLIBC_2.26)$(build-shared),yesyes)
@@ -291,6 +291,7 @@ tst-spawn-ARGS = -- $(host-test-program-cmd)
tst-spawn-static-ARGS = $(tst-spawn-ARGS)
tst-spawn5-ARGS = -- $(host-test-program-cmd)
tst-spawn6-ARGS = -- $(host-test-program-cmd)
+tst-spawn7-ARGS = -- $(host-test-program-cmd)
tst-dir-ARGS = `pwd` `cd $(common-objdir)/$(subdir); pwd` `cd $(common-objdir); pwd` $(objpfx)tst-dir
tst-chmod-ARGS = $(objdir)
tst-vfork3-ARGS = --test-dir=$(objpfx)
diff --git a/posix/tst-spawn7.c b/posix/tst-spawn7.c
new file mode 100644
index 0000000000..7cb7347c58
--- /dev/null
+++ b/posix/tst-spawn7.c
@@ -0,0 +1,179 @@
+/* Tests for posix_spawn signal handling.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <assert.h>
+#include <getopt.h>
+#include <spawn.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xsignal.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+/* Nonzero if the program gets called via `exec'. */
+#define CMDLINE_OPTIONS \
+ { "restart", no_argument, &restart, 1 },
+static int restart;
+
+/* Hold the four initial argument used to respawn the process, plus the extra
+ '--direct', '--restart', the check type ('SIG_IGN' or 'SIG_DFL'), and a
+ final NULL. */
+static char *spargs[8];
+static int check_type_argc;
+
+/* Called on process re-execution. */
+_Noreturn static void
+handle_restart (int argc, char *argv[])
+{
+ assert (argc == 1);
+
+ if (strcmp (argv[0], "SIG_DFL") == 0)
+ {
+ for (int i = 1; i < NSIG; i++)
+ {
+ struct sigaction sa;
+ int r = sigaction (i, NULL, &sa);
+ /* Skip internal signals (such as SIGCANCEL). */
+ if (r == -1)
+ continue;
+ TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
+ }
+ exit (EXIT_SUCCESS);
+ }
+ else if (strcmp (argv[0], "SIG_IGN") == 0)
+ {
+ for (int i = 1; i < NSIG; i++)
+ {
+ struct sigaction sa;
+ int r = sigaction (i, NULL, &sa);
+ /* Skip internal signals (such as SIGCANCEL). */
+ if (r == -1)
+ continue;
+ if (i == SIGUSR1 || i == SIGUSR2)
+ TEST_VERIFY_EXIT (sa.sa_handler == SIG_IGN);
+ else
+ TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
+ }
+ exit (EXIT_SUCCESS);
+ }
+
+ exit (EXIT_FAILURE);
+}
+
+static void
+spawn_signal_test (const char *type, const posix_spawnattr_t *attr)
+{
+ spargs[check_type_argc] = (char*) type;
+
+ pid_t pid;
+ int status;
+
+ TEST_COMPARE (posix_spawn (&pid, spargs[0], NULL, attr, spargs, environ), 0);
+ TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
+ TEST_VERIFY (WIFEXITED (status));
+ TEST_VERIFY (!WIFSIGNALED (status));
+ TEST_COMPARE (WEXITSTATUS (status), 0);
+}
+
+static void
+dummy_sa_handler (int)
+{
+}
+
+static void
+do_test_signals (void)
+{
+ {
+ /* Check if all signals handler are set to SIG_DFL on spawned process. */
+ spawn_signal_test ("SIG_DFL", NULL);
+ }
+
+ {
+ /* Same as before, but set SIGUSR1 and SIGUSR2 to a handler different than
+ SIG_IGN or SIG_DFL. */
+ struct sigaction sa = { 0 };
+ sa.sa_handler = dummy_sa_handler;
+ xsigaction (SIGUSR1, &sa, NULL);
+ xsigaction (SIGUSR2, &sa, NULL);
+ spawn_signal_test ("SIG_DFL", NULL);
+ }
+
+ {
+ /* Check if SIG_IGN is keep as is. */
+ struct sigaction sa = { 0 };
+ sa.sa_handler = SIG_IGN;
+ xsigaction (SIGUSR1, &sa, NULL);
+ xsigaction (SIGUSR2, &sa, NULL);
+ spawn_signal_test ("SIG_IGN", NULL);
+ }
+
+ {
+ /* Check if SIG_IGN handlers are set to SIG_DFL. */
+ posix_spawnattr_t attr;
+ posix_spawnattr_init (&attr);
+ sigset_t mask;
+ sigemptyset (&mask);
+ sigaddset (&mask, SIGUSR1);
+ sigaddset (&mask, SIGUSR2);
+ posix_spawnattr_setsigdefault (&attr, &mask);
+ posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGDEF);
+ spawn_signal_test ("SIG_DFL", &attr);
+ posix_spawnattr_destroy (&attr);
+ }
+}
+
+static int
+do_test (int argc, char *argv[])
+{
+ /* We must have either:
+
+ - one or four parameters if called initially:
+ + argv[1]: path for ld.so optional
+ + argv[2]: "--library-path" optional
+ + argv[3]: the library path optional
+ + argv[4]: the application name
+
+ - six parameters left if called through re-execution:
+ + argv[1]: the application name
+ + argv[2]: check SIG_IGN/SIG_DFL.
+
+ * When built with --enable-hardcoded-path-in-tests or issued without
+ using the loader directly. */
+
+ if (restart)
+ handle_restart (argc - 1, &argv[1]);
+
+ TEST_VERIFY_EXIT (argc == 2 || argc == 5);
+
+ int i;
+ for (i = 0; i < argc - 1; i++)
+ spargs[i] = argv[i + 1];
+ spargs[i++] = (char *) "--direct";
+ spargs[i++] = (char *) "--restart";
+ check_type_argc = i++;
+ spargs[i] = NULL;
+
+
+ do_test_signals ();
+
+ return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 60e4cb1521..c183ccd4ab 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -175,8 +175,7 @@ tests-clone-internal = \
tst-align-clone-internal \
tst-clone2-internal \
tst-clone3-internal \
- tst-getpid1-internal \
- tst-misalign-clone-internal
+ tst-getpid1-internal
tests-internal += $(tests-clone-internal)
tests-static += $(tests-clone-internal)
diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
index a71effcbd3..7bc991e033 100644
--- a/sysdeps/unix/sysv/linux/clone-internal.c
+++ b/sysdeps/unix/sysv/linux/clone-internal.c
@@ -44,27 +44,15 @@ _Static_assert (sizeof (struct clone_args) == CLONE_ARGS_SIZE_VER2,
"sizeof (struct clone_args) != CLONE_ARGS_SIZE_VER2");
int
-__clone_internal (struct clone_args *cl_args,
- int (*func) (void *arg), void *arg)
+__clone_internal_fallback (struct clone_args *cl_args,
+ int (*func) (void *arg), void *arg)
{
- int ret;
-#ifdef HAVE_CLONE3_WRAPPER
- /* Try clone3 first. */
- int saved_errno = errno;
- ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
- if (ret != -1 || errno != ENOSYS)
- return ret;
-
- /* NB: Restore errno since errno may be checked against non-zero
- return value. */
- __set_errno (saved_errno);
-#endif
-
/* Map clone3 arguments to clone arguments. NB: No need to check
invalid clone3 specific bits in flags nor exit_signal since this
is an internal function. */
int flags = cl_args->flags | cl_args->exit_signal;
void *stack = cast_to_pointer (cl_args->stack);
+ int ret;
#ifdef __ia64__
ret = __clone2 (func, stack, cl_args->stack_size,
@@ -88,4 +76,23 @@ __clone_internal (struct clone_args *cl_args,
return ret;
}
+
+int
+__clone_internal (struct clone_args *cl_args,
+ int (*func) (void *arg), void *arg)
+{
+#ifdef HAVE_CLONE3_WRAPPER
+ int saved_errno = errno;
+ int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
+ if (ret != -1 || errno != ENOSYS)
+ return ret;
+
+ /* NB: Restore errno since errno may be checked against non-zero
+ return value. */
+ __set_errno (saved_errno);
+#endif
+
+ return __clone_internal_fallback (cl_args, func, arg);
+}
+
libc_hidden_def (__clone_internal)
diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
index 7c7e2cdcb0..7dd42ec2d2 100644
--- a/sysdeps/unix/sysv/linux/clone3.h
+++ b/sysdeps/unix/sysv/linux/clone3.h
@@ -23,6 +23,13 @@
#include <stddef.h>
#include <bits/types.h>
+
+/* Flags for the clone3 syscall. */
+#define CLONE_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and
+ reset to SIG_DFL. */
+#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given
+ the right permissions. */
+
/* The unsigned 64-bit and 8-byte aligned integer type. */
typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 65ee03c804..ebc3c3e521 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -66,6 +66,7 @@ struct posix_spawn_args
ptrdiff_t argc;
char *const *envp;
int xflags;
+ bool use_clone3;
int err;
};
@@ -104,12 +105,11 @@ __spawni_child (void *arguments)
const posix_spawnattr_t *restrict attr = args->attr;
const posix_spawn_file_actions_t *file_actions = args->fa;
- /* The child must ensure that no signal handler are enabled because it shared
- memory with parent, so the signal disposition must be either SIG_DFL or
- SIG_IGN. It does by iterating over all signals and although it could
- possibly be more optimized (by tracking which signal potentially have a
- signal handler), it might requires system specific solutions (since the
- sigset_t data type can be very different on different architectures). */
+ /* The child must ensure that no signal handler is enabled because it
+ shares memory with parent, so all signal dispositions must be either
+ SIG_DFL or SIG_IGN. If clone3/CLONE_CLEAR_SIGHAND is used, there is
+ only need to set the defined signals POSIX_SPAWN_SETSIGDEF to SIG_DFL;
+ otherwise, the code iterates over all signals. */
struct sigaction sa;
memset (&sa, '\0', sizeof (sa));
@@ -122,7 +122,7 @@ __spawni_child (void *arguments)
{
sa.sa_handler = SIG_DFL;
}
- else if (__sigismember (&hset, sig))
+ else if (!args->use_clone3 && __sigismember (&hset, sig))
{
if (is_internal_signal (sig))
sa.sa_handler = SIG_IGN;
@@ -382,12 +382,25 @@ __spawnix (pid_t * pid, const char *file,
for instance). */
struct clone_args clone_args =
{
- .flags = CLONE_VM | CLONE_VFORK,
+ /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by
+ __clone_internal_fallback. */
+ .flags = CLONE_CLEAR_SIGHAND | CLONE_VM | CLONE_VFORK,
.exit_signal = SIGCHLD,
.stack = (uintptr_t) stack,
.stack_size = stack_size,
};
- new_pid = __clone_internal (&clone_args, __spawni_child, &args);
+#ifdef HAVE_CLONE3_WRAPPER
+ args.use_clone3 = true;
+ new_pid = __clone3 (&clone_args, sizeof (clone_args), __spawni_child,
+ &args);
+ /* clone3 was added on 5.3 and CLONE_CLEAR_SIGHAND on 5.5. */
+ if (new_pid == -1 && (errno == ENOSYS || errno == EINVAL))
+#endif
+ {
+ args.use_clone3 = false;
+ new_pid = __clone_internal_fallback (&clone_args, __spawni_child,
+ &args);
+ }
/* It needs to collect the case where the auxiliary process was created
but failed to execute the file (due either any preparation step or
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 04/11] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
2022-11-04 19:01 ` [PATCH v2 04/11] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn Adhemerval Zanella
@ 2023-01-11 21:44 ` Carlos O'Donell
0 siblings, 0 replies; 22+ messages in thread
From: Carlos O'Donell @ 2023-01-11 21:44 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha, Szabolcs Nagy, H . J . Lu
Cc: Christian Brauner
On 11/4/22 15:01, Adhemerval Zanella via Libc-alpha wrote:
> The clone3 flag resets all signal handlers of the child not set to
> SIG_IGN to SIG_DFL. It allows to skip most of the sigaction calls
> to setup child signal handling, where previously a posix_spawn
> has to issue 2 times NSIG sigaction calls (one to obtain the current
> disposition and another to set either SIG_DFL or SIG_IGN).
>
> The expection is POSIX_SPAWN_SETSIGDEF the child still setup the
> signal for the case the disposition is SIG_IGN.
>
> It also need to handle the fallback where clone3 is not available,
> to set the fallback in child. This is done by splitting of
> __clone_internal_fallback from __clone_internal.
>
See original v2 review. Please post v4.
> Checked on x86_64-linux-gnu.
> ---
> include/clone_internal.h | 5 +
> posix/Makefile | 3 +-
> posix/tst-spawn7.c | 179 +++++++++++++++++++++++
> sysdeps/unix/sysv/linux/Makefile | 3 +-
> sysdeps/unix/sysv/linux/clone-internal.c | 37 +++--
> sysdeps/unix/sysv/linux/clone3.h | 7 +
> sysdeps/unix/sysv/linux/spawni.c | 31 ++--
> 7 files changed, 238 insertions(+), 27 deletions(-)
> create mode 100644 posix/tst-spawn7.c
>
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 5a48c7caa5..81292b3eb1 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -26,6 +26,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
> fall back to clone or clone2. */
> extern int __clone_internal (struct clone_args *__cl_args,
> int (*__func) (void *__arg), void *__arg);
> +/* The fallback code which calls clone/clone2 based on clone3 arguments. */
> +extern int __clone_internal_fallback (struct clone_args *__cl_args,
> + int (*__func) (void *__arg),
> + void *__arg)
> + attribute_hidden;
>
> #ifndef _ISOMAC
> libc_hidden_proto (__clone3)
> diff --git a/posix/Makefile b/posix/Makefile
> index 4d60449426..6e61ce50f2 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -109,7 +109,7 @@ tests := test-errno tstgetopt testfnm runtests runptests \
> tst-glob-tilde test-ssize-max tst-spawn4 bug-regex37 \
> bug-regex38 tst-regcomp-truncated tst-spawn-chdir \
> tst-wordexp-nocmd tst-execveat tst-spawn5 \
> - tst-sched_getaffinity tst-spawn6
> + tst-sched_getaffinity tst-spawn6 tst-spawn7
>
> # Test for the glob symbol version that was replaced in glibc 2.27.
> ifeq ($(have-GLIBC_2.26)$(build-shared),yesyes)
> @@ -291,6 +291,7 @@ tst-spawn-ARGS = -- $(host-test-program-cmd)
> tst-spawn-static-ARGS = $(tst-spawn-ARGS)
> tst-spawn5-ARGS = -- $(host-test-program-cmd)
> tst-spawn6-ARGS = -- $(host-test-program-cmd)
> +tst-spawn7-ARGS = -- $(host-test-program-cmd)
> tst-dir-ARGS = `pwd` `cd $(common-objdir)/$(subdir); pwd` `cd $(common-objdir); pwd` $(objpfx)tst-dir
> tst-chmod-ARGS = $(objdir)
> tst-vfork3-ARGS = --test-dir=$(objpfx)
> diff --git a/posix/tst-spawn7.c b/posix/tst-spawn7.c
> new file mode 100644
> index 0000000000..7cb7347c58
> --- /dev/null
> +++ b/posix/tst-spawn7.c
> @@ -0,0 +1,179 @@
> +/* Tests for posix_spawn signal handling.
> + Copyright (C) 2022 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <assert.h>
> +#include <getopt.h>
> +#include <spawn.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/xsignal.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +
> +/* Nonzero if the program gets called via `exec'. */
> +#define CMDLINE_OPTIONS \
> + { "restart", no_argument, &restart, 1 },
> +static int restart;
> +
> +/* Hold the four initial argument used to respawn the process, plus the extra
> + '--direct', '--restart', the check type ('SIG_IGN' or 'SIG_DFL'), and a
> + final NULL. */
> +static char *spargs[8];
> +static int check_type_argc;
> +
> +/* Called on process re-execution. */
> +_Noreturn static void
> +handle_restart (int argc, char *argv[])
> +{
> + assert (argc == 1);
> +
> + if (strcmp (argv[0], "SIG_DFL") == 0)
> + {
> + for (int i = 1; i < NSIG; i++)
> + {
> + struct sigaction sa;
> + int r = sigaction (i, NULL, &sa);
> + /* Skip internal signals (such as SIGCANCEL). */
> + if (r == -1)
> + continue;
> + TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
> + }
> + exit (EXIT_SUCCESS);
> + }
> + else if (strcmp (argv[0], "SIG_IGN") == 0)
> + {
> + for (int i = 1; i < NSIG; i++)
> + {
> + struct sigaction sa;
> + int r = sigaction (i, NULL, &sa);
> + /* Skip internal signals (such as SIGCANCEL). */
> + if (r == -1)
> + continue;
> + if (i == SIGUSR1 || i == SIGUSR2)
> + TEST_VERIFY_EXIT (sa.sa_handler == SIG_IGN);
> + else
> + TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
> + }
> + exit (EXIT_SUCCESS);
> + }
> +
> + exit (EXIT_FAILURE);
> +}
> +
> +static void
> +spawn_signal_test (const char *type, const posix_spawnattr_t *attr)
> +{
> + spargs[check_type_argc] = (char*) type;
> +
> + pid_t pid;
> + int status;
> +
> + TEST_COMPARE (posix_spawn (&pid, spargs[0], NULL, attr, spargs, environ), 0);
> + TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
> + TEST_VERIFY (WIFEXITED (status));
> + TEST_VERIFY (!WIFSIGNALED (status));
> + TEST_COMPARE (WEXITSTATUS (status), 0);
> +}
> +
> +static void
> +dummy_sa_handler (int)
> +{
> +}
> +
> +static void
> +do_test_signals (void)
> +{
> + {
> + /* Check if all signals handler are set to SIG_DFL on spawned process. */
> + spawn_signal_test ("SIG_DFL", NULL);
> + }
> +
> + {
> + /* Same as before, but set SIGUSR1 and SIGUSR2 to a handler different than
> + SIG_IGN or SIG_DFL. */
> + struct sigaction sa = { 0 };
> + sa.sa_handler = dummy_sa_handler;
> + xsigaction (SIGUSR1, &sa, NULL);
> + xsigaction (SIGUSR2, &sa, NULL);
> + spawn_signal_test ("SIG_DFL", NULL);
> + }
> +
> + {
> + /* Check if SIG_IGN is keep as is. */
> + struct sigaction sa = { 0 };
> + sa.sa_handler = SIG_IGN;
> + xsigaction (SIGUSR1, &sa, NULL);
> + xsigaction (SIGUSR2, &sa, NULL);
> + spawn_signal_test ("SIG_IGN", NULL);
> + }
> +
> + {
> + /* Check if SIG_IGN handlers are set to SIG_DFL. */
> + posix_spawnattr_t attr;
> + posix_spawnattr_init (&attr);
> + sigset_t mask;
> + sigemptyset (&mask);
> + sigaddset (&mask, SIGUSR1);
> + sigaddset (&mask, SIGUSR2);
> + posix_spawnattr_setsigdefault (&attr, &mask);
> + posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGDEF);
> + spawn_signal_test ("SIG_DFL", &attr);
> + posix_spawnattr_destroy (&attr);
> + }
> +}
> +
> +static int
> +do_test (int argc, char *argv[])
> +{
> + /* We must have either:
> +
> + - one or four parameters if called initially:
> + + argv[1]: path for ld.so optional
> + + argv[2]: "--library-path" optional
> + + argv[3]: the library path optional
> + + argv[4]: the application name
> +
> + - six parameters left if called through re-execution:
> + + argv[1]: the application name
> + + argv[2]: check SIG_IGN/SIG_DFL.
> +
> + * When built with --enable-hardcoded-path-in-tests or issued without
> + using the loader directly. */
> +
> + if (restart)
> + handle_restart (argc - 1, &argv[1]);
> +
> + TEST_VERIFY_EXIT (argc == 2 || argc == 5);
> +
> + int i;
> + for (i = 0; i < argc - 1; i++)
> + spargs[i] = argv[i + 1];
> + spargs[i++] = (char *) "--direct";
> + spargs[i++] = (char *) "--restart";
> + check_type_argc = i++;
> + spargs[i] = NULL;
> +
> +
> + do_test_signals ();
> +
> + return 0;
> +}
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 60e4cb1521..c183ccd4ab 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -175,8 +175,7 @@ tests-clone-internal = \
> tst-align-clone-internal \
> tst-clone2-internal \
> tst-clone3-internal \
> - tst-getpid1-internal \
> - tst-misalign-clone-internal
> + tst-getpid1-internal
> tests-internal += $(tests-clone-internal)
> tests-static += $(tests-clone-internal)
>
> diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
> index a71effcbd3..7bc991e033 100644
> --- a/sysdeps/unix/sysv/linux/clone-internal.c
> +++ b/sysdeps/unix/sysv/linux/clone-internal.c
> @@ -44,27 +44,15 @@ _Static_assert (sizeof (struct clone_args) == CLONE_ARGS_SIZE_VER2,
> "sizeof (struct clone_args) != CLONE_ARGS_SIZE_VER2");
>
> int
> -__clone_internal (struct clone_args *cl_args,
> - int (*func) (void *arg), void *arg)
> +__clone_internal_fallback (struct clone_args *cl_args,
> + int (*func) (void *arg), void *arg)
> {
> - int ret;
> -#ifdef HAVE_CLONE3_WRAPPER
> - /* Try clone3 first. */
> - int saved_errno = errno;
> - ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> - if (ret != -1 || errno != ENOSYS)
> - return ret;
> -
> - /* NB: Restore errno since errno may be checked against non-zero
> - return value. */
> - __set_errno (saved_errno);
> -#endif
> -
> /* Map clone3 arguments to clone arguments. NB: No need to check
> invalid clone3 specific bits in flags nor exit_signal since this
> is an internal function. */
> int flags = cl_args->flags | cl_args->exit_signal;
> void *stack = cast_to_pointer (cl_args->stack);
> + int ret;
>
> #ifdef __ia64__
> ret = __clone2 (func, stack, cl_args->stack_size,
> @@ -88,4 +76,23 @@ __clone_internal (struct clone_args *cl_args,
> return ret;
> }
>
> +
> +int
> +__clone_internal (struct clone_args *cl_args,
> + int (*func) (void *arg), void *arg)
> +{
> +#ifdef HAVE_CLONE3_WRAPPER
> + int saved_errno = errno;
> + int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> + if (ret != -1 || errno != ENOSYS)
> + return ret;
> +
> + /* NB: Restore errno since errno may be checked against non-zero
> + return value. */
> + __set_errno (saved_errno);
> +#endif
> +
> + return __clone_internal_fallback (cl_args, func, arg);
> +}
> +
> libc_hidden_def (__clone_internal)
> diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> index 7c7e2cdcb0..7dd42ec2d2 100644
> --- a/sysdeps/unix/sysv/linux/clone3.h
> +++ b/sysdeps/unix/sysv/linux/clone3.h
> @@ -23,6 +23,13 @@
> #include <stddef.h>
> #include <bits/types.h>
>
> +
> +/* Flags for the clone3 syscall. */
> +#define CLONE_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and
> + reset to SIG_DFL. */
> +#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given
> + the right permissions. */
> +
> /* The unsigned 64-bit and 8-byte aligned integer type. */
> typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
>
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 65ee03c804..ebc3c3e521 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -66,6 +66,7 @@ struct posix_spawn_args
> ptrdiff_t argc;
> char *const *envp;
> int xflags;
> + bool use_clone3;
> int err;
> };
>
> @@ -104,12 +105,11 @@ __spawni_child (void *arguments)
> const posix_spawnattr_t *restrict attr = args->attr;
> const posix_spawn_file_actions_t *file_actions = args->fa;
>
> - /* The child must ensure that no signal handler are enabled because it shared
> - memory with parent, so the signal disposition must be either SIG_DFL or
> - SIG_IGN. It does by iterating over all signals and although it could
> - possibly be more optimized (by tracking which signal potentially have a
> - signal handler), it might requires system specific solutions (since the
> - sigset_t data type can be very different on different architectures). */
> + /* The child must ensure that no signal handler is enabled because it
> + shares memory with parent, so all signal dispositions must be either
> + SIG_DFL or SIG_IGN. If clone3/CLONE_CLEAR_SIGHAND is used, there is
> + only need to set the defined signals POSIX_SPAWN_SETSIGDEF to SIG_DFL;
> + otherwise, the code iterates over all signals. */
> struct sigaction sa;
> memset (&sa, '\0', sizeof (sa));
>
> @@ -122,7 +122,7 @@ __spawni_child (void *arguments)
> {
> sa.sa_handler = SIG_DFL;
> }
> - else if (__sigismember (&hset, sig))
> + else if (!args->use_clone3 && __sigismember (&hset, sig))
> {
> if (is_internal_signal (sig))
> sa.sa_handler = SIG_IGN;
> @@ -382,12 +382,25 @@ __spawnix (pid_t * pid, const char *file,
> for instance). */
> struct clone_args clone_args =
> {
> - .flags = CLONE_VM | CLONE_VFORK,
> + /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by
> + __clone_internal_fallback. */
> + .flags = CLONE_CLEAR_SIGHAND | CLONE_VM | CLONE_VFORK,
> .exit_signal = SIGCHLD,
> .stack = (uintptr_t) stack,
> .stack_size = stack_size,
> };
> - new_pid = __clone_internal (&clone_args, __spawni_child, &args);
> +#ifdef HAVE_CLONE3_WRAPPER
> + args.use_clone3 = true;
> + new_pid = __clone3 (&clone_args, sizeof (clone_args), __spawni_child,
> + &args);
> + /* clone3 was added on 5.3 and CLONE_CLEAR_SIGHAND on 5.5. */
> + if (new_pid == -1 && (errno == ENOSYS || errno == EINVAL))
> +#endif
> + {
> + args.use_clone3 = false;
> + new_pid = __clone_internal_fallback (&clone_args, __spawni_child,
> + &args);
> + }
>
> /* It needs to collect the case where the auxiliary process was created
> but failed to execute the file (due either any preparation step or
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 05/11] powerpc64: Add the clone3 wrapper
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (3 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 04/11] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2022-11-28 16:38 ` Paul E Murphy
2022-11-04 19:01 ` [PATCH v2 06/11] aarch64: " Adhemerval Zanella
` (6 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
It follows the internal signature:
extern int clone3 (struct clone_args *__cl_args, size_t __size,
int (*__func) (void *__arg), void *__arg);
The powerpc64 ABI requires an initial stackframe so the child can
store/restore the TOC. Iit is create prior calling clone3 by
adjusting the stack size (since kernel will compute the stack as
stack plus size).
Checked on powerpc64-linux-gnu (power8, kernel 6.0) and
powerpc64le-linux-gnu (power9, kernel 4.18).
---
.../sysv/linux/powerpc/powerpc64/clone3.S | 152 ++++++++++++++++++
sysdeps/unix/sysv/linux/powerpc/sysdep.h | 1 +
2 files changed, 153 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
new file mode 100644
index 0000000000..0fe2fe91db
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
@@ -0,0 +1,152 @@
+/* The clone3 syscall wrapper. Linux/powerpc64 version.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <sysdep.h>
+#define _ERRNO_H 1
+#include <bits/errno.h>
+
+/* The userland implementation is:
+ int clone3 (struct clone_args *cl_args, size_t size,
+ int (*func)(void *arg), void *arg);
+
+ the kernel entry is:
+ int clone3 (struct clone_args *cl_args, size_t size);
+
+ The parameters are passed in registers from userland:
+ r3: cl_args
+ r4: size
+ r5: func
+ r6: arg */
+
+ .text
+ENTRY(__clone3)
+ CALL_MCOUNT 4
+
+ /* Sanity checks args. */
+ cmpdi cr0, r3, 0
+ cmpdi cr1, r5, 0
+ cror cr0*4+eq, cr1*4+eq, cr0*4+eq
+ beq cr0,L(badargs)
+
+ /* Save some regs in the "red zone". */
+#ifdef USE_PPC_SCV
+ std r28, -24(r1)
+ cfi_offset (r28, -24)
+#endif
+ std r29, -16(r1)
+ std r30, -8(r1)
+ cfi_offset (r29, -16)
+ cfi_offset (r30, -8)
+
+ /* Save fn and args across syscall. */
+ mr r30, r5 /* Function in r30. */
+ mr r29, r6 /* Argument in r29. */
+
+ /* End FDE now, because in the child the unwind info will be
+ wrong. */
+ cfi_endproc
+
+ /* Setup a minimum stack fram for child. It needs to first calculate
+ the effective stack address, write the start NULL frame-pointer,
+ and update the frame size in the input cl_args. */
+ ld r7, 40(r3) /* Load stack value. */
+ ld r8, 48(r3) /* Load stack_size value. */
+ li r10, 0
+ add r7, r7, r8 /* Calculate effective stack address. */
+ std r10, -FRAME_MIN_SIZE_PARM(r7)
+ addi r8, r8, -FRAME_MIN_SIZE_PARM
+ std r8, 48(r3)
+
+ /* Do the system call, the kernel expects:
+ r0: system call numer
+ r3: cl_args
+ r4: size */
+ li r0, SYS_ify(clone3)
+#ifdef USE_PPC_SCV
+ CHECK_SCV_SUPPORT r28 0f
+ /* This is equivalent to DO_CALL_SCV, but we cannot use the macro here
+ because it uses CFI directives and we just called cfi_endproc. */
+ mflr r9
+ std r9, FRAME_LR_SAVE(r1)
+ .machine "push"
+ .machine "power9"
+ scv 0
+ .machine "pop"
+ ld r9, FRAME_LR_SAVE(r1)
+ mtlr r9
+
+ /* When using scv, error is indicated by negative r3. */
+ cmpdi cr1, r3, 0
+ b 1f
+#endif
+0: DO_CALL_SC
+
+ /* With sc, error is indicated by cr0.SO. */
+ cmpdi cr1, r3, 0
+ crandc cr1*4+eq, cr1*4+eq, cr0*4+so
+
+1: bne- cr1,L(parent)
+
+ /* Child, load the function and arguments. */
+ std r2, FRAME_TOC_SAVE(r1)
+ PPC64_LOAD_FUNCPTR r30
+ mr r3, r29
+ bctrl
+ ld r2, FRAME_TOC_SAVE(r1)
+
+ li r0, SYS_ify(exit)
+ DO_CALL_SC
+ /* We won't ever get here but provide a nop so that the linker
+ will insert a toc adjusting stub if necessary. */
+ nop
+
+L(badargs):
+ cfi_startproc
+ li r3, EINVAL
+ TAIL_CALL_SYSCALL_ERROR
+
+L(parent):
+ /* Check if svc is available. */
+ cmpdi cr1, r28, 0
+
+ /* Parent. Restore registers & return. */
+#ifdef USE_PPC_SCV
+ cfi_offset (r28, -24)
+ ld r28, -24(r1)
+ cfi_restore (r28)
+#endif
+ cfi_offset (r29,-16)
+ cfi_offset (r30,-8)
+ ld r29, -16(r1)
+ ld r30, -8(r1)
+ cfi_restore (r29)
+ cfi_restore (r30)
+
+#ifdef USE_PPC_SCV
+ beq cr1, 0f
+ RET_SCV
+ b 1f
+#endif
+0: RET_SC
+1: TAIL_CALL_SYSCALL_ERROR
+
+PSEUDO_END (__clone3)
+
+libc_hidden_def (__clone3)
+weak_alias (__clone3, clone3)
diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
index 9e44818978..af63b3974b 100644
--- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
@@ -214,6 +214,7 @@
#if defined(__PPC64__) || defined(__powerpc64__)
#define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres"
#define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime"
+#define HAVE_CLONE3_WRAPPER 1
#else
#define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres"
#define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 05/11] powerpc64: Add the clone3 wrapper
2022-11-04 19:01 ` [PATCH v2 05/11] powerpc64: Add the clone3 wrapper Adhemerval Zanella
@ 2022-11-28 16:38 ` Paul E Murphy
2022-11-29 20:43 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 22+ messages in thread
From: Paul E Murphy @ 2022-11-28 16:38 UTC (permalink / raw)
To: libc-alpha, Adhemerval Zanella
On 11/4/22 2:01 PM, Adhemerval Zanella via Libc-alpha wrote:
> It follows the internal signature:
>
> extern int clone3 (struct clone_args *__cl_args, size_t __size,
> int (*__func) (void *__arg), void *__arg);
>
> The powerpc64 ABI requires an initial stackframe so the child can
> store/restore the TOC. Iit is create prior calling clone3 by
trivial nit, s/Iit/It/.
> adjusting the stack size (since kernel will compute the stack as
> stack plus size).
>
> Checked on powerpc64-linux-gnu (power8, kernel 6.0) and
> powerpc64le-linux-gnu (power9, kernel 4.18).
> ---
> .../sysv/linux/powerpc/powerpc64/clone3.S | 152 ++++++++++++++++++
> sysdeps/unix/sysv/linux/powerpc/sysdep.h | 1 +
> 2 files changed, 153 insertions(+)
> create mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
>
> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
> new file mode 100644
> index 0000000000..0fe2fe91db
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
> @@ -0,0 +1,152 @@
> +/* The clone3 syscall wrapper. Linux/powerpc64 version.
> + Copyright (C) 2022 Free Software Foundation, Inc.
> +
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <sysdep.h>
> +#define _ERRNO_H 1
> +#include <bits/errno.h>
> +
> +/* The userland implementation is:
> + int clone3 (struct clone_args *cl_args, size_t size,
> + int (*func)(void *arg), void *arg);
> +
> + the kernel entry is:
> + int clone3 (struct clone_args *cl_args, size_t size);
> +
> + The parameters are passed in registers from userland:
> + r3: cl_args
> + r4: size
> + r5: func
> + r6: arg */
> +
> + .text
> +ENTRY(__clone3)
> + CALL_MCOUNT 4
> +
> + /* Sanity checks args. */
> + cmpdi cr0, r3, 0
> + cmpdi cr1, r5, 0
> + cror cr0*4+eq, cr1*4+eq, cr0*4+eq
> + beq cr0,L(badargs)
> +
> + /* Save some regs in the "red zone". */
Is there reason to avoid explicitly stacking a frame in here? Also, does
the red zone exist on ELFv1?
> +#ifdef USE_PPC_SCV
> + std r28, -24(r1)
> + cfi_offset (r28, -24)
> +#endif
> + std r29, -16(r1)
> + std r30, -8(r1)
> + cfi_offset (r29, -16)
> + cfi_offset (r30, -8)
> +
> + /* Save fn and args across syscall. */
trivial nit, s/fn/func/ s/args/arg/.
> + mr r30, r5 /* Function in r30. */
> + mr r29, r6 /* Argument in r29. */
> +
> + /* End FDE now, because in the child the unwind info will be
> + wrong. */
> + cfi_endproc
> +
> + /* Setup a minimum stack fram for child. It needs to first calculate
s/fram/frame/.
> + the effective stack address, write the start NULL frame-pointer,
> + and update the frame size in the input cl_args. */
> + ld r7, 40(r3) /* Load stack value. */
> + ld r8, 48(r3) /* Load stack_size value. */
> + li r10, 0
> + add r7, r7, r8 /* Calculate effective stack address. */
> + std r10, -FRAME_MIN_SIZE_PARM(r7)
This is clearing the backchain pointer? Maybe a small comment here.
> + addi r8, r8, -FRAME_MIN_SIZE_PARM
> + std r8, 48(r3)
> +
> + /* Do the system call, the kernel expects:
> + r0: system call numer
> + r3: cl_args
> + r4: size */
> + li r0, SYS_ify(clone3)
> +#ifdef USE_PPC_SCV
> + CHECK_SCV_SUPPORT r28 0f
> + /* This is equivalent to DO_CALL_SCV, but we cannot use the macro here
> + because it uses CFI directives and we just called cfi_endproc. */
> + mflr r9
> + std r9, FRAME_LR_SAVE(r1)
> + .machine "push"
> + .machine "power9"
> + scv 0
> + .machine "pop"
> + ld r9, FRAME_LR_SAVE(r1)
> + mtlr r9
> +
> + /* When using scv, error is indicated by negative r3. */
With scv an, an error is a value -4095 <= x < 0. This test should be
more specific.
> + cmpdi cr1, r3, 0
> + b 1f
> +#endif
> +0: DO_CALL_SC
> +
> + /* With sc, error is indicated by cr0.SO. */
> + cmpdi cr1, r3, 0
> + crandc cr1*4+eq, cr1*4+eq, cr0*4+so > +
> +1: bne- cr1,L(parent)
> +
> + /* Child, load the function and arguments. */
> + std r2, FRAME_TOC_SAVE(r1)
> + PPC64_LOAD_FUNCPTR r30
> + mr r3, r29
> + bctrl
> + ld r2, FRAME_TOC_SAVE(r1)
> +
> + li r0, SYS_ify(exit)
> + DO_CALL_SC
Minor nit, this should also use scv if supported.
> + /* We won't ever get here but provide a nop so that the linker
> + will insert a toc adjusting stub if necessary. */
> + nop
> +
> +L(badargs):
> + cfi_startproc
> + li r3, EINVAL
> + TAIL_CALL_SYSCALL_ERROR
> +
> +L(parent):
> + /* Check if svc is available. */
s/svc/scv/.
> + cmpdi cr1, r28, 0
> +
> + /* Parent. Restore registers & return. */
> +#ifdef USE_PPC_SCV
> + cfi_offset (r28, -24)
> + ld r28, -24(r1)
> + cfi_restore (r28)
> +#endif
> + cfi_offset (r29,-16)
> + cfi_offset (r30,-8)
> + ld r29, -16(r1)
> + ld r30, -8(r1)
> + cfi_restore (r29)
> + cfi_restore (r30)
> +
> +#ifdef USE_PPC_SCV
> + beq cr1, 0f
> + RET_SCV
> + b 1f
> +#endif
> +0: RET_SC
> +1: TAIL_CALL_SYSCALL_ERROR
> +
> +PSEUDO_END (__clone3)
> +
> +libc_hidden_def (__clone3)
> +weak_alias (__clone3, clone3)
> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
> index 9e44818978..af63b3974b 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
> @@ -214,6 +214,7 @@
> #if defined(__PPC64__) || defined(__powerpc64__)
> #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres"
> #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime"
> +#define HAVE_CLONE3_WRAPPER 1
> #else
> #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres"
> #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 05/11] powerpc64: Add the clone3 wrapper
2022-11-28 16:38 ` Paul E Murphy
@ 2022-11-29 20:43 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2022-11-29 20:43 UTC (permalink / raw)
To: Paul E Murphy, libc-alpha
On 28/11/22 13:38, Paul E Murphy wrote:
>
>
> On 11/4/22 2:01 PM, Adhemerval Zanella via Libc-alpha wrote:
>> It follows the internal signature:
>>
>> extern int clone3 (struct clone_args *__cl_args, size_t __size,
>> int (*__func) (void *__arg), void *__arg);
>>
>> The powerpc64 ABI requires an initial stackframe so the child can
>> store/restore the TOC. Iit is create prior calling clone3 by
>
> trivial nit, s/Iit/It/.
Ack.
>
>> adjusting the stack size (since kernel will compute the stack as
>> stack plus size).
>>
>> Checked on powerpc64-linux-gnu (power8, kernel 6.0) and
>> powerpc64le-linux-gnu (power9, kernel 4.18).
>> ---
>> .../sysv/linux/powerpc/powerpc64/clone3.S | 152 ++++++++++++++++++
>> sysdeps/unix/sysv/linux/powerpc/sysdep.h | 1 +
>> 2 files changed, 153 insertions(+)
>> create mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
>>
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
>> new file mode 100644
>> index 0000000000..0fe2fe91db
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
>> @@ -0,0 +1,152 @@
>> +/* The clone3 syscall wrapper. Linux/powerpc64 version.
>> + Copyright (C) 2022 Free Software Foundation, Inc.
>> +
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, see
>> + <https://www.gnu.org/licenses/>. */
>> +
>> +#include <sysdep.h>
>> +#define _ERRNO_H 1
>> +#include <bits/errno.h>
>> +
>> +/* The userland implementation is:
>> + int clone3 (struct clone_args *cl_args, size_t size,
>> + int (*func)(void *arg), void *arg);
>> +
>> + the kernel entry is:
>> + int clone3 (struct clone_args *cl_args, size_t size);
>> +
>> + The parameters are passed in registers from userland:
>> + r3: cl_args
>> + r4: size
>> + r5: func
>> + r6: arg */
>> +
>> + .text
>> +ENTRY(__clone3)
>> + CALL_MCOUNT 4
>> +
>> + /* Sanity checks args. */
>> + cmpdi cr0, r3, 0
>> + cmpdi cr1, r5, 0
>> + cror cr0*4+eq, cr1*4+eq, cr0*4+eq
>> + beq cr0,L(badargs)
>> +
>> + /* Save some regs in the "red zone". */
> Is there reason to avoid explicitly stacking a frame in here? Also, does the red zone exist on ELFv1?
>
I don't think so, although it allows to tail call directly to
__syscall_error. I used powerpc64 clone.S as example, which does
something similar.
>> +#ifdef USE_PPC_SCV
>> + std r28, -24(r1)
>> + cfi_offset (r28, -24)
>> +#endif
>> + std r29, -16(r1)
>> + std r30, -8(r1)
>> + cfi_offset (r29, -16)
>> + cfi_offset (r30, -8)
>> +
>> + /* Save fn and args across syscall. */
> trivial nit, s/fn/func/ s/args/arg/.
Ack.
>
>> + mr r30, r5 /* Function in r30. */
>> + mr r29, r6 /* Argument in r29. */
>> +
>> + /* End FDE now, because in the child the unwind info will be
>> + wrong. */
>> + cfi_endproc
>> +
>> + /* Setup a minimum stack fram for child. It needs to first calculate
> s/fram/frame/.
Ack.
>
>> + the effective stack address, write the start NULL frame-pointer,
>> + and update the frame size in the input cl_args. */
>> + ld r7, 40(r3) /* Load stack value. */
>> + ld r8, 48(r3) /* Load stack_size value. */
>> + li r10, 0
>> + add r7, r7, r8 /* Calculate effective stack address. */
>> + std r10, -FRAME_MIN_SIZE_PARM(r7)
> This is clearing the backchain pointer? Maybe a small comment here.
Yes, since this stack will be the initial one. I changed the comment to:
/* Setup a minimum stack frame for child. It needs to first calculate
the effective stack address, write the start empty backchain pointer,
and update the frame size in the input cl_args. */
To make it more clear.
>
>> + addi r8, r8, -FRAME_MIN_SIZE_PARM
>> + std r8, 48(r3)
>> +
>> + /* Do the system call, the kernel expects:
>> + r0: system call numer
>> + r3: cl_args
>> + r4: size */
>> + li r0, SYS_ify(clone3)
>> +#ifdef USE_PPC_SCV
>> + CHECK_SCV_SUPPORT r28 0f
>> + /* This is equivalent to DO_CALL_SCV, but we cannot use the macro here
>> + because it uses CFI directives and we just called cfi_endproc. */
>> + mflr r9
>> + std r9, FRAME_LR_SAVE(r1)
>> + .machine "push"
>> + .machine "power9"
>> + scv 0
>> + .machine "pop"
>> + ld r9, FRAME_LR_SAVE(r1)
>> + mtlr r9
>> +
>> + /* When using scv, error is indicated by negative r3. */
> With scv an, an error is a value -4095 <= x < 0. This test should be more specific.
Ack, I changed the comment (btw clone.S has the same mistake).
>
>> + cmpdi cr1, r3, 0
>> + b 1f
>> +#endif
>> +0: DO_CALL_SC
>> +
>> + /* With sc, error is indicated by cr0.SO. */
>> + cmpdi cr1, r3, 0
>> + crandc cr1*4+eq, cr1*4+eq, cr0*4+so > +
>> +1: bne- cr1,L(parent)
>> +
>> + /* Child, load the function and arguments. */
>> + std r2, FRAME_TOC_SAVE(r1)
>> + PPC64_LOAD_FUNCPTR r30
>> + mr r3, r29
>> + bctrl
>> + ld r2, FRAME_TOC_SAVE(r1)
>> +
>> + li r0, SYS_ify(exit)
>> + DO_CALL_SC
> Minor nit, this should also use scv if supported.
Ack (btw clone.S also use issues DO_CALL_SC).
>> + /* We won't ever get here but provide a nop so that the linker
>> + will insert a toc adjusting stub if necessary. */
>> + nop
>> +
>> +L(badargs):
>> + cfi_startproc
>> + li r3, EINVAL
>> + TAIL_CALL_SYSCALL_ERROR
>> +
>> +L(parent):
>> + /* Check if svc is available. */
> s/svc/scv/.
Ack.
>
>> + cmpdi cr1, r28, 0
>> +
>> + /* Parent. Restore registers & return. */
>> +#ifdef USE_PPC_SCV
>> + cfi_offset (r28, -24)
>> + ld r28, -24(r1)
>> + cfi_restore (r28)
>> +#endif
>> + cfi_offset (r29,-16)
>> + cfi_offset (r30,-8)
>> + ld r29, -16(r1)
>> + ld r30, -8(r1)
>> + cfi_restore (r29)
>> + cfi_restore (r30)
>> +
>> +#ifdef USE_PPC_SCV
>> + beq cr1, 0f
>> + RET_SCV
>> + b 1f
>> +#endif
>> +0: RET_SC
>> +1: TAIL_CALL_SYSCALL_ERROR
>> +
>> +PSEUDO_END (__clone3)
>> +
>> +libc_hidden_def (__clone3)
>> +weak_alias (__clone3, clone3)
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
>> index 9e44818978..af63b3974b 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
>> @@ -214,6 +214,7 @@
>> #if defined(__PPC64__) || defined(__powerpc64__)
>> #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres"
>> #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime"
>> +#define HAVE_CLONE3_WRAPPER 1
>> #else
>> #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres"
>> #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 06/11] aarch64: Add the clone3 wrapper
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (4 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 05/11] powerpc64: Add the clone3 wrapper Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2022-11-07 16:14 ` Szabolcs Nagy
2022-11-04 19:01 ` [PATCH v2 07/11] s390x: " Adhemerval Zanella
` (5 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
It follow the internal signature:
extern int clone3 (struct clone_args *__cl_args, size_t __size,
int (*__func) (void *__arg), void *__arg);
Checked on aarch64-linux-gnu.
---
sysdeps/unix/sysv/linux/aarch64/clone3.S | 85 ++++++++++++++++++++++++
sysdeps/unix/sysv/linux/aarch64/sysdep.h | 2 +
2 files changed, 87 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
diff --git a/sysdeps/unix/sysv/linux/aarch64/clone3.S b/sysdeps/unix/sysv/linux/aarch64/clone3.S
new file mode 100644
index 0000000000..5d42bc03e9
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/clone3.S
@@ -0,0 +1,85 @@
+/* The clone3 syscall wrapper. Linux/aarch64 version.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <sysdep.h>
+#define _ERRNO_H 1
+#include <bits/errno.h>
+
+/* The userland implementation is:
+ int clone3 (struct clone_args *cl_args, size_t size,
+ int (*func)(void *arg), void *arg);
+
+ the kernel entry is:
+ int clone3 (struct clone_args *cl_args, size_t size);
+
+ The parameters are passed in registers from userland:
+ x0: cl_args
+ x1: size
+ x2: func
+ x3: arg */
+
+ .text
+ENTRY(__clone3)
+ PTR_ARG (0)
+ PTR_ARG (1)
+ PTR_ARG (3)
+ PTR_ARG (4)
+ /* Save args for the child. */
+ mov x10, x0 /* cl_args */
+ mov x11, x2 /* func */
+ mov x12, x3 /* args */
+
+ /* Sanity check args. */
+ mov x0, #-EINVAL
+ cbz x10, .Lsyscall_error /* No NULL cl_args pointer. */
+ cbz x2, .Lsyscall_error /* No NULL function pointer. */
+
+ /* Do the system call, the kernel expects:
+ x8: system call number
+ x0: cl_args
+ x1: size */
+ mov x0, x10
+ mov x8, #SYS_ify(clone3)
+ svc 0x0
+
+ cmp x0, #0
+ beq thread_start
+ blt .Lsyscall_error
+ RET
+PSEUDO_END (__clone3)
+
+ .align 4
+ .type thread_start, %function
+thread_start:
+ cfi_startproc
+ cfi_undefined (x30)
+ mov x29, 0
+
+ /* Pick the function arg and execute. */
+ mov x0, x12
+ blr x11
+
+ /* We are done, pass the return value through x0. */
+ mov x8, #SYS_ify(exit)
+ svc 0x0
+ cfi_endproc
+ .size thread_start, .-thread_start
+
+libc_hidden_def (__clone3)
+weak_alias (__clone3, clone3)
diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
index 8ba50dab8f..00c0a02693 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
@@ -164,6 +164,8 @@
# define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime"
# define HAVE_GETTIMEOFDAY_VSYSCALL "__kernel_gettimeofday"
+# define HAVE_CLONE3_WRAPPER 1
+
# undef INTERNAL_SYSCALL_RAW
# define INTERNAL_SYSCALL_RAW(name, nr, args...) \
({ long _sys_result; \
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 06/11] aarch64: Add the clone3 wrapper
2022-11-04 19:01 ` [PATCH v2 06/11] aarch64: " Adhemerval Zanella
@ 2022-11-07 16:14 ` Szabolcs Nagy
0 siblings, 0 replies; 22+ messages in thread
From: Szabolcs Nagy @ 2022-11-07 16:14 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, H . J . Lu, Christian Brauner
The 11/04/2022 16:01, Adhemerval Zanella wrote:
> It follow the internal signature:
>
> extern int clone3 (struct clone_args *__cl_args, size_t __size,
> int (*__func) (void *__arg), void *__arg);
>
> Checked on aarch64-linux-gnu.
the patch looks good once the initial patches are accepted.
Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
> ---
> sysdeps/unix/sysv/linux/aarch64/clone3.S | 85 ++++++++++++++++++++++++
> sysdeps/unix/sysv/linux/aarch64/sysdep.h | 2 +
> 2 files changed, 87 insertions(+)
> create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>
> diff --git a/sysdeps/unix/sysv/linux/aarch64/clone3.S b/sysdeps/unix/sysv/linux/aarch64/clone3.S
> new file mode 100644
> index 0000000000..5d42bc03e9
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/aarch64/clone3.S
> @@ -0,0 +1,85 @@
> +/* The clone3 syscall wrapper. Linux/aarch64 version.
> + Copyright (C) 2022 Free Software Foundation, Inc.
> +
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <sysdep.h>
> +#define _ERRNO_H 1
> +#include <bits/errno.h>
> +
> +/* The userland implementation is:
> + int clone3 (struct clone_args *cl_args, size_t size,
> + int (*func)(void *arg), void *arg);
> +
> + the kernel entry is:
> + int clone3 (struct clone_args *cl_args, size_t size);
> +
> + The parameters are passed in registers from userland:
> + x0: cl_args
> + x1: size
> + x2: func
> + x3: arg */
> +
> + .text
> +ENTRY(__clone3)
> + PTR_ARG (0)
> + PTR_ARG (1)
> + PTR_ARG (3)
> + PTR_ARG (4)
> + /* Save args for the child. */
> + mov x10, x0 /* cl_args */
> + mov x11, x2 /* func */
> + mov x12, x3 /* args */
> +
> + /* Sanity check args. */
> + mov x0, #-EINVAL
> + cbz x10, .Lsyscall_error /* No NULL cl_args pointer. */
> + cbz x2, .Lsyscall_error /* No NULL function pointer. */
> +
> + /* Do the system call, the kernel expects:
> + x8: system call number
> + x0: cl_args
> + x1: size */
> + mov x0, x10
> + mov x8, #SYS_ify(clone3)
> + svc 0x0
> +
> + cmp x0, #0
> + beq thread_start
> + blt .Lsyscall_error
> + RET
> +PSEUDO_END (__clone3)
> +
> + .align 4
> + .type thread_start, %function
> +thread_start:
> + cfi_startproc
> + cfi_undefined (x30)
> + mov x29, 0
> +
> + /* Pick the function arg and execute. */
> + mov x0, x12
> + blr x11
> +
> + /* We are done, pass the return value through x0. */
> + mov x8, #SYS_ify(exit)
> + svc 0x0
> + cfi_endproc
> + .size thread_start, .-thread_start
> +
> +libc_hidden_def (__clone3)
> +weak_alias (__clone3, clone3)
> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> index 8ba50dab8f..00c0a02693 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> @@ -164,6 +164,8 @@
> # define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime"
> # define HAVE_GETTIMEOFDAY_VSYSCALL "__kernel_gettimeofday"
>
> +# define HAVE_CLONE3_WRAPPER 1
> +
> # undef INTERNAL_SYSCALL_RAW
> # define INTERNAL_SYSCALL_RAW(name, nr, args...) \
> ({ long _sys_result; \
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 07/11] s390x: Add the clone3 wrapper
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (5 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 06/11] aarch64: " Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2022-11-04 19:01 ` [PATCH v2 08/11] riscv: " Adhemerval Zanella
` (4 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
It follows the internal signature:
extern int clone3 (struct clone_args *__cl_args, size_t __size,
int (*__func) (void *__arg), void *__arg);
Checked on s390x-linux-gnu.
---
sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 81 +++++++++++++++++++
sysdeps/unix/sysv/linux/s390/sysdep.h | 1 +
2 files changed, 82 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S
new file mode 100644
index 0000000000..873d9a74b7
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S
@@ -0,0 +1,81 @@
+/* The clone3 syscall wrapper. Linux/s390x version.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <sysdep.h>
+#define _ERRNO_H 1
+#include <bits/errno.h>
+
+/* The userland implementation is:
+ int clone3 (struct clone_args *cl_args, size_t size,
+ int (*func)(void *arg), void *arg);
+
+ the kernel entry is:
+ int clone3 (struct clone_args *cl_args, size_t size);
+
+ The parameters are passed in registers from userland:
+ r2: cl_args
+ r3: size
+ r4: func
+ r5: arg */
+
+ .text
+ENTRY(__clone3)
+ stg %r6, 48(%r15)
+
+ /* Sanity check args. */
+ ltgr %r2, %r2
+ je error
+ ltgr %r6, %r4
+ je error
+
+ /* Do the system call, the kernel expects:
+ r1: system call number
+ r2: cl_args
+ r3: size */
+ lghi %r1, SYS_ify(clone3)
+ svc 0
+ ltgr %r2,%r2 /* check return code */
+ jz thread_start
+ lg %r6, 48(%r15)
+ jgm SYSCALL_ERROR_LABEL
+ br %r14
+error:
+ lghi %r2,-EINVAL
+ jg SYSCALL_ERROR_LABEL
+PSEUDO_END (__clone3)
+
+ .align 4
+ .type thread_start, %function
+thread_start:
+ cfi_startproc
+ /* Mark r14 as undefined in order to stop unwinding here. */
+ cfi_undefined (r14)
+
+ /* func is in gpr 6, arg in gpr 5. */
+ lgr %r2, %r5
+ aghi %r15, -160
+ xc 0(8,%r15),0(%r15)
+ basr %r14, %r6
+
+ DO_CALL (exit, 1)
+ cfi_endproc
+ .size thread_start, .-thread_start
+
+libc_hidden_def (__clone3)
+weak_alias (__clone3, clone3)
diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h
index 930d7efe03..366c797ef1 100644
--- a/sysdeps/unix/sysv/linux/s390/sysdep.h
+++ b/sysdeps/unix/sysv/linux/s390/sysdep.h
@@ -72,6 +72,7 @@
#ifdef __s390x__
#define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres"
#define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime"
+#define HAVE_CLONE3_WRAPPER 1
#else
#define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres"
#define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 08/11] riscv: Add the clone3 wrapper
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (6 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 07/11] s390x: " Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2022-11-04 19:01 ` [PATCH v2 09/11] arm: " Adhemerval Zanella
` (3 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
It follows the internal signature:
extern int clone3 (struct clone_args *__cl_args, size_t __size,
int (*__func) (void *__arg), void *__arg);
Checked on riscv64-linux-gnu-rv64imafdc-lp64d.
---
sysdeps/unix/sysv/linux/riscv/clone3.S | 80 ++++++++++++++++++++++++++
sysdeps/unix/sysv/linux/riscv/sysdep.h | 1 +
2 files changed, 81 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/riscv/clone3.S
diff --git a/sysdeps/unix/sysv/linux/riscv/clone3.S b/sysdeps/unix/sysv/linux/riscv/clone3.S
new file mode 100644
index 0000000000..ddbcbe36e5
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/riscv/clone3.S
@@ -0,0 +1,80 @@
+/* The clone3 syscall wrapper. Linux/RISC-V version.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <asm/errno.h>
+#include <sys/asm.h>
+#include <sysdep.h>
+
+/* The userland implementation is:
+ int clone3 (struct clone_args *cl_args, size_t size,
+ int (*func)(void *arg), void *arg);
+
+ the kernel entry is:
+ int clone3 (struct clone_args *cl_args, size_t size);
+
+ The parameters are passed in registers from userland:
+ a0: cl_args
+ a1: size
+ a2: func
+ a3: arg */
+
+ .text
+ENTRY(__clone3)
+ /* Sanity check args. */
+ beqz a0, L(invalid) /* No NULL cl_args pointer. */
+ beqz a2, L(invalid) /* No NULL function pointer. */
+
+ /* Do the system call, the kernel expects:
+ a7: system call number
+ a0: cl_args
+ a1: size */
+ li a7, __NR_clone3
+ scall
+
+ bltz a0, L(error)
+ beqz a0, L(thread_start)
+
+ ret
+
+L(invalid):
+ li a0, -EINVAL
+L(error):
+ tail __syscall_error
+END (__clone3)
+
+ENTRY(__thread_start_clone3)
+L(thread_start):
+ /* Terminate call stack by noting ra is undefined. Use a dummy
+ .cfi_label to force starting the FDE. */
+ .cfi_label .Ldummy
+ cfi_undefined (ra)
+
+ /* Restore the arg for user's function and call the user's
+ function. */
+ mv a1, a2 /* Function pointer. */
+ mv a0, a3 /* Argument pointer. */
+ jalr a1
+
+ /* Call exit with the function's return value. */
+ li a7, __NR_exit
+ scall
+END(__thread_start_clone3)
+
+libc_hidden_def (__clone3)
+weak_alias (__clone3, clone3)
diff --git a/sysdeps/unix/sysv/linux/riscv/sysdep.h b/sysdeps/unix/sysv/linux/riscv/sysdep.h
index c9af888132..4fb92d2406 100644
--- a/sysdeps/unix/sysv/linux/riscv/sysdep.h
+++ b/sysdeps/unix/sysv/linux/riscv/sysdep.h
@@ -150,6 +150,7 @@
/* RV32 does not support the gettime VDSO syscalls. */
# endif
+# define HAVE_CLONE3_WRAPPER 1
/* List of system calls which are supported as vsyscalls (for RV32 and
RV64). */
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 09/11] arm: Add the clone3 wrapper
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (7 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 08/11] riscv: " Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2022-11-04 19:01 ` [PATCH v2 10/11] mips: " Adhemerval Zanella
` (2 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
It follows the internal signature:
extern int clone3 (struct clone_args *__cl_args, size_t __size,
int (*__func) (void *__arg), void *__arg);
Checked on arm-linux-gnueabihf.
---
sysdeps/unix/sysv/linux/arm/clone3.S | 80 ++++++++++++++++++++++++++++
sysdeps/unix/sysv/linux/arm/sysdep.h | 1 +
2 files changed, 81 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/arm/clone3.S
diff --git a/sysdeps/unix/sysv/linux/arm/clone3.S b/sysdeps/unix/sysv/linux/arm/clone3.S
new file mode 100644
index 0000000000..d5949fae77
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/arm/clone3.S
@@ -0,0 +1,80 @@
+/* The clone3 syscall wrapper. Linux/arm version.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <sysdep.h>
+#define _ERRNO_H 1
+#include <bits/errno.h>
+
+/* The userland implementation is:
+ int clone3 (struct clone_args *cl_args, size_t size,
+ int (*func)(void *arg), void *arg);
+
+ the kernel entry is:
+ int clone3 (struct clone_args *cl_args, size_t size);
+
+ The parameters are passed in registers from userland:
+ r0: cl_args
+ r1: size
+ r2: func
+ r3: arg */
+
+ .text
+ENTRY(__clone3)
+ /* Sanity check args. */
+ cmp r0, #0
+ ite ne
+ cmpne r1, #0
+ moveq r0, #-EINVAL
+ beq PLTJMP(syscall_error)
+
+ /* Do the syscall, the kernel expects:
+ r7: system call number:
+ r0: cl_args
+ r1: size */
+ push { r7 }
+ cfi_adjust_cfa_offset (4)
+ cfi_rel_offset (r7, 0)
+ ldr r7, =SYS_ify(clone3)
+ swi 0x0
+ cfi_endproc
+
+ cmp r0, #0
+ beq 1f
+ pop {r7}
+ blt PLTJMP(C_SYMBOL_NAME(__syscall_error))
+ RETINSTR(, lr)
+
+ cfi_startproc
+PSEUDO_END (__clone3)
+
+1:
+ .fnstart
+ .cantunwind
+ mov r0, r3
+ mov ip, r2
+ BLX (ip)
+
+ /* And we are done, passing the return value through r0. */
+ ldr r7, =SYS_ify(exit)
+ swi 0x0
+
+ .fnend
+
+libc_hidden_def (__clone3)
+weak_alias (__clone3, clone3)
diff --git a/sysdeps/unix/sysv/linux/arm/sysdep.h b/sysdeps/unix/sysv/linux/arm/sysdep.h
index 1f270b961e..fcc86189d6 100644
--- a/sysdeps/unix/sysv/linux/arm/sysdep.h
+++ b/sysdeps/unix/sysv/linux/arm/sysdep.h
@@ -362,6 +362,7 @@ __local_syscall_error: \
#define HAVE_CLOCK_GETTIME_VSYSCALL "__vdso_clock_gettime"
#define HAVE_CLOCK_GETTIME64_VSYSCALL "__vdso_clock_gettime64"
#define HAVE_GETTIMEOFDAY_VSYSCALL "__vdso_gettimeofday"
+#define HAVE_CLONE3_WRAPPER 1
#define LOAD_ARGS_0()
#define ASM_ARGS_0
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 10/11] mips: Add the clone3 wrapper
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (8 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 09/11] arm: " Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2022-11-04 19:01 ` [PATCH v2 11/11] Linux: optimize clone3 internal usage Adhemerval Zanella
2023-01-11 21:24 ` [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Carlos O'Donell
11 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
It follows the internal signature:
extern int clone3 (struct clone_args *__cl_args, size_t __size,
int (*__func) (void *__arg), void *__arg);
Checked on mips64el-linux-gnueabihf and mipsel-linux-gnu.
---
sysdeps/unix/sysv/linux/mips/clone3.S | 139 ++++++++++++++++++++++++++
sysdeps/unix/sysv/linux/mips/sysdep.h | 2 +
2 files changed, 141 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/mips/clone3.S
diff --git a/sysdeps/unix/sysv/linux/mips/clone3.S b/sysdeps/unix/sysv/linux/mips/clone3.S
new file mode 100644
index 0000000000..7810994264
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/mips/clone3.S
@@ -0,0 +1,139 @@
+/* The clone3 syscall wrapper. Linux/mips version.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <sys/asm.h>
+#include <sysdep.h>
+#define _ERRNO_H 1
+#include <bits/errno.h>
+
+/* The userland implementation is:
+ int clone3 (struct clone_args *cl_args, size_t size,
+ int (*func)(void *arg), void *arg);
+
+ the kernel entry is:
+ int clone3 (struct clone_args *cl_args, size_t size);
+
+ The parameters are passed in registers from userland:
+ a0/$4: cl_args
+ a1/$5: size
+ a2/$6: func
+ a3/$7: arg */
+
+ .text
+ .set nomips16
+#if _MIPS_SIM == _ABIO32
+# define EXTRA_LOCALS 1
+#else
+# define EXTRA_LOCALS 0
+#endif
+#define FRAMESZ ((NARGSAVE*SZREG)+ALSZ)&ALMASK
+GPOFF= FRAMESZ-(1*SZREG)
+NESTED(__clone3, SZREG, sp)
+#ifdef __PIC__
+ SETUP_GP
+#endif
+#if FRAMESZ
+ PTR_SUBU sp, FRAMESZ
+ cfi_adjust_cfa_offset (FRAMESZ)
+#endif
+ SETUP_GP64_STACK (GPOFF, __clone)
+#ifdef __PIC__
+ SAVE_GP (GPOFF)
+#endif
+#ifdef PROF
+ .set noat
+ move $1,ra
+ jal _mcount
+ .set at
+#endif
+
+ /* Sanity check args. */
+ li v0, EINVAL
+ beqz a0, L(error) /* No NULL cl_args pointer. */
+ beqz a2, L(error) /* No NULL function pointer. */
+
+ move $8, a3 /* a3 is set to 0/1 for syscall success/error
+ while a4/$8 is returned unmodified. */
+
+ /* Do the system call, the kernel expects:
+ v0: system call number
+ a0: cl_args
+ a1: size */
+ li v0, __NR_clone3
+ cfi_endproc
+ syscall
+
+ bnez a3, L(error)
+ beqz v0, L(thread_start_clone3)
+
+ /* Successful return from the parent */
+ cfi_startproc
+#if FRAMESZ
+ cfi_adjust_cfa_offset (FRAMESZ)
+#endif
+ SETUP_GP64_STACK_CFI (GPOFF)
+ cfi_remember_state
+ RESTORE_GP64_STACK
+#if FRAMESZ
+ PTR_ADDU sp, FRAMESZ
+ cfi_adjust_cfa_offset (-FRAMESZ)
+#endif
+ ret
+
+L(error):
+ cfi_restore_state
+#ifdef __PIC__
+ PTR_LA t9,__syscall_error
+ RESTORE_GP64_STACK
+ PTR_ADDU sp, FRAMESZ
+ cfi_adjust_cfa_offset (-FRAMESZ)
+ jr t9
+#else
+ RESTORE_GP64_STACK
+ PTR_ADDU sp, FRAMESZ
+ cfi_adjust_cfa_offset (-FRAMESZ)
+ j __syscall_error
+#endif
+END (__clone3)
+
+/* Load up the arguments to the function. Put this block of code in
+ its own function so that we can terminate the stack trace with our
+ debug info. */
+
+ENTRY(__thread_start_clone3)
+L(thread_start_clone3):
+ cfi_undefined ($31)
+ /* cp is already loaded. */
+ SAVE_GP (GPOFF)
+ /* The stackframe has been created on entry of clone3. */
+
+ /* Restore the arg for user's function. */
+ move t9, a2 /* Function pointer. */
+ move a0, $8 /* Argument pointer. */
+
+ /* Call the user's function. */
+ jal t9
+
+ move a0, v0
+ li v0, __NR_exit
+ syscall
+END(__thread_start_clone3)
+
+libc_hidden_def (__clone3)
+weak_alias (__clone3, clone3)
diff --git a/sysdeps/unix/sysv/linux/mips/sysdep.h b/sysdeps/unix/sysv/linux/mips/sysdep.h
index 2515dadc26..3f464925e7 100644
--- a/sysdeps/unix/sysv/linux/mips/sysdep.h
+++ b/sysdeps/unix/sysv/linux/mips/sysdep.h
@@ -28,3 +28,5 @@
#endif
#define HAVE_GETTIMEOFDAY_VSYSCALL "__vdso_gettimeofday"
#define HAVE_CLOCK_GETRES_VSYSCALL "__vdso_clock_getres"
+
+#define HAVE_CLONE3_WRAPPER 1
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 11/11] Linux: optimize clone3 internal usage
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (9 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 10/11] mips: " Adhemerval Zanella
@ 2022-11-04 19:01 ` Adhemerval Zanella
2023-01-11 21:45 ` Carlos O'Donell
2023-01-11 21:24 ` [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Carlos O'Donell
11 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2022-11-04 19:01 UTC (permalink / raw)
To: libc-alpha, Szabolcs Nagy, H . J . Lu; +Cc: Christian Brauner
Now that clone3 is used on more architectures, add an optimization
to avoid calling when glibc detects that it is no supported by the
kernel. It also adds __ASSUME_CLONE3, which allows skip this
optimization and issue clone3 syscall directly.
It does not handle the the small window between 5.3 and 5.5 for
posix_spawn (CLONE_CLEAR_SIGHAND was added in 5.5).
Checked on x86_64-linux-gnu.
---
include/clone_internal.h | 5 +++++
sysdeps/unix/sysv/linux/clone-internal.c | 24 ++++++++++++++++++++++-
sysdeps/unix/sysv/linux/kernel-features.h | 9 +++++++++
3 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/include/clone_internal.h b/include/clone_internal.h
index 81292b3eb1..5f936dd8dc 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -26,6 +26,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
fall back to clone or clone2. */
extern int __clone_internal (struct clone_args *__cl_args,
int (*__func) (void *__arg), void *__arg);
+/* clone3 wrapper with a sticky check to avoid re-issue the syscall if
+ it fails with ENOSYS. */
+extern int __clone3_internal (struct clone_args *cl_args,
+ int (*func) (void *args), void *arg)
+ attribute_hidden;
/* The fallback code which calls clone/clone2 based on clone3 arguments. */
extern int __clone_internal_fallback (struct clone_args *__cl_args,
int (*__func) (void *__arg),
diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
index 7bc991e033..39d76733db 100644
--- a/sysdeps/unix/sysv/linux/clone-internal.c
+++ b/sysdeps/unix/sysv/linux/clone-internal.c
@@ -76,6 +76,28 @@ __clone_internal_fallback (struct clone_args *cl_args,
return ret;
}
+int
+__clone3_internal (struct clone_args *cl_args, int (*func) (void *args),
+ void *arg)
+{
+#ifdef HAVE_CLONE3_WRAPPER
+# if __ASSUME_CLONE3
+ return __clone3 (cl_args, sizeof (*cl_args), func, arg);
+# else
+ static int clone3_supported = 1;
+ if (atomic_load_relaxed (&clone3_supported) == 1)
+ {
+ int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
+ if (ret != -1 || errno != ENOSYS)
+ return ret;
+
+ atomic_store_relaxed (&clone3_supported, 0);
+ }
+# endif
+#endif
+ __set_errno (ENOSYS);
+ return -1;
+}
int
__clone_internal (struct clone_args *cl_args,
@@ -83,7 +105,7 @@ __clone_internal (struct clone_args *cl_args,
{
#ifdef HAVE_CLONE3_WRAPPER
int saved_errno = errno;
- int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
+ int ret = __clone3_internal (cl_args, func, arg);
if (ret != -1 || errno != ENOSYS)
return ret;
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 74adc3956b..4ecd08a98f 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -236,4 +236,13 @@
# define __ASSUME_FUTEX_LOCK_PI2 0
#endif
+/* The clone3 system call was introduced across on most architectures in
+ Linux 5.3. Not all ports implements it, so it should be used along
+ HAVE_CLONE3_WRAPPER define. */
+#if __LINUX_KERNEL_VERSION >= 0x050300
+# define __ASSUME_CLONE3 1
+#else
+# define __ASSUME_CLONE3 0
+#endif
+
#endif /* kernel-features.h */
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 11/11] Linux: optimize clone3 internal usage
2022-11-04 19:01 ` [PATCH v2 11/11] Linux: optimize clone3 internal usage Adhemerval Zanella
@ 2023-01-11 21:45 ` Carlos O'Donell
0 siblings, 0 replies; 22+ messages in thread
From: Carlos O'Donell @ 2023-01-11 21:45 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha, Szabolcs Nagy, H . J . Lu
Cc: Christian Brauner
On 11/4/22 15:01, Adhemerval Zanella via Libc-alpha wrote:
> Now that clone3 is used on more architectures, add an optimization
> to avoid calling when glibc detects that it is no supported by the
> kernel. It also adds __ASSUME_CLONE3, which allows skip this
> optimization and issue clone3 syscall directly.
>
> It does not handle the the small window between 5.3 and 5.5 for
> posix_spawn (CLONE_CLEAR_SIGHAND was added in 5.5).
>
> Checked on x86_64-linux-gnu.
Please see original v2 review. Please post v4.
> ---
> include/clone_internal.h | 5 +++++
> sysdeps/unix/sysv/linux/clone-internal.c | 24 ++++++++++++++++++++++-
> sysdeps/unix/sysv/linux/kernel-features.h | 9 +++++++++
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 81292b3eb1..5f936dd8dc 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -26,6 +26,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
> fall back to clone or clone2. */
> extern int __clone_internal (struct clone_args *__cl_args,
> int (*__func) (void *__arg), void *__arg);
> +/* clone3 wrapper with a sticky check to avoid re-issue the syscall if
> + it fails with ENOSYS. */
> +extern int __clone3_internal (struct clone_args *cl_args,
> + int (*func) (void *args), void *arg)
> + attribute_hidden;
> /* The fallback code which calls clone/clone2 based on clone3 arguments. */
> extern int __clone_internal_fallback (struct clone_args *__cl_args,
> int (*__func) (void *__arg),
> diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
> index 7bc991e033..39d76733db 100644
> --- a/sysdeps/unix/sysv/linux/clone-internal.c
> +++ b/sysdeps/unix/sysv/linux/clone-internal.c
> @@ -76,6 +76,28 @@ __clone_internal_fallback (struct clone_args *cl_args,
> return ret;
> }
>
> +int
> +__clone3_internal (struct clone_args *cl_args, int (*func) (void *args),
> + void *arg)
> +{
> +#ifdef HAVE_CLONE3_WRAPPER
> +# if __ASSUME_CLONE3
> + return __clone3 (cl_args, sizeof (*cl_args), func, arg);
> +# else
> + static int clone3_supported = 1;
> + if (atomic_load_relaxed (&clone3_supported) == 1)
> + {
> + int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> + if (ret != -1 || errno != ENOSYS)
> + return ret;
> +
> + atomic_store_relaxed (&clone3_supported, 0);
> + }
> +# endif
> +#endif
> + __set_errno (ENOSYS);
> + return -1;
> +}
>
> int
> __clone_internal (struct clone_args *cl_args,
> @@ -83,7 +105,7 @@ __clone_internal (struct clone_args *cl_args,
> {
> #ifdef HAVE_CLONE3_WRAPPER
> int saved_errno = errno;
> - int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> + int ret = __clone3_internal (cl_args, func, arg);
> if (ret != -1 || errno != ENOSYS)
> return ret;
>
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index 74adc3956b..4ecd08a98f 100644
> --- a/sysdeps/unix/sysv/linux/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/kernel-features.h
> @@ -236,4 +236,13 @@
> # define __ASSUME_FUTEX_LOCK_PI2 0
> #endif
>
> +/* The clone3 system call was introduced across on most architectures in
> + Linux 5.3. Not all ports implements it, so it should be used along
> + HAVE_CLONE3_WRAPPER define. */
> +#if __LINUX_KERNEL_VERSION >= 0x050300
> +# define __ASSUME_CLONE3 1
> +#else
> +# define __ASSUME_CLONE3 0
> +#endif
> +
> #endif /* kernel-features.h */
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3
2022-11-04 19:01 [PATCH v2 00/11] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
` (10 preceding siblings ...)
2022-11-04 19:01 ` [PATCH v2 11/11] Linux: optimize clone3 internal usage Adhemerval Zanella
@ 2023-01-11 21:24 ` Carlos O'Donell
11 siblings, 0 replies; 22+ messages in thread
From: Carlos O'Donell @ 2023-01-11 21:24 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha, Szabolcs Nagy, H . J . Lu
Cc: Christian Brauner
On 11/4/22 15:01, Adhemerval Zanella via Libc-alpha wrote:
> Currently posix_spawn has to issue at least NSIG sigaction syscalls
> to obtain current signal disposition and more if they there are not
> either SIG_IGN or SIG_DFL. The new clone3 CLONE_CLEAR_SIGHAND flags
> introduced with Linux 5.5 resets all signal handlers of the child not
> set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
> preparation phase.
>
> The exception is the signals defined by posix_spawnattr_setsigdefault
> when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN). In
> this case posix_spawn helper process needs to issue a sigction to
> set the signal disposition to SIG_DFL.
>
> The patchset also adds clone3 implementation for aarch64, powerpc64,
> s390x, riscv, arm, and mips. The clone3 for x86_64 is change to not
> align the stack, since this is not async-signal safe (although current
> calls does mask/unmask).
Same comments here that apply for the original v2.
This is actually v3.
Please post v4 with changes and limit it to x86_64 and aarch64.
If arch maintainers respond with a Reviewed-by: then we can include it into the release.
I don't want to make assembly changes this close to the release.
> * Changes from v1:
> - Extend clone3 documentation.
> - Remove x86_64 stack alignment on clone3 child.
> - Fixed powerpc64 missing initial stack frame.
> - Fixed missing save/restore r7 for arm.
>
> Adhemerval Zanella (11):
> linux: Do not reset signal handler in posix_spawn if it is already
> SIG_DFL
> linux: Extend internal clone3 documentation
> Linux: Do not align the stack for __clone3
> linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
> powerpc64: Add the clone3 wrapper
> aarch64: Add the clone3 wrapper
> s390x: Add the clone3 wrapper
> riscv: Add the clone3 wrapper
> arm: Add the clone3 wrapper
> mips: Add the clone3 wrapper
> Linux: optimize clone3 internal usage
>
> include/clone_internal.h | 39 +++-
> posix/Makefile | 3 +-
> posix/tst-spawn7.c | 179 ++++++++++++++++++
> sysdeps/unix/sysv/linux/Makefile | 3 +-
> sysdeps/unix/sysv/linux/aarch64/clone3.S | 85 +++++++++
> sysdeps/unix/sysv/linux/aarch64/sysdep.h | 2 +
> sysdeps/unix/sysv/linux/arm/clone3.S | 80 ++++++++
> sysdeps/unix/sysv/linux/arm/sysdep.h | 1 +
> sysdeps/unix/sysv/linux/clone-internal.c | 59 ++++--
> sysdeps/unix/sysv/linux/clone3.h | 15 +-
> sysdeps/unix/sysv/linux/kernel-features.h | 9 +
> sysdeps/unix/sysv/linux/mips/clone3.S | 139 ++++++++++++++
> sysdeps/unix/sysv/linux/mips/sysdep.h | 2 +
> .../sysv/linux/powerpc/powerpc64/clone3.S | 152 +++++++++++++++
> sysdeps/unix/sysv/linux/powerpc/sysdep.h | 1 +
> sysdeps/unix/sysv/linux/riscv/clone3.S | 80 ++++++++
> sysdeps/unix/sysv/linux/riscv/sysdep.h | 1 +
> sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 81 ++++++++
> sysdeps/unix/sysv/linux/s390/sysdep.h | 1 +
> sysdeps/unix/sysv/linux/spawni.c | 33 +++-
> .../sysv/linux/tst-misalign-clone-internal.c | 74 --------
> sysdeps/unix/sysv/linux/x86_64/clone3.S | 3 -
> 22 files changed, 924 insertions(+), 118 deletions(-)
> create mode 100644 posix/tst-spawn7.c
> create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
> create mode 100644 sysdeps/unix/sysv/linux/arm/clone3.S
> create mode 100644 sysdeps/unix/sysv/linux/mips/clone3.S
> create mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S
> create mode 100644 sysdeps/unix/sysv/linux/riscv/clone3.S
> create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S
> delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
>
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 22+ messages in thread