* [PATCH v3 2/4] libsupport: Add support_select_normalizes_timeout
2021-04-09 15:53 [PATCH v3 1/4] libsupport: Add support_select_modifies_timeout Adhemerval Zanella
@ 2021-04-09 15:53 ` Adhemerval Zanella
2021-04-09 16:24 ` Florian Weimer
2021-04-09 15:53 ` [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651) Adhemerval Zanella
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2021-04-09 15:53 UTC (permalink / raw)
To: libc-alpha
It will be used on a select() test.
---
support/Makefile | 1 +
support/support.h | 4 +++
support/support_select_normalizes_timeout.c | 29 +++++++++++++++++++++
3 files changed, 34 insertions(+)
create mode 100644 support/support_select_normalizes_timeout.c
diff --git a/support/Makefile b/support/Makefile
index 91457d8613..da6dc58d37 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -69,6 +69,7 @@ libsupport-routines = \
support_record_failure \
support_run_diff \
support_select_modifies_timeout \
+ support_select_normalizes_timeout \
support_set_small_thread_stack_size \
support_shared_allocate \
support_small_stack_thread_attribute \
diff --git a/support/support.h b/support/support.h
index f730611841..db264e3db7 100644
--- a/support/support.h
+++ b/support/support.h
@@ -148,6 +148,10 @@ extern bool support_stat_nanoseconds (const char *path);
no slept. */
extern bool support_select_modifies_timeout (void);
+/* Return true if select normalize the timeout input by taking in account
+ tv_usec larger than 1000000. */
+extern bool support_select_normalizes_timeout (void);
+
__END_DECLS
#endif /* SUPPORT_H */
diff --git a/support/support_select_normalizes_timeout.c b/support/support_select_normalizes_timeout.c
new file mode 100644
index 0000000000..5dcf5dcbfc
--- /dev/null
+++ b/support/support_select_normalizes_timeout.c
@@ -0,0 +1,29 @@
+/* Return whether select normalizes the timeout.
+ 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/>. */
+
+#include <stdbool.h>
+
+bool
+support_select_normalizes_timeout (void)
+{
+#ifdef __linux__
+ return true;
+#else
+ return false;
+#endif
+}
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 2/4] libsupport: Add support_select_normalizes_timeout
2021-04-09 15:53 ` [PATCH v3 2/4] libsupport: Add support_select_normalizes_timeout Adhemerval Zanella
@ 2021-04-09 16:24 ` Florian Weimer
0 siblings, 0 replies; 10+ messages in thread
From: Florian Weimer @ 2021-04-09 16:24 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> diff --git a/support/support_select_normalizes_timeout.c b/support/support_select_normalizes_timeout.c
> new file mode 100644
> index 0000000000..5dcf5dcbfc
> --- /dev/null
> +++ b/support/support_select_normalizes_timeout.c
> @@ -0,0 +1,29 @@
> +/* Return whether select normalizes the timeout.
> + 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/>. */
> +
> +#include <stdbool.h>
Likewise <support/support.h>
Okay with that change.
Thanks,
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651)
2021-04-09 15:53 [PATCH v3 1/4] libsupport: Add support_select_modifies_timeout Adhemerval Zanella
2021-04-09 15:53 ` [PATCH v3 2/4] libsupport: Add support_select_normalizes_timeout Adhemerval Zanella
@ 2021-04-09 15:53 ` Adhemerval Zanella
2021-04-12 8:28 ` Andreas Schwab
2021-04-09 15:53 ` [PATCH v3 4/4] linux: always update select timeout (BZ #27706) Adhemerval Zanella
2021-04-09 16:23 ` [PATCH v3 1/4] libsupport: Add support_select_modifies_timeout Florian Weimer
3 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2021-04-09 15:53 UTC (permalink / raw)
To: libc-alpha
The commit 2433d39b697, which added time64 support to select, changed
the function to use __NR_pselect6 (or __NR_pelect6_time64) on all
architectures. However, on architectures where the symbol was
implemented with __NR_select the kernel normalizes the passed timeout
instead of return EINVAL. For instance, the input timeval
{ 0, 5000000 } is interpreted as { 5, 0 }.
And as indicated by BZ #27651, this semantic seems to be expected
and changing it results in some performance issues (most likely
the program does not check the return code and keeps issuing
select with unormalized tv_usec argument).
To avoid a different semantic depending whether which syscall the
architecture used to issue, select now always normalize the timeout
input. This is a slight change for some ABIs (for instance aarch64).
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
include/time.h | 5 ++++
misc/tst-select.c | 17 ++++++++++++
sunrpc/svcauth_des.c | 1 -
sysdeps/unix/sysv/linux/select.c | 44 +++++++++++++++++++++++++-------
4 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/include/time.h b/include/time.h
index caf2af5e74..e0636132a6 100644
--- a/include/time.h
+++ b/include/time.h
@@ -502,6 +502,11 @@ time_now (void)
__clock_gettime (TIME_CLOCK_GETTIME_CLOCKID, &ts);
return ts.tv_sec;
}
+
+#define NSEC_PER_SEC 1000000000L /* Nanoseconds per second. */
+#define USEC_PER_SEC 1000000L /* Microseconds per second. */
+#define NSEC_PER_USEC 1000L /* Nanoseconds per microsecond. */
+
#endif
#endif
diff --git a/misc/tst-select.c b/misc/tst-select.c
index 5ad057cd51..534105b500 100644
--- a/misc/tst-select.c
+++ b/misc/tst-select.c
@@ -19,6 +19,7 @@
#include <errno.h>
#include <support/capture_subprocess.h>
#include <support/check.h>
+#include <support/support.h>
#include <support/timespec.h>
#include <support/xunistd.h>
#include <support/xtime.h>
@@ -47,6 +48,12 @@ do_test_child (void *clousure)
int r = select (args->fds[0][0] + 1, &rfds, NULL, NULL, &args->tmo);
TEST_COMPARE (r, 0);
+ if (support_select_modifies_timeout ())
+ {
+ TEST_COMPARE (args->tmo.tv_sec, 0);
+ TEST_COMPARE (args->tmo.tv_usec, 0);
+ }
+
TEST_TIMESPEC_NOW_OR_AFTER (CLOCK_REALTIME, ts);
xwrite (args->fds[1][1], "foo", 3);
@@ -69,6 +76,16 @@ do_test (void)
sc_allow_none);
}
+ if (support_select_normalizes_timeout ())
+ {
+ /* This is handled as 1 second instead of failing with EINVAL. */
+ args.tmo = (struct timeval) { .tv_sec = 0, .tv_usec = 1000000 };
+ struct support_capture_subprocess result;
+ result = support_capture_subprocess (do_test_child, &args);
+ support_capture_subprocess_check (&result, "tst-select-child", 0,
+ sc_allow_none);
+ }
+
/* Same as before, but simulating polling. */
args.tmo = (struct timeval) { .tv_sec = 0, .tv_usec = 0 };
{
diff --git a/sunrpc/svcauth_des.c b/sunrpc/svcauth_des.c
index 7607abc818..25a85c9097 100644
--- a/sunrpc/svcauth_des.c
+++ b/sunrpc/svcauth_des.c
@@ -58,7 +58,6 @@
#define debug(msg) /*printf("svcauth_des: %s\n", msg) */
-#define USEC_PER_SEC ((uint32_t) 1000000L)
#define BEFORE(t1, t2) timercmp(t1, t2, <)
/*
diff --git a/sysdeps/unix/sysv/linux/select.c b/sysdeps/unix/sysv/linux/select.c
index 415aa87d3c..8deab751c0 100644
--- a/sysdeps/unix/sysv/linux/select.c
+++ b/sysdeps/unix/sysv/linux/select.c
@@ -33,12 +33,34 @@ int
__select64 (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
struct __timeval64 *timeout)
{
- struct __timespec64 ts64, *pts64 = NULL;
- if (timeout != NULL)
+ __time64_t s = timeout != NULL ? timeout->tv_sec : 0;
+ int32_t us = timeout != NULL ? timeout->tv_usec : 0;
+ int32_t ns;
+
+ if (s < 0 || us < 0)
+ return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
+
+ /* Normalize the timeout, as legacy Linux __NR_select and __NR__newselect.
+ Different than syscall, it also handle possible overflow. */
+ if (us / USEC_PER_SEC > INT64_MAX - s)
{
- ts64 = timeval64_to_timespec64 (*timeout);
- pts64 = &ts64;
+ s = INT64_MAX;
+ ns = NSEC_PER_SEC - 1;
}
+ else
+ {
+ s += us / USEC_PER_SEC;
+ us = us % USEC_PER_SEC;
+ ns = us * NSEC_PER_USEC;
+ }
+
+ struct __timespec64 ts64, *pts64 = NULL;
+ if (timeout != NULL)
+ {
+ ts64.tv_sec = s;
+ ts64.tv_nsec = ns;
+ pts64 = &ts64;
+ }
#ifndef __NR_pselect6_time64
# define __NR_pselect6_time64 __NR_pselect6
@@ -52,10 +74,13 @@ __select64 (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
(though the pselect() glibc call suppresses this behavior).
Since select() on Linux has the same behavior as the pselect6
syscall, we update the timeout here. */
- if (r == 0 || errno != ENOSYS)
+ if (r >= 0 || errno != ENOSYS)
{
if (timeout != NULL)
- TIMEVAL_TO_TIMESPEC (timeout, &ts64);
+ {
+ timeout->tv_sec = ts64.tv_sec;
+ timeout->tv_usec = ts64.tv_nsec / NSEC_PER_USEC;
+ }
return r;
}
@@ -64,14 +89,15 @@ __select64 (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
#ifndef __ASSUME_TIME64_SYSCALLS
struct timespec ts32, *pts32 = NULL;
- if (timeout != NULL)
+ if (pts64 != NULL)
{
- if (! in_time_t_range (timeout->tv_sec))
+ if (! in_time_t_range (pts64->tv_sec))
{
__set_errno (EINVAL);
return -1;
}
- ts32 = valid_timespec64_to_timespec (ts64);
+ ts32.tv_sec = s;
+ ts32.tv_nsec = ns;
pts32 = &ts32;
}
# ifndef __ASSUME_PSELECT
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651)
2021-04-09 15:53 ` [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651) Adhemerval Zanella
@ 2021-04-12 8:28 ` Andreas Schwab
2021-04-12 12:20 ` Adhemerval Zanella
0 siblings, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2021-04-12 8:28 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
On Apr 09 2021, Adhemerval Zanella via Libc-alpha wrote:
> @@ -52,10 +74,13 @@ __select64 (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> (though the pselect() glibc call suppresses this behavior).
> Since select() on Linux has the same behavior as the pselect6
> syscall, we update the timeout here. */
> - if (r == 0 || errno != ENOSYS)
> + if (r >= 0 || errno != ENOSYS)
> {
> if (timeout != NULL)
> - TIMEVAL_TO_TIMESPEC (timeout, &ts64);
> + {
> + timeout->tv_sec = ts64.tv_sec;
> + timeout->tv_usec = ts64.tv_nsec / NSEC_PER_USEC;
> + }
What's wrong with TIMESPEC_TO_TIMEVAL?
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651)
2021-04-12 8:28 ` Andreas Schwab
@ 2021-04-12 12:20 ` Adhemerval Zanella
2021-04-12 18:26 ` Adhemerval Zanella
0 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2021-04-12 12:20 UTC (permalink / raw)
To: Andreas Schwab, Adhemerval Zanella via Libc-alpha
On 12/04/2021 05:28, Andreas Schwab wrote:
> On Apr 09 2021, Adhemerval Zanella via Libc-alpha wrote:
>
>> @@ -52,10 +74,13 @@ __select64 (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>> (though the pselect() glibc call suppresses this behavior).
>> Since select() on Linux has the same behavior as the pselect6
>> syscall, we update the timeout here. */
>> - if (r == 0 || errno != ENOSYS)
>> + if (r >= 0 || errno != ENOSYS)
>> {
>> if (timeout != NULL)
>> - TIMEVAL_TO_TIMESPEC (timeout, &ts64);
>> + {
>> + timeout->tv_sec = ts64.tv_sec;
>> + timeout->tv_usec = ts64.tv_nsec / NSEC_PER_USEC;
>> + }
>
> What's wrong with TIMESPEC_TO_TIMEVAL?
>
> Andreas.
>
None in fact, I will adjust to use TIMESPEC_TO_TIMEVAL.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651)
2021-04-12 12:20 ` Adhemerval Zanella
@ 2021-04-12 18:26 ` Adhemerval Zanella
2021-04-12 19:20 ` Andreas Schwab
0 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2021-04-12 18:26 UTC (permalink / raw)
To: Andreas Schwab, Adhemerval Zanella via Libc-alpha
On 12/04/2021 09:20, Adhemerval Zanella wrote:
>
>
> On 12/04/2021 05:28, Andreas Schwab wrote:
>> On Apr 09 2021, Adhemerval Zanella via Libc-alpha wrote:
>>
>>> @@ -52,10 +74,13 @@ __select64 (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>>> (though the pselect() glibc call suppresses this behavior).
>>> Since select() on Linux has the same behavior as the pselect6
>>> syscall, we update the timeout here. */
>>> - if (r == 0 || errno != ENOSYS)
>>> + if (r >= 0 || errno != ENOSYS)
>>> {
>>> if (timeout != NULL)
>>> - TIMEVAL_TO_TIMESPEC (timeout, &ts64);
>>> + {
>>> + timeout->tv_sec = ts64.tv_sec;
>>> + timeout->tv_usec = ts64.tv_nsec / NSEC_PER_USEC;
>>> + }
>>
>> What's wrong with TIMESPEC_TO_TIMEVAL?
>>
>> Andreas.
>>
>
> None in fact, I will adjust to use TIMESPEC_TO_TIMEVAL.
>
Are you ok with the patch with the TIMESPEC_TO_TIMEVAL change?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651)
2021-04-12 18:26 ` Adhemerval Zanella
@ 2021-04-12 19:20 ` Andreas Schwab
0 siblings, 0 replies; 10+ messages in thread
From: Andreas Schwab @ 2021-04-12 19:20 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
On Apr 12 2021, Adhemerval Zanella wrote:
> Are you ok with the patch with the TIMESPEC_TO_TIMEVAL change?
Yes.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 4/4] linux: always update select timeout (BZ #27706)
2021-04-09 15:53 [PATCH v3 1/4] libsupport: Add support_select_modifies_timeout Adhemerval Zanella
2021-04-09 15:53 ` [PATCH v3 2/4] libsupport: Add support_select_normalizes_timeout Adhemerval Zanella
2021-04-09 15:53 ` [PATCH v3 3/4] linux: Normalize and return timeout on select (BZ #27651) Adhemerval Zanella
@ 2021-04-09 15:53 ` Adhemerval Zanella
2021-04-09 16:23 ` [PATCH v3 1/4] libsupport: Add support_select_modifies_timeout Florian Weimer
3 siblings, 0 replies; 10+ messages in thread
From: Adhemerval Zanella @ 2021-04-09 15:53 UTC (permalink / raw)
To: libc-alpha
The timeout should be updated even on failure for time64 support.
Checked on i686-linux-gnu.
---
misc/tst-select.c | 30 ++++++++++++++++++++++++++++++
sysdeps/unix/sysv/linux/select.c | 4 ++--
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/misc/tst-select.c b/misc/tst-select.c
index 534105b500..52aa26651f 100644
--- a/misc/tst-select.c
+++ b/misc/tst-select.c
@@ -23,6 +23,7 @@
#include <support/timespec.h>
#include <support/xunistd.h>
#include <support/xtime.h>
+#include <support/xsignal.h>
struct child_args
{
@@ -30,6 +31,12 @@ struct child_args
struct timeval tmo;
};
+static void
+alarm_handler (int signum)
+{
+ /* Do nothing. */
+}
+
static void
do_test_child (void *clousure)
{
@@ -59,6 +66,22 @@ do_test_child (void *clousure)
xwrite (args->fds[1][1], "foo", 3);
}
+static void
+do_test_child_alarm (void *clousure)
+{
+ struct sigaction act = { .sa_handler = alarm_handler };
+ xsigaction (SIGALRM, &act, NULL);
+ alarm (1);
+
+ struct timeval tv = { .tv_sec = 10, .tv_usec = 0 };
+ int r = select (0, NULL, NULL, NULL, &tv);
+ TEST_COMPARE (r, -1);
+ TEST_COMPARE (errno, EINTR);
+
+ if (support_select_modifies_timeout ())
+ TEST_VERIFY (tv.tv_sec < 10);
+}
+
static int
do_test (void)
{
@@ -98,6 +121,13 @@ do_test (void)
xclose (args.fds[0][0]);
xclose (args.fds[1][1]);
+ {
+ struct support_capture_subprocess result;
+ result = support_capture_subprocess (do_test_child_alarm, NULL);
+ support_capture_subprocess_check (&result, "tst-select-child", 0,
+ sc_allow_none);
+ }
+
{
fd_set rfds;
FD_ZERO (&rfds);
diff --git a/sysdeps/unix/sysv/linux/select.c b/sysdeps/unix/sysv/linux/select.c
index 8deab751c0..5d87a3d1f9 100644
--- a/sysdeps/unix/sysv/linux/select.c
+++ b/sysdeps/unix/sysv/linux/select.c
@@ -110,7 +110,7 @@ __select64 (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
r = SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds, pts32,
NULL);
# endif
- if (r >= 0 && timeout != NULL)
+ if (timeout != NULL)
*timeout = valid_timespec_to_timeval64 (ts32);
#endif
@@ -131,7 +131,7 @@ __select (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
ptv64 = &tv64;
}
int r = __select64 (nfds, readfds, writefds, exceptfds, ptv64);
- if (r >= 0 && timeout != NULL)
+ if (timeout != NULL)
/* The remanining timeout will be always less the input TIMEOUT. */
*timeout = valid_timeval64_to_timeval (tv64);
return r;
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 1/4] libsupport: Add support_select_modifies_timeout
2021-04-09 15:53 [PATCH v3 1/4] libsupport: Add support_select_modifies_timeout Adhemerval Zanella
` (2 preceding siblings ...)
2021-04-09 15:53 ` [PATCH v3 4/4] linux: always update select timeout (BZ #27706) Adhemerval Zanella
@ 2021-04-09 16:23 ` Florian Weimer
3 siblings, 0 replies; 10+ messages in thread
From: Florian Weimer @ 2021-04-09 16:23 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> diff --git a/support/support_select_modifies_timeout.c b/support/support_select_modifies_timeout.c
> new file mode 100644
> index 0000000000..a4f3bf145c
> --- /dev/null
> +++ b/support/support_select_modifies_timeout.c
> @@ -0,0 +1,29 @@
> +/* Return whether select modify the timeout.
“modifies”
> + 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/>. */
> +
> +#include <stdbool.h>
Sorry for not catching this early. This should be <support/support.h>.
Okay with these changes.
Thanks,
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread