Re: [PATCH] powerpc: Use sysconf (_SC_SIGSTKSZ) to set SIGSTKSZ and MINSIGSTKSZ.

[Manjunath S Matti <mmatti@linux.vnet.ibm.com>]

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


On 28/04/23 11:35 pm, Tulio Magno Quites Machado Filho wrote:
> Manjunath Matti via Libc-alpha<libc-alpha@sourceware.org>  writes:
>
>> Add support in PowerPC to use sysconf (_SC_SIGSTKSZ) to set SIGSTKSZ
>> and MINSIGSTKSZ similar to x86.
> This commit message explains what is being done, but it doesn't make it
> clear why this commit is important.
> If I understand correctly, the goal is to have dynamic values for the
> signal stack size and minimum signal stack size. Is this correct?

Yes that is correct,

file: sysdeps/unix/sysv/linux/dl-parse_auxv.h

  52   _dl_random = (void *) auxv_values[AT_RANDOM];
  53   GLRO(dl_minsigstacksize) = auxv_values[AT_MINSIGSTKSZ]; <-- gets 
the value from kernel.
  54   GLRO(dl_sysinfo_dso) = (void *) auxv_values[AT_SYSINFO_EHDR];

> I also suggest to mention the kernel commit ID that started doing this:
> 2896b2dff49d0377e4372f470dcddbcb26f2be59
Yes I will add it.
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
>> new file mode 100644
>> index 0000000000..2bec1e7917
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
>> @@ -0,0 +1,33 @@
>> +/* Definition of MINSIGSTKSZ and SIGSTKSZ.  Linux/PowerPC version.
>> +   Copyright (C) 2020-2023 Free Software Foundation, Inc.
>                      ^ I believe this should be just 2023.
Ack.
>
>> +#if defined __USE_DYNAMIC_STACK_SIZE && __USE_DYNAMIC_STACK_SIZE
>> +# include <unistd.h>
>> +
>> +/* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
>> +# undef SIGSTKSZ
>> +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
> I have been told that structs could be using SIGSTKSZ to set their size.
> If that's happening, the softwares using them will stop building after this
> change.
> Is this reasonable?
>
>> +/* Minimum stack size for a signal handler: SIGSTKSZ/4.  */
>> +# undef MINSIGSTKSZ
>> +# define MINSIGSTKSZ (SIGSTKSZ >> 2)
>> +#endif
> I didn't understand this part.
> Why SIGSTKSZ/4 ? I know this is correct now, but I think the kernel is
> allowed to use another value.
> Why is this part not using sysconf(_SC_MINSIGSTKSZ)?
> I'm not suggesting to use sysconf() here, but I'm trying to understand
> why the same source of value for both SIGSTKSZ and MINSIGSTKSZ is not
> being used.
In file: sysdeps/unix/sysv/linux/sysconf-sigstksz.h

  28   if (minsigstacksize < MINSIGSTKSZ)
  29     minsigstacksize = MINSIGSTKSZ;
  30   /* MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4.  */
  31   long int sigstacksize = minsigstacksize * 4;

So we are not changing the default implementation.

> If we reach consensus that both macros in this file can have values set
> at runtime, then I it might be worth adding a test in order to check that
> dl_minsigstacksize, MINSIGSTKSZ and AT_MINSIGSTKSZ passed by the kernel
> are identical.
>
There are testcases which already use MINSIGSTKSZ

sysdeps/pthread/tst-signal6.c

signal/tst-minsigstksz-5.c


I will resubmit my patch with the changes I have acknowledged.


Thanks for your time and support.

Manjunath Matti.