[PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Rajalakshmi Srinivasaraghavan]

This patch increases the value of SIGSTKSZ and MINSIGSTKSZ
for powerpc64 similar to the kernel commit
2f82ec19757f58549467db568c56e7dfff8af283 to allow
further expansion of the signal stack frame size.
---
 sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
index abc87cd7a6..4bff1fe1e7 100644
--- a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
+++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
@@ -23,10 +23,15 @@
 # error "Never include this file directly.  Use <signal.h> instead"
 #endif
 
+#ifdef __powerpc64__
+#define MINSIGSTKSZ	8192
+#define SIGSTKSZ	32768
+#else
 /* Minimum stack size for a signal handler.  */
 #define MINSIGSTKSZ	4096
 
 /* System default stack size.  */
 #define SIGSTKSZ	16384
+#endif
 
 #endif /* bits/sigstack.h */
-- 
2.31.1

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Paul E Murphy]

On 12/8/22 7:16 PM, Rajalakshmi Srinivasaraghavan via Libc-alpha wrote:
> This patch increases the value of SIGSTKSZ and MINSIGSTKSZ
> for powerpc64 similar to the kernel commit
> 2f82ec19757f58549467db568c56e7dfff8af283 to allow
> further expansion of the signal stack frame size.

This LGTM. The kernel's commit message is a little confusing to someone 
who didn't experience the original issue, but make sense once looking 
through the tree of referenced commits.

> ---
>   sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
> index abc87cd7a6..4bff1fe1e7 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
> @@ -23,10 +23,15 @@
>   # error "Never include this file directly.  Use <signal.h> instead"
>   #endif
> 
> +#ifdef __powerpc64__
> +#define MINSIGSTKSZ	8192
> +#define SIGSTKSZ	32768
> +#else
>   /* Minimum stack size for a signal handler.  */
>   #define MINSIGSTKSZ	4096
> 
>   /* System default stack size.  */
>   #define SIGSTKSZ	16384
> +#endif
> 
>   #endif /* bits/sigstack.h */

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Adhemerval Zanella Netto]

On 12/12/22 20:31, Paul E Murphy via Libc-alpha wrote:
> On 12/8/22 7:16 PM, Rajalakshmi Srinivasaraghavan via Libc-alpha wrote:
>> This patch increases the value of SIGSTKSZ and MINSIGSTKSZ
>> for powerpc64 similar to the kernel commit
>> 2f82ec19757f58549467db568c56e7dfff8af283 to allow
>> further expansion of the signal stack frame size.
> 
> This LGTM. The kernel's commit message is a little confusing to someone who didn't experience the original issue, but make sense once looking through the tree of referenced commits.

Shouldn't powerpc follow x86 to use sysconf (_SC_SIGSTKSZ) as well?
Or this is done unconditionally on kernel without it providing a
AT_MINSIGSTKSZ? 

> 
>> ---
>>   sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>> index abc87cd7a6..4bff1fe1e7 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>> @@ -23,10 +23,15 @@
>>   # error "Never include this file directly.  Use <signal.h> instead"
>>   #endif
>>
>> +#ifdef __powerpc64__
>> +#define MINSIGSTKSZ    8192
>> +#define SIGSTKSZ    32768
>> +#else
>>   /* Minimum stack size for a signal handler.  */
>>   #define MINSIGSTKSZ    4096
>>
>>   /* System default stack size.  */
>>   #define SIGSTKSZ    16384
>> +#endif
>>
>>   #endif /* bits/sigstack.h */

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Rajalakshmi Srinivasaraghavan]

On 12/12/22 9:27 PM, Adhemerval Zanella Netto via Libc-alpha wrote:
>
> On 12/12/22 20:31, Paul E Murphy via Libc-alpha wrote:
>> On 12/8/22 7:16 PM, Rajalakshmi Srinivasaraghavan via Libc-alpha wrote:
>>> This patch increases the value of SIGSTKSZ and MINSIGSTKSZ
>>> for powerpc64 similar to the kernel commit
>>> 2f82ec19757f58549467db568c56e7dfff8af283 to allow
>>> further expansion of the signal stack frame size.
>> This LGTM. The kernel's commit message is a little confusing to someone who didn't experience the original issue, but make sense once looking through the tree of referenced commits.
> Shouldn't powerpc follow x86 to use sysconf (_SC_SIGSTKSZ) as well?
> Or this is done unconditionally on kernel without it providing a
> AT_MINSIGSTKSZ?


Yes, We have that in the plan after merging this.  There is a kernel commit
2896b2dff49d0377e4372f470dcddbcb26f2be59 which allows userspace
to fetch AT_MINSIGSTKSZ AUXV entry for powerpc.


>
>>> ---
>>>    sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>>> index abc87cd7a6..4bff1fe1e7 100644
>>> --- a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>>> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>>> @@ -23,10 +23,15 @@
>>>    # error "Never include this file directly.  Use <signal.h> instead"
>>>    #endif
>>>
>>> +#ifdef __powerpc64__
>>> +#define MINSIGSTKSZ    8192
>>> +#define SIGSTKSZ    32768
>>> +#else
>>>    /* Minimum stack size for a signal handler.  */
>>>    #define MINSIGSTKSZ    4096
>>>
>>>    /* System default stack size.  */
>>>    #define SIGSTKSZ    16384
>>> +#endif
>>>
>>>    #endif /* bits/sigstack.h */

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Rajalakshmi Srinivasaraghavan]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]


On 12/12/22 5:31 PM, Paul E Murphy wrote:
> On 12/8/22 7:16 PM, Rajalakshmi Srinivasaraghavan via Libc-alpha wrote:
>> This patch increases the value of SIGSTKSZ and MINSIGSTKSZ
>> for powerpc64 similar to the kernel commit
>> 2f82ec19757f58549467db568c56e7dfff8af283 to allow
>> further expansion of the signal stack frame size.
>
Committed. (e2b68828fab4fdfa5595fa89180230cdc4373ec1)

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Florian Weimer]

* Rajalakshmi Srinivasaraghavan via Libc-alpha:

> This patch increases the value of SIGSTKSZ and MINSIGSTKSZ
> for powerpc64 similar to the kernel commit
> 2f82ec19757f58549467db568c56e7dfff8af283 to allow
> further expansion of the signal stack frame size.
> ---
>  sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
> index abc87cd7a6..4bff1fe1e7 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
> @@ -23,10 +23,15 @@
>  # error "Never include this file directly.  Use <signal.h> instead"
>  #endif
>  
> +#ifdef __powerpc64__
> +#define MINSIGSTKSZ	8192
> +#define SIGSTKSZ	32768
> +#else
>  /* Minimum stack size for a signal handler.  */
>  #define MINSIGSTKSZ	4096
>  
>  /* System default stack size.  */
>  #define SIGSTKSZ	16384
> +#endif

This is technically an ABI change, but if you think this is the right
forward, you can certainly make that change.  It's not the path some
other architectures have taken when they faced similar problems, so I
don't think we have much experience in this area.

Should we backport this to older releases (or downstream)?

Thanks,
Florian

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Rajalakshmi Srinivasaraghavan]

On 12/13/22 10:23 AM, Florian Weimer wrote:
> * Rajalakshmi Srinivasaraghavan via Libc-alpha:
>
>> This patch increases the value of SIGSTKSZ and MINSIGSTKSZ
>> for powerpc64 similar to the kernel commit
>> 2f82ec19757f58549467db568c56e7dfff8af283 to allow
>> further expansion of the signal stack frame size.
>> ---
>>   sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>> index abc87cd7a6..4bff1fe1e7 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h
>> @@ -23,10 +23,15 @@
>>   # error "Never include this file directly.  Use <signal.h> instead"
>>   #endif
>>   
>> +#ifdef __powerpc64__
>> +#define MINSIGSTKSZ	8192
>> +#define SIGSTKSZ	32768
>> +#else
>>   /* Minimum stack size for a signal handler.  */
>>   #define MINSIGSTKSZ	4096
>>   
>>   /* System default stack size.  */
>>   #define SIGSTKSZ	16384
>> +#endif
> This is technically an ABI change, but if you think this is the right
> forward, you can certainly make that change.  It's not the path some
> other architectures have taken when they faced similar problems, so I
> don't think we have much experience in this area.
>
> Should we backport this to older releases (or downstream)?

Yes, This should be backported to older releases as well.


>
> Thanks,
> Florian
>

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Tulio Magno Quites Machado Filho]

Rajalakshmi Srinivasaraghavan via Libc-alpha <libc-alpha@sourceware.org>
writes:

> On 12/13/22 10:23 AM, Florian Weimer wrote:

>> This is technically an ABI change, but if you think this is the right
>> forward, you can certainly make that change.  It's not the path some
>> other architectures have taken when they faced similar problems, so I
>> don't think we have much experience in this area.

I do agree this can be interpreted as an ABI change.
However, I used abidiff against the following libraries and it didn't
report any changes:

hesiod/libnss_hesiod.so.2
nis/libnsl.so.1
login/libutil.so.1
libc.so.6
crypt/libcrypt.so.1
locale/libBrokenLocale.so.1
nptl_db/libthread_db.so.1
math/libm.so.6
resolv/libanl.so.1
resolv/libresolv.so.2
resolv/libnss_dns.so.2
nss/libnss_files.so.2
nss/libnss_compat.so.2
nss/libnss_db.so.2
nss/libnss_test1.so.2
nss/libnss_test_errno.so.2
nss/libnss_test2.so.2
dlfcn/libdl.so.2
nptl/libpthread.so.0
rt/librt.so.1
malloc/libc_malloc_debug.so.0

So, I think this change is safe for glibc 2.37.

>> Should we backport this to older releases (or downstream)?
>
> Yes, This should be backported to older releases as well.

IMHO, this is trickier. Not because a glibc binary interface will change,
but because user software might change.
Anyway, the kernel >= 5.19 is already requesting the new sizes, although
it isn't using it to the best of my knowledge.

Maybe we should be extra careful and avoid backports upstream?
That would leave for downstream to apply this patch as necessary
(ideally, before a mass rebuild).

Am I missing anything?

-- 
Tulio Magno

Re: [PATCH] powerpc64: Increase SIGSTKSZ and MINSIGSTKSZ

[Rajalakshmi Srinivasaraghavan]

On 12/15/22 7:43 AM, Tulio Magno Quites Machado Filho wrote:
> Rajalakshmi Srinivasaraghavan via Libc-alpha <libc-alpha@sourceware.org>
> writes:
>
>> On 12/13/22 10:23 AM, Florian Weimer wrote:
>>> This is technically an ABI change, but if you think this is the right
>>> forward, you can certainly make that change.  It's not the path some
>>> other architectures have taken when they faced similar problems, so I
>>> don't think we have much experience in this area.
> I do agree this can be interpreted as an ABI change.
> However, I used abidiff against the following libraries and it didn't
> report any changes:
>
> hesiod/libnss_hesiod.so.2
> nis/libnsl.so.1
> login/libutil.so.1
> libc.so.6
> crypt/libcrypt.so.1
> locale/libBrokenLocale.so.1
> nptl_db/libthread_db.so.1
> math/libm.so.6
> resolv/libanl.so.1
> resolv/libresolv.so.2
> resolv/libnss_dns.so.2
> nss/libnss_files.so.2
> nss/libnss_compat.so.2
> nss/libnss_db.so.2
> nss/libnss_test1.so.2
> nss/libnss_test_errno.so.2
> nss/libnss_test2.so.2
> dlfcn/libdl.so.2
> nptl/libpthread.so.0
> rt/librt.so.1
> malloc/libc_malloc_debug.so.0
>
> So, I think this change is safe for glibc 2.37.

Thanks for checking this.

>
>>> Should we backport this to older releases (or downstream)?
>> Yes, This should be backported to older releases as well.
> IMHO, this is trickier. Not because a glibc binary interface will change,
> but because user software might change.
> Anyway, the kernel >= 5.19 is already requesting the new sizes, although
> it isn't using it to the best of my knowledge.
>
> Maybe we should be extra careful and avoid backports upstream?
> That would leave for downstream to apply this patch as necessary
> (ideally, before a mass rebuild).

I agree that downstream will be a safer approach.

>
> Am I missing anything?
>