[RFC,4/5] RISC-V: Extend MINSIGSTKSZ and SIGSTKSZ to backup RVV registers
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
As using RVV extension, the original MINSIGSTKSZ is not enough to
back up all RVV registers for the normal case. Therefore, the MINSIGSTKSZ
is expanded to about 5K and the SIGSTKSZ is expanded to about 16K. This
space is enough for the case that the VLENB of a vector register is 128
bytes. For the case that VLENB > 128 bytes, users can use
sysconf (_SC_MINSIGSTKSZ) and sysconf (_SC_SIGSTKSZ) to get the
appropriate signal stack size.
---
sysdeps/unix/sysv/linux/riscv/bits/sigstack.h | 32 +++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/riscv/bits/sigstack.h
Comments
On Mon, Sep 13, 2021 at 09:41:17AM +0800, Vincent Chen wrote:
> As using RVV extension, the original MINSIGSTKSZ is not enough to
> back up all RVV registers for the normal case. Therefore, the MINSIGSTKSZ
> is expanded to about 5K and the SIGSTKSZ is expanded to about 16K. This
> space is enough for the case that the VLENB of a vector register is 128
> bytes. For the case that VLENB > 128 bytes, users can use
> sysconf (_SC_MINSIGSTKSZ) and sysconf (_SC_SIGSTKSZ) to get the
> appropriate signal stack size.
> ---
> sysdeps/unix/sysv/linux/riscv/bits/sigstack.h | 32 +++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 sysdeps/unix/sysv/linux/riscv/bits/sigstack.h
>
> diff --git a/sysdeps/unix/sysv/linux/riscv/bits/sigstack.h b/sysdeps/unix/sysv/linux/riscv/bits/sigstack.h
> new file mode 100644
> index 0000000..c18512f
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/riscv/bits/sigstack.h
> @@ -0,0 +1,32 @@
> +/* sigstack, sigaltstack definitions.
> + Copyright (C) 2021 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/>. */
> +
> +#ifndef _BITS_SIGSTACK_H
> +#define _BITS_SIGSTACK_H 1
> +
> +#if !defined _SIGNAL_H && !defined _SYS_UCONTEXT_H
> +# error "Never include this file directly. Use <signal.h> instead"
> +#endif
> +
> +/* Minimum stack size (5k+256 bytes) for a signal handler. */
> +#define MINSIGSTKSZ 5376
> +
> +/* System default stack size. */
> +#define SIGSTKSZ 16384
> +
> +#endif /* bits/sigstack.h */
> --
> 2.7.4
Strictly speaking this is also an ABI change (and what the kernel is
doing is too). If possible I think there should be an effort to get
the riscv folks to rethink this. Aside from being breakage, large
state that has the be saved/restored at context switch time is an
anti-feature. Any reasonable amount of vector state fits in the
existing size.
If it is to be changed, I suspect 5376 is too small. IIRC other archs
that have large (4k or so?) register files used something like 6k as
the min (1-2k margin for actual execution space).
Rich
On Mon, Sep 13, 2021 at 9:51 PM Rich Felker <dalias@libc.org> wrote:
>
> On Mon, Sep 13, 2021 at 09:41:17AM +0800, Vincent Chen wrote:
> > As using RVV extension, the original MINSIGSTKSZ is not enough to
> > back up all RVV registers for the normal case. Therefore, the MINSIGSTKSZ
> > is expanded to about 5K and the SIGSTKSZ is expanded to about 16K. This
> > space is enough for the case that the VLENB of a vector register is 128
> > bytes. For the case that VLENB > 128 bytes, users can use
> > sysconf (_SC_MINSIGSTKSZ) and sysconf (_SC_SIGSTKSZ) to get the
> > appropriate signal stack size.
> > ---
> > sysdeps/unix/sysv/linux/riscv/bits/sigstack.h | 32 +++++++++++++++++++++++++++
> > 1 file changed, 32 insertions(+)
> > create mode 100644 sysdeps/unix/sysv/linux/riscv/bits/sigstack.h
> >
> > diff --git a/sysdeps/unix/sysv/linux/riscv/bits/sigstack.h b/sysdeps/unix/sysv/linux/riscv/bits/sigstack.h
> > new file mode 100644
> > index 0000000..c18512f
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/riscv/bits/sigstack.h
> > @@ -0,0 +1,32 @@
> > +/* sigstack, sigaltstack definitions.
> > + Copyright (C) 2021 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/>. */
> > +
> > +#ifndef _BITS_SIGSTACK_H
> > +#define _BITS_SIGSTACK_H 1
> > +
> > +#if !defined _SIGNAL_H && !defined _SYS_UCONTEXT_H
> > +# error "Never include this file directly. Use <signal.h> instead"
> > +#endif
> > +
> > +/* Minimum stack size (5k+256 bytes) for a signal handler. */
> > +#define MINSIGSTKSZ 5376
> > +
> > +/* System default stack size. */
> > +#define SIGSTKSZ 16384
> > +
> > +#endif /* bits/sigstack.h */
> > --
> > 2.7.4
>
> Strictly speaking this is also an ABI change (and what the kernel is
> doing is too). If possible I think there should be an effort to get
> the riscv folks to rethink this. Aside from being breakage, large
> state that has the be saved/restored at context switch time is an
> anti-feature. Any reasonable amount of vector state fits in the
> existing size.
>
> If it is to be changed, I suspect 5376 is too small. IIRC other archs
> that have large (4k or so?) register files used something like 6k as
> the min (1-2k margin for actual execution space).
>
> Rich
Hi Rich
I will share this modification in the RISC-V SW mailing list to
discuss it. Thank you very much for your suggestions.
Vincent
new file mode 100644
@@ -0,0 +1,32 @@
+/* sigstack, sigaltstack definitions.
+ Copyright (C) 2021 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/>. */
+
+#ifndef _BITS_SIGSTACK_H
+#define _BITS_SIGSTACK_H 1
+
+#if !defined _SIGNAL_H && !defined _SYS_UCONTEXT_H
+# error "Never include this file directly. Use <signal.h> instead"
+#endif
+
+/* Minimum stack size (5k+256 bytes) for a signal handler. */
+#define MINSIGSTKSZ 5376
+
+/* System default stack size. */
+#define SIGSTKSZ 16384
+
+#endif /* bits/sigstack.h */