Re: [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+

[Peter Seiderer <ps.report@gmx.net>]

Hello Arnout, David, all,

On Thu, 23 Sep 2021 11:26:18 +0200, Arnout Vandecappelle <arnout@mind.be> wrote:

> On 23/09/2021 10:23, David Laight wrote:
> > From: Arnout Vandecappelle
> >> Sent: 22 September 2021 21:13
> > ...
> >> On attempt to build ntp with glibc 2.34 the following error happens:
> >> -------------------------------->8------------------------------
> >> In file included from .../output/host/lib/gcc/i586-buildroot-linux-gnu/10.3.0/include-
> >> fixed/pthread.h:42,
> >>                   from work_thread.c:13:
> >> work_thread.c:45:57: error: missing binary operator before token "("
> >>     45 | #if defined(PTHREAD_STACK_MIN) && THREAD_MINSTACKSIZE < PTHREAD_STACK_MIN
> >>        |                                                         ^~~~~~~~~~~~~~~~~
> >> -------------------------------->8------------------------------
> >>
> >> That's because starting from glibc 2.34 PTHREAD_STACK_MIN gets determined
> >> dynamically in runtime via sysconf(), see [1].
> > ...
> >> +In glibc 2.34+ PTHREAD_STACK_MIN is not a compile-time constant which
> >> +could mean different stack sizes at runtime on different architectures
> >> +and it also causes compile failure. Default glibc thread stack size
> >> +or 64Kb set by ntp should be good in glibc these days.
> > ...
> >> + #ifndef THREAD_MINSTACKSIZE
> >> + # define THREAD_MINSTACKSIZE	(64U * 1024)
> >> + #endif
> >> +-#ifndef __sun
> >> ++#if !defined(__sun) && !defined(__GLIBC__)
> >> + #if defined(PTHREAD_STACK_MIN) && THREAD_MINSTACKSIZE < PTHREAD_STACK_MIN
> >> + # undef THREAD_MINSTACKSIZE
> >> + # define THREAD_MINSTACKSIZE PTHREAD_STACK_MIN
> >
> > I'm not convinced that is the correct fix.
> > The same issues apply to the thread stack as they do to the signal stack.
> > Albeit the sizes are larger.
> >
> > The very reason that PTHREAD_STACK_MIN isn't a compile-time constant
> > is that the value cannot be determined at compile time.
> > While 64k may seem ok for now, there is no reason for it to be enough.
> >
> > Possibly something like:
> > #ifndef PTHREAD_STACK_MIN
> > #define THREAD_MINSTACKSIZE	(64U * 1024)
> > #else
> > #define THREAD_MINSTACKSIZE	(PTHREAD_STACK_MIN > 64U * 1024 ? PTHREAD_STACK_MIN : 64U * 1024)
> > #endif
> > would be better.
> > Although a form that only evaluates PTHREAD_STACK_MIN once may be better.
>
>   It really doesn't matter that much. The only place where THREAD_MINSTACKSIZE
> is used is in the code leading up to this:
>
>                  if (nstacksize != ostacksize)
>                          rc = pthread_attr_setstacksize(&thr_attr, nstacksize);
>                  if (0 != rc)
>                          msyslog(LOG_ERR,
>                                  "start_blocking_thread:
> pthread_attr_setstacksize(0x%lx -> 0x%lx) -> %s",
>                                  (u_long)ostacksize, (u_long)nstacksize,
>                                  strerror(rc));
>
>   Note that there's no error handling other than printing an error message. So,
> if PTHREAD_STACK_MIN turns out to be less than 64K (which is very, very unlikely
> to be the case), then the only thing that will happen is that
> pthread_attr_setstacksize will fail to set the stack size to a lower value and
> we keep the default stack size (and print an error message).
>
>   I do agree however that your alternative fix is better (though Id keep the
> existing __sun ifdeffery, just to make the change minimal). So feel free to send
> a patch that changes the existing patch.
>
>
> > Quite why ntp is setting the thread stack size is any bodies guess though.
>
>   I guess because there are some threads that need more than 4K stack (which is
> minstacksize on some platforms), and on the other hand they don't want to waste
> stack space on threads that really don't need that much.
>
> > It can only get it wrong in a different way!
>
>   For relatively small code, it's not that hard to measure or calculate the
> amount of stack you need.
>
> > Although ISTR one version of ntp thinking itself so important
> > that it locked all its memory.
> > So maybe it is just trying to reduce that locked memory footprint.
>
>   I do indeed see an mlockall() in ntpd.c, but I don't see any code doing any
> prefaulting of the stack, so it doesn't seem to be very effective :-)
>
>
> > Given that it's pretty much impossible for ntp to accurately set
> > the time to less than the RTT to the server (it can't tell whether
> > the RTT delays are on the tx or rx side) I can't see that it matters.
>
>   Yeah, it looks like ntp is a pretty dead-end project...

Maybe time to provide NTPsec [1] as alternative...

Regards,
Peter

[1] https://www.ntpsec.org/

>
>   Regards,
>   Arnout
>
> >
> > Anyone who needs sub-ms time sync has bigger problems.
> >
> > 	David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >
> _______________________________________________
> buildroot mailing list
> buildroot@lists.buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

_______________________________________________
buildroot mailing list
buildroot@lists.buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot