From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x834.google.com (mail-qt1-x834.google.com [IPv6:2607:f8b0:4864:20::834]) by sourceware.org (Postfix) with ESMTPS id 0D2813986C1C for ; Thu, 17 Jun 2021 17:45:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0D2813986C1C Received: by mail-qt1-x834.google.com with SMTP id v6so5382069qta.9 for ; Thu, 17 Jun 2021 10:45:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=UKurNs+xc9rDJc02Smf8e/KjBogJPl7rRmn0wGeoP/Y=; b=Q/GcnkR69N8fegTAwA6w3J6T898T4QMFvGnN/hFIpN60h23wiKlnmQmrH7Qsmgo0Pn PHSWYXtzrfBJ/3qvsBKhTxDXKQts+jgty2EPrY4suCxz95qBHfeDZbgUdjH/SUd0agj6 O4dWgOzuLLsdKmA8Y7qlD2MPi0HGErIQT3c+n2CgyQSQOZk80ajp7ueynr80zx+RF83x TSGRBzb677EFH71a4VB1iTL/1RBhOVDSSTII745jbVsyA0iNyGtwO/bXodex3C0XR5xi 2Z7Babwa1JVXMw8ofUkXCRpXzNdMFH6fMMsMEqWnWhsfKYJaIG7o696DoVknaGuMBap3 PHCg== X-Gm-Message-State: AOAM531aQ1zln6AKwzW1CzlPoTnpNAvo9edM5Ol+4V20DWvpI90A1e6G 8zmZkwqyIavsUaJtEujIDAM5tA== X-Google-Smtp-Source: ABdhPJwbzKE//apNaf5RKwQ0GC+qBw8mGmdcJecUeBx2c8RcjOEaM4P/lza+S9HKMFOaghcZd1jY5Q== X-Received: by 2002:ac8:6711:: with SMTP id e17mr6329810qtp.168.1623951950559; Thu, 17 Jun 2021 10:45:50 -0700 (PDT) Received: from [192.168.1.4] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id x11sm2191462qkf.23.2021.06.17.10.45.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 17 Jun 2021 10:45:50 -0700 (PDT) Subject: Re: [PATCH 18/18] linux: Only use 64-bit syscall if required for clock_nanosleep To: Lukasz Majewski Cc: libc-alpha@sourceware.org, Carlos O'Donell References: <20210617115104.1359598-1-adhemerval.zanella@linaro.org> <20210617115104.1359598-19-adhemerval.zanella@linaro.org> <20210617171115.403b27bc@ktm> From: Adhemerval Zanella Message-ID: <3664e85a-4ea0-20ce-61a4-ffdbe7b9db4b@linaro.org> Date: Thu, 17 Jun 2021 14:45:47 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <20210617171115.403b27bc@ktm> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Jun 2021 17:45:52 -0000 On 17/06/2021 12:11, Lukasz Majewski wrote: > Hi Adhemerval, > >> For !__ASSUME_TIME64_SYSCALLS there is no need to issue a 64-bit >> syscall if the provided timeout fits in a 32-bit one. The 64-bit >> usage should be rare since the timeout is a relative one. > > I'm not sure if the above description is correct. > > As fair as I understand this patch set - we check if timeout fits into > 32-bit if: > > - We want to use legacy (32 bit) syscall when __ASSUME_TIME64_SYSCALLS > is not defined to avoid calling syscalls twice. > > The problem with relative 64 bit timeouts will be apparent after we > pass Y2038 year threshold. Yes, but 64-bit relative timeout means that caller is passing a *very* large timeout. It is possible indeed, but I don't think it the usual way. > >> >> Checked on i686-linux-gnu on a 4.15 kernel and on a 5.11 kernel >> (with and without --enable-kernel=5.1) and on x86_64-linux-gnu. >> --- >> sysdeps/unix/sysv/linux/clock_nanosleep.c | 47 >> +++++++++++++---------- time/Makefile | >> 9 +++++ time/tst-clock_nanosleep.c | 40 >> +++++++++++-------- 3 files changed, 60 insertions(+), 36 deletions(-) >> >> diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c >> b/sysdeps/unix/sysv/linux/clock_nanosleep.c index >> 007f1736cb..46b0f1e269 100644 --- >> a/sysdeps/unix/sysv/linux/clock_nanosleep.c +++ >> b/sysdeps/unix/sysv/linux/clock_nanosleep.c @@ -27,8 +27,9 @@ >> /* We can simply use the syscall. The CPU clocks are not supported >> with this function. */ >> int >> -__clock_nanosleep_time64 (clockid_t clock_id, int flags, const >> struct __timespec64 *req, >> - struct __timespec64 *rem) >> +__clock_nanosleep_time64 (clockid_t clock_id, int flags, >> + const struct __timespec64 *req, >> + struct __timespec64 *rem) >> { >> if (clock_id == CLOCK_THREAD_CPUTIME_ID) >> return EINVAL; >> @@ -37,33 +38,37 @@ __clock_nanosleep_time64 (clockid_t clock_id, int >> flags, const struct __timespec >> /* If the call is interrupted by a signal handler or encounters an >> error, it returns a positive value similar to errno. */ >> + >> #ifndef __NR_clock_nanosleep_time64 >> # define __NR_clock_nanosleep_time64 __NR_clock_nanosleep >> #endif >> - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, clock_id, >> - flags, req, rem); >> - >> -#ifndef __ASSUME_TIME64_SYSCALLS >> - if (r == 0 || r != -ENOSYS) >> - return -r; >> >> - if (! in_time_t_range (req->tv_sec)) >> + int r; >> +#ifdef __ASSUME_TIME64_SYSCALLS >> + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, clock_id, >> flags, req, >> + rem); >> +#else >> + bool is32bit = in_time_t_range (req->tv_sec); >> + if (!is32bit) >> { >> - __set_errno (EOVERFLOW); >> - return -1; >> + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, clock_id, >> flags, >> + req, rem); >> + if (r == -ENOSYS) >> + r = -EOVERFLOW; >> } >> - >> - struct timespec tr32; >> - struct timespec ts32 = valid_timespec64_to_timespec (*req); >> - r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, clock_id, flags, >> - &ts32, &tr32); >> - if (INTERNAL_SYSCALL_ERROR_P (r)) >> + else >> { >> - if (r == -EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0) >> - *rem = valid_timespec_to_timespec64 (tr32); >> + struct timespec tr32; >> + struct timespec ts32 = valid_timespec64_to_timespec (*req); >> + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, clock_id, flags, >> &ts32, >> + &tr32); >> + if (INTERNAL_SYSCALL_ERROR_P (r)) >> + { >> + if (r == -EINTR && rem != NULL && (flags & TIMER_ABSTIME) >> == 0) >> + *rem = valid_timespec_to_timespec64 (tr32); >> + } >> } >> -#endif /* __ASSUME_TIME64_SYSCALLS */ >> - >> +#endif >> return -r; >> } >> >> diff --git a/time/Makefile b/time/Makefile >> index c84bd5d3ec..0bea84966c 100644 >> --- a/time/Makefile >> +++ b/time/Makefile >> @@ -86,6 +86,15 @@ $(objpfx)tst-strftime2.out: $(gen-locales) >> $(objpfx)tst-strftime3.out: $(gen-locales) >> endif >> >> +ifeq (yes,$(build-shared)) >> +librt = $(common-objpfx)rt/librt.so >> +else >> +librt = $(common-objpfx)rt/librt.a >> +endif >> + >> +$(objpfx)tst-clock_nanosleep: $(librt) >> +$(objpfx)tst-clock_nanosleep-time64: $(librt) >> + >> tz-cflags = -DTZDIR='"$(zonedir)"' \ >> -DTZDEFAULT='"$(localtime-file)"' \ >> -DTZDEFRULES='"$(posixrules-file)"' >> diff --git a/time/tst-clock_nanosleep.c b/time/tst-clock_nanosleep.c >> index 47537435c1..a5a7f9430a 100644 >> --- a/time/tst-clock_nanosleep.c >> +++ b/time/tst-clock_nanosleep.c >> @@ -20,38 +20,48 @@ >> #include >> #include >> #include >> - >> +#include >> +#include >> +#include >> >> /* Test that clock_nanosleep() does sleep. */ >> -static int >> -do_test (void) >> +static void >> +clock_nanosleep_test (void) >> { >> /* Current time. */ >> struct timeval tv1; >> - (void) gettimeofday (&tv1, NULL); >> + gettimeofday (&tv1, NULL); >> >> - struct timespec ts; >> - ts.tv_sec = 1; >> - ts.tv_nsec = 0; >> + struct timespec ts = { 1, 0 }; >> TEMP_FAILURE_RETRY (clock_nanosleep (CLOCK_REALTIME, 0, &ts, &ts)); >> >> /* At least one second must have passed. */ >> struct timeval tv2; >> - (void) gettimeofday (&tv2, NULL); >> + gettimeofday (&tv2, NULL); >> >> tv2.tv_sec -= tv1.tv_sec; >> tv2.tv_usec -= tv1.tv_usec; >> if (tv2.tv_usec < 0) >> --tv2.tv_sec; >> >> - if (tv2.tv_sec < 1) >> - { >> - puts ("clock_nanosleep didn't sleep long enough"); >> - return 1; >> - } >> + TEST_VERIFY (tv2.tv_sec >= 1); >> +} >> + >> +static void >> +clock_nanosleep_large_timeout (void) >> +{ > > Please correct me if I'm wrong: > >> + support_create_timer (0, 100000000, false, NULL); > > You create timer with 100ms timeout > >> + struct timespec ts = { TYPE_MAXIMUM (time_t), 0 }; > > ts is created to have maximal value for time_t seconds. > >> + int r = clock_nanosleep (CLOCK_REALTIME, 0, &ts, NULL); > > The clock_nanosleep is called (64 or 32 bit version - with proper > in-glibc aliasing). > >> + TEST_VERIFY (r == EINTR || r == EOVERFLOW); > > We check if: > > - return is EOVERFLOW - this means that we want to use 64 bit time on > machine which is only supporting 64 bit time. We want to use a 64-bit relative timeout on a machine where __NR_clock_nanosleep_time64 failed with ENOSYS, meaning a pre v5.1 kernel without 64-bit time_t support. > > - return is EINTR - as the timer's (introduced in the earlier patch in > this set) alarm triggers. Am I right that this check is to find out > if the syscall is correctly executed by the kernel? Yes, the EINTR means that syscall was interrupted by the alarm. > > Or is there any other issue? I think these are the two possible situations. > >> +} >> >> +static int >> +do_test (void) >> +{ >> + clock_nanosleep_test (); >> + clock_nanosleep_large_timeout (); >> return 0; >> } >> >> -#define TEST_FUNCTION do_test () >> -#include "../test-skeleton.c" >> +#include > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de >