diff mbox series

[RFC,5/5] RISC-V: Expand PTHREAD_STACK_MIN to support RVV environment

Message ID 1631497278-29829-6-git-send-email-vincent.chen@sifive.com
State RFC, archived
Headers
Series RISC-V: Add vector ISA support |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Vincent Chen Sept. 13, 2021, 1:41 a.m. UTC 
  In order to support all pthread operations in the RVV environment, here
PTHREAD_STACK_MIN is set to 4 times GLRO(dl_minsigstacksize), and the
default PTHREAD_STACK_MIN is expanded to 20K bytes.
---
 .../unix/sysv/linux/riscv/bits/pthread_stack_min.h | 21 ++++++++++++
 .../sysv/linux/riscv/sysconf-pthread_stack_min.h   | 39 ++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/riscv/bits/pthread_stack_min.h
 create mode 100644 sysdeps/unix/sysv/linux/riscv/sysconf-pthread_stack_min.h

Comments

Joseph Myers Sept. 14, 2021, 11:43 p.m. UTC | #1 
On Mon, 13 Sep 2021, Vincent Chen wrote:

> In order to support all pthread operations in the RVV environment, here
> PTHREAD_STACK_MIN is set to 4 times GLRO(dl_minsigstacksize), and the
> default PTHREAD_STACK_MIN is expanded to 20K bytes.

A change to PTHREAD_STACK_MIN has been considered an ABI change in the 
past, requiring new symbol versions for pthread_attr_setstack and 
pthread_attr_setstacksize to ensure that binaries built with the old 
PTHREAD_STACK_MIN definition continue to work rather than failing because 
the old size is too small.  You may need symbol versioning updates for 
those functions in RISC-V if you make such a change.  (All the existing 
versioning support for this in architecture-independent files assumes the 
change in value was done before libpthread was merged into libc, so there 
will be some extra work involved in being the first architecture to 
increase PTHREAD_STACK_MIN after that merge.)
Florian Weimer Sept. 15, 2021, 10:42 a.m. UTC | #2 
* Joseph Myers:

> On Mon, 13 Sep 2021, Vincent Chen wrote:
>
>> In order to support all pthread operations in the RVV environment, here
>> PTHREAD_STACK_MIN is set to 4 times GLRO(dl_minsigstacksize), and the
>> default PTHREAD_STACK_MIN is expanded to 20K bytes.
>
> A change to PTHREAD_STACK_MIN has been considered an ABI change in the 
> past, requiring new symbol versions for pthread_attr_setstack and 
> pthread_attr_setstacksize to ensure that binaries built with the old 
> PTHREAD_STACK_MIN definition continue to work rather than failing because 
> the old size is too small.  You may need symbol versioning updates for 
> those functions in RISC-V if you make such a change.  (All the existing 
> versioning support for this in architecture-independent files assumes the 
> change in value was done before libpthread was merged into libc, so there 
> will be some extra work involved in being the first architecture to 
> increase PTHREAD_STACK_MIN after that merge.)

Instead it may make sense to leave PTHREAD_STACK_MIN as is and switch to
the dynamic version (same for the SIGSTKSZ constants).

Thanks,
Florian
H.J. Lu Sept. 15, 2021, 2:31 p.m. UTC | #3 
On Wed, Sep 15, 2021 at 3:42 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> * Joseph Myers:
>
> > On Mon, 13 Sep 2021, Vincent Chen wrote:
> >
> >> In order to support all pthread operations in the RVV environment, here
> >> PTHREAD_STACK_MIN is set to 4 times GLRO(dl_minsigstacksize), and the
> >> default PTHREAD_STACK_MIN is expanded to 20K bytes.
> >
> > A change to PTHREAD_STACK_MIN has been considered an ABI change in the
> > past, requiring new symbol versions for pthread_attr_setstack and
> > pthread_attr_setstacksize to ensure that binaries built with the old
> > PTHREAD_STACK_MIN definition continue to work rather than failing because
> > the old size is too small.  You may need symbol versioning updates for
> > those functions in RISC-V if you make such a change.  (All the existing
> > versioning support for this in architecture-independent files assumes the
> > change in value was done before libpthread was merged into libc, so there
> > will be some extra work involved in being the first architecture to
> > increase PTHREAD_STACK_MIN after that merge.)
>
> Instead it may make sense to leave PTHREAD_STACK_MIN as is and switch to
> the dynamic version (same for the SIGSTKSZ constants).
>

I don't know what problem RISC-V ran into.   It should be fixed with:

commit 5d98a7dae955bafa6740c26eaba9c86060ae0344
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Mon Jun 21 12:42:56 2021 -0700

    Define PTHREAD_STACK_MIN to sysconf(_SC_THREAD_STACK_MIN)

    The constant PTHREAD_STACK_MIN may be too small for some processors.
    Rename _SC_SIGSTKSZ_SOURCE to _DYNAMIC_STACK_SIZE_SOURCE.  When
    _DYNAMIC_STACK_SIZE_SOURCE or _GNU_SOURCE are defined, define
    PTHREAD_STACK_MIN to sysconf(_SC_THREAD_STACK_MIN) which is changed
    to MIN (PTHREAD_STACK_MIN, sysconf(_SC_MINSIGSTKSZ)).

    Consolidate <bits/local_lim.h> with <bits/pthread_stack_min.h> to
    provide a constant target specific PTHREAD_STACK_MIN value.

    Reviewed-by: Carlos O'Donell <carlos@redhat.com>

--
H.J.
Vincent Chen Sept. 16, 2021, 10:21 a.m. UTC | #4 
On Wed, Sep 15, 2021 at 10:31 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Sep 15, 2021 at 3:42 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > * Joseph Myers:
> >
> > > On Mon, 13 Sep 2021, Vincent Chen wrote:
> > >
> > >> In order to support all pthread operations in the RVV environment, here
> > >> PTHREAD_STACK_MIN is set to 4 times GLRO(dl_minsigstacksize), and the
> > >> default PTHREAD_STACK_MIN is expanded to 20K bytes.
> > >
> > > A change to PTHREAD_STACK_MIN has been considered an ABI change in the
> > > past, requiring new symbol versions for pthread_attr_setstack and
> > > pthread_attr_setstacksize to ensure that binaries built with the old
> > > PTHREAD_STACK_MIN definition continue to work rather than failing because
> > > the old size is too small.  You may need symbol versioning updates for
> > > those functions in RISC-V if you make such a change.  (All the existing
> > > versioning support for this in architecture-independent files assumes the
> > > change in value was done before libpthread was merged into libc, so there
> > > will be some extra work involved in being the first architecture to
> > > increase PTHREAD_STACK_MIN after that merge.)
> >
Hi Joseph,
I understood. I will add symbol versioning to pthread_attr_setstack
and pthread_attr_setstacksize functions if I decide to change the
PTHREAD_STACK_MIN definition. Thank you.

> > Instead it may make sense to leave PTHREAD_STACK_MIN as is and switch to
> > the dynamic version (same for the SIGSTKSZ constants).
> >
>
> I don't know what problem RISC-V ran into.   It should be fixed with:
>

Hi Florian and H.J. Lu,
The dynamic version works for RISC-V. However, I am afraid that some
existing programs, such as nptl/tst-minstack-cancel, do not define
_DYNAMIC_STACK_SIZE_SOURCE or _GNU_SOURCE. In this case, the original
PTHREAD_STACK_MIN definition is too small to support V extension.
Therefore, I finally decided to extend PTHREAD_STACK_MIN to 20K for
normal use cases.

> commit 5d98a7dae955bafa6740c26eaba9c86060ae0344
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Mon Jun 21 12:42:56 2021 -0700
>
>     Define PTHREAD_STACK_MIN to sysconf(_SC_THREAD_STACK_MIN)
>
>     The constant PTHREAD_STACK_MIN may be too small for some processors.
>     Rename _SC_SIGSTKSZ_SOURCE to _DYNAMIC_STACK_SIZE_SOURCE.  When
>     _DYNAMIC_STACK_SIZE_SOURCE or _GNU_SOURCE are defined, define
>     PTHREAD_STACK_MIN to sysconf(_SC_THREAD_STACK_MIN) which is changed
>     to MIN (PTHREAD_STACK_MIN, sysconf(_SC_MINSIGSTKSZ)).
>
>     Consolidate <bits/local_lim.h> with <bits/pthread_stack_min.h> to
>     provide a constant target specific PTHREAD_STACK_MIN value.
>
>     Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
For this patch, I really appreciate H.J. Lu for providing this feature
in glibc. It's really helpful. But, I have a little question. If
possible, could H.J. Lu help me clarify it?

In __get_pthread_stack_min(), when GLRO(dl_minsigstacksize) !=0 and
GLRO(dl_minsigstacksize) > PTHREAD_STACK_MIN, the pthread_stack_min
will be set to GLRO(dl_minsigstacksize). However, If my understanding
is correct, the size of GLRO(dl_minsigstacksize) approximately equals
the size of the signal context. In this case, the remaining free stack
seems not enough for GCC to execute unwind if this pthread is
terminated by pthread_cancel, such as the case in
tst-minstack-cancel.c. Therefore, my question is that the
PTHREAD_STACK_MIN does not need to reserve the space for GCC to
execute unwind? Thank you in advance.

Regards,
Vincent
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/riscv/bits/pthread_stack_min.h b/sysdeps/unix/sysv/linux/riscv/bits/pthread_stack_min.h
new file mode 100644
index 0000000..83585b3
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/riscv/bits/pthread_stack_min.h
@@ -0,0 +1,21 @@ 
+/* Definition of PTHREAD_STACK_MIN.  Linux/riscv version.
+   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/>.  */
+
+/* Minimum size for a thread.  We are free to choose a reasonable value.  */
+#define PTHREAD_STACK_MIN	20480
diff --git a/sysdeps/unix/sysv/linux/riscv/sysconf-pthread_stack_min.h b/sysdeps/unix/sysv/linux/riscv/sysconf-pthread_stack_min.h
new file mode 100644
index 0000000..53ba6a1
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/riscv/sysconf-pthread_stack_min.h
@@ -0,0 +1,39 @@ 
+/* __get_pthread_stack_min ().  Linux version.
+   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/>.  */
+
+/* Return sysconf (_SC_THREAD_STACK_MIN).  */
+
+static inline long int
+__get_pthread_stack_min (void)
+{
+  /* sysconf (_SC_THREAD_STACK_MIN) >= sysconf (_SC_MINSIGSTKSZ).  */
+  long int pthread_stack_min = GLRO(dl_minsigstacksize) * 4;
+  assert (pthread_stack_min != 0);
+  _Static_assert (__builtin_constant_p (PTHREAD_STACK_MIN),
+		  "PTHREAD_STACK_MIN is constant");
+  /* Return MAX (PTHREAD_STACK_MIN, pthread_stack_min).  */
+  if (pthread_stack_min < PTHREAD_STACK_MIN)
+    pthread_stack_min = PTHREAD_STACK_MIN;
+  /* We have a private interface, __pthread_get_minstack@GLIBC_PRIVATE
+     which returns a larger size that includes the required TLS variable
+     space which has been determined at startup.  For sysconf here we are
+     conservative and don't include the space required for TLS access.
+     Eventually the TLS variable space will not be part of the stack
+     (Bug 11787).  */
+  return pthread_stack_min;
+}