* [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+
@ 2021-09-22 20:13 Arnout Vandecappelle
2021-09-23 8:23 ` David Laight
0 siblings, 1 reply; 5+ messages in thread
From: Arnout Vandecappelle @ 2021-09-22 20:13 UTC (permalink / raw)
To: buildroot
commit: https://git.buildroot.net/buildroot/commit/?id=44d5f1d05d9cf9dc03af22ae32d31cb3f647a339
branch: https://git.buildroot.net/buildroot/commit/?id=refs/heads/master
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].
Original fix proposed by Khem Raj in OpenEmbedded, see [2].
[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=5d98a7dae955bafa6740c26eaba9c86060ae0344
[2] https://github.com/openembedded/meta-openembedded/commit/7055c764c83150f9310ce04bcfb19330460582fc
Suggested-by: Artem Panfilov <artemp@synopsys.com>
Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
...ntp-Do-not-use-PTHREAD_STACK_MIN-on-glibc.patch | 33 ++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/package/ntp/0004-libntp-Do-not-use-PTHREAD_STACK_MIN-on-glibc.patch b/package/ntp/0004-libntp-Do-not-use-PTHREAD_STACK_MIN-on-glibc.patch
new file mode 100644
index 0000000000..da03ad4aac
--- /dev/null
+++ b/package/ntp/0004-libntp-Do-not-use-PTHREAD_STACK_MIN-on-glibc.patch
@@ -0,0 +1,33 @@
+From 082a504cfcc046c3d8adaae1164268bc94e5108a Mon Sep 17 00:00:00 2001
+From: Khem Raj <raj.khem@gmail.com>
+Date: Sat, 31 Jul 2021 10:51:41 -0700
+Subject: [PATCH] libntp: Do not use PTHREAD_STACK_MIN on glibc
+
+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.
+
+Signed-off-by: Khem Raj <raj.khem@gmail.com>
+[Copied from https://github.com/openembedded/meta-openembedded/blob/master/meta-networking/recipes-support/ntp/ntp/0001-libntp-Do-not-use-PTHREAD_STACK_MIN-on-glibc.patch]
+Signed-off-by: Alexey Brodkin <abrokdin@synopsys.com>
+---
+ libntp/work_thread.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/libntp/work_thread.c b/libntp/work_thread.c
+index 03a5647..3ddd751 100644
+--- a/libntp/work_thread.c
++++ b/libntp/work_thread.c
+@@ -41,7 +41,7 @@
+ #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
+--
+2.32.0
+
_______________________________________________
buildroot mailing list
buildroot@lists.buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+
2021-09-22 20:13 [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+ Arnout Vandecappelle
@ 2021-09-23 8:23 ` David Laight
2021-09-23 9:26 ` Arnout Vandecappelle
0 siblings, 1 reply; 5+ messages in thread
From: David Laight @ 2021-09-23 8:23 UTC (permalink / raw)
To: 'Arnout Vandecappelle', buildroot
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.
Quite why ntp is setting the thread stack size is any bodies guess though.
It can only get it wrong in a different way!
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.
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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+
2021-09-23 8:23 ` David Laight
@ 2021-09-23 9:26 ` Arnout Vandecappelle
2021-09-23 21:34 ` Peter Seiderer
0 siblings, 1 reply; 5+ messages in thread
From: Arnout Vandecappelle @ 2021-09-23 9:26 UTC (permalink / raw)
To: David Laight, buildroot
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...
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+
2021-09-23 9:26 ` Arnout Vandecappelle
@ 2021-09-23 21:34 ` Peter Seiderer
2021-10-05 6:27 ` Peter Korsgaard
0 siblings, 1 reply; 5+ messages in thread
From: Peter Seiderer @ 2021-09-23 21:34 UTC (permalink / raw)
To: Arnout Vandecappelle; +Cc: David Laight, buildroot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+
2021-09-23 21:34 ` Peter Seiderer
@ 2021-10-05 6:27 ` Peter Korsgaard
0 siblings, 0 replies; 5+ messages in thread
From: Peter Korsgaard @ 2021-10-05 6:27 UTC (permalink / raw)
To: Peter Seiderer; +Cc: David Laight, buildroot
>>>>> "Peter" == Peter Seiderer <ps.report@gmx.net> writes:
Hi,
>> 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/
Feel free to send a patch ;)
Alternatively, chrony is quite good (and already available).
--
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-05 6:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 20:13 [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+ Arnout Vandecappelle
2021-09-23 8:23 ` David Laight
2021-09-23 9:26 ` Arnout Vandecappelle
2021-09-23 21:34 ` Peter Seiderer
2021-10-05 6:27 ` Peter Korsgaard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.