From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-4.2 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 470251F4B4 for ; Wed, 21 Oct 2020 06:50:07 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 23833386F472; Wed, 21 Oct 2020 06:50:06 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 23833386F472 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1603263006; bh=YqKkwZIPR6aqd6xFKim2N77f3h/zcraqMqdSNVm8JQ8=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=XI2qHg3Is6iEC+arXsDSS1XDYiZb/EyBgkOKwr0RrhrlWDaF7N9xhs8A9eVX53cnd PLH5BC7IAz/vhK81Gcu0w5xmBlfQK2HiLEi/vZRV7pLpJQQ6zj4V2k5GnhH/yxRtDm xYlioO2BeYJFniYyYvvBh1Znsgnv2xlD8D4cGNVg= Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id F15EE3861878 for ; Wed, 21 Oct 2020 06:50:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F15EE3861878 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 09L6Xmi7024198; Wed, 21 Oct 2020 02:49:42 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 34afansc6t-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Oct 2020 02:49:42 -0400 Received: from m0098410.ppops.net (m0098410.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 09L6aYkS032533; Wed, 21 Oct 2020 02:49:41 -0400 Received: from ppma06fra.de.ibm.com (48.49.7a9f.ip4.static.sl-reverse.com [159.122.73.72]) by mx0a-001b2d01.pphosted.com with ESMTP id 34afansc5n-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Oct 2020 02:49:41 -0400 Received: from pps.filterd (ppma06fra.de.ibm.com [127.0.0.1]) by ppma06fra.de.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 09L6mCD9021719; Wed, 21 Oct 2020 06:49:39 GMT Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by ppma06fra.de.ibm.com with ESMTP id 347qvha399-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Oct 2020 06:49:39 +0000 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 09L6na3w27459928 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 21 Oct 2020 06:49:37 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C4B7B4C050; Wed, 21 Oct 2020 06:49:36 +0000 (GMT) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3277E4C04A; Wed, 21 Oct 2020 06:49:36 +0000 (GMT) Received: from oc4452167425.ibm.com (unknown [9.145.74.42]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 21 Oct 2020 06:49:36 +0000 (GMT) Subject: Re: [PATCH] y2038: nptl: Provide __futex_clock_wait_bitset64 to support 64 bit bitset To: Lukasz Majewski References: <20201019145739.32361-1-lukma@denx.de> <60c4d768-9768-3ef9-94a3-184b3da8095b@linux.ibm.com> <20201020153113.6177d9f2@jawa> Message-ID: Date: Wed, 21 Oct 2020 08:49:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20201020153113.6177d9f2@jawa> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235, 18.0.737 definitions=2020-10-21_03:2020-10-20, 2020-10-21 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 mlxlogscore=999 impostorscore=0 adultscore=0 malwarescore=0 priorityscore=1501 phishscore=0 lowpriorityscore=0 clxscore=1015 suspectscore=0 mlxscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2010210052 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: , From: Stefan Liebler via Libc-alpha Reply-To: Stefan Liebler Cc: Florian Weimer , GNU C Library , Andreas Schwab , Stepan Golosunov , Alistair Francis , Joseph Myers Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" On 10/20/20 3:31 PM, Lukasz Majewski wrote: > Hi Stefan, > > Thank you for testing. > >> On 10/19/20 4:57 PM, Lukasz Majewski wrote: >>> The commit: >>> "y2038: nptl: Convert pthread_mutex_{clock|timed}lock to support 64 >>> bit" SHA1: 29e9874a048f47e2d46c40253036c8d2de921548 >>> >>> introduced support for 64 bit timeouts. Unfortunately, it was >>> missing the code for bitset - i.e. lll_futex_clock_wait_bitset C >>> preprocessor macro was used. As a result the 64 bit struct >>> __timespec64 was coerced to 32 bit struct timespec and regression >>> visible as timeout was observed (nptl/tst-robust10 on s390). >>> >>> Reported-by: Stefan Liebler >> >> I've successfully run some tests on s390 (31bit) / x86 (32bit) with >> this patch. >> - strace output of nptl/tst-robust10 on s390 (31bit): >> 10:41:37.553933 futex_time64(0x406144, >> FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 2150823950, >> {tv_sec=1603183298, tv_nsec=553918404}, FUTEX_BITSET_MATCH_ANY) = -1 >> ETIMEDOUT (Connection timed out) <1.000090> >> => Now there is only this single FUTEX_WAIT_BITSET call which times >> out after a second. > > The tv_sec=1603183298 now seems to be correct. > >> >> - strace output of nptl/tst-robust10 on x86 (32bit): >> 10:45:50.985125 futex_time64(0x804f0f4, >> FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 2147912575, >> {tv_sec=1603183551, tv_nsec=985109825}, FUTEX_BITSET_MATCH_ANY) = -1 >> ETIMEDOUT (Connection timed out) <1.000439> >> => Now tv_nsec is not zero anymore and it times out after a second. >> >> I have one question - see below. >> >> Thanks, >> Stefan >> >>> --- >>> nptl/pthread_mutex_timedlock.c | 2 +- >>> sysdeps/nptl/futex-internal.c | 48 >>> +++++++++++++++++++++++++++++++++- sysdeps/nptl/futex-internal.h | >>> 5 ++++ 3 files changed, 53 insertions(+), 2 deletions(-) >>> >>> diff --git a/nptl/pthread_mutex_timedlock.c >>> b/nptl/pthread_mutex_timedlock.c index dc40399f02..fe9e651f6c 100644 >>> --- a/nptl/pthread_mutex_timedlock.c >>> +++ b/nptl/pthread_mutex_timedlock.c >>> @@ -268,7 +268,7 @@ __pthread_mutex_clocklock_common >>> (pthread_mutex_t *mutex, assume_other_futex_waiters |= >>> FUTEX_WAITERS; >>> /* Block using the futex. */ >>> - int err = lll_futex_clock_wait_bitset >>> (&mutex->__data.__lock, >>> + int err = __futex_clock_wait_bitset64 >>> (&mutex->__data.__lock, oldval, clockid, abstime, >>> PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); >>> /* The futex call timed out. */ >>> diff --git a/sysdeps/nptl/futex-internal.c >>> b/sysdeps/nptl/futex-internal.c index 2e1919f056..a978ad0ad2 100644 >>> --- a/sysdeps/nptl/futex-internal.c >>> +++ b/sysdeps/nptl/futex-internal.c >>> @@ -70,7 +70,28 @@ __futex_abstimed_wait32 (unsigned int* >>> futex_word, abstime != NULL ? &ts32 : NULL, >>> NULL /* Unused. */, >>> FUTEX_BITSET_MATCH_ANY); } >>> -#endif >>> + >>> +static int >>> +__futex_clock_wait_bitset32 (int *futexp, int val, clockid_t >>> clockid, >>> + const struct __timespec64 *abstime, >>> int private) +{ >>> + struct timespec ts32; >>> + >>> + if (abstime != NULL && ! in_time_t_range (abstime->tv_sec)) >>> + return -EOVERFLOW; >>> + >>> + const unsigned int clockbit = >>> + (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0; >>> + const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, >>> private); + >>> + if (abstime != NULL) >>> + ts32 = valid_timespec64_to_timespec (*abstime); >>> + >>> + return INTERNAL_SYSCALL_CALL (futex, futexp, op, val, >>> + abstime != NULL ? &ts32 : NULL, >>> + NULL /* Unused. */, >>> FUTEX_BITSET_MATCH_ANY); +} >>> +#endif /* ! __ASSUME_TIME64_SYSCALLS */ >>> >>> int >>> __futex_abstimed_wait_cancelable64 (unsigned int* futex_word, >>> @@ -222,3 +243,28 @@ __futex_clocklock_wait64 (int *futex, int val, >>> clockid_t clockid, >>> return -err; >>> } >>> + >>> +int >>> +__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t >>> clockid, >>> + const struct __timespec64 *abstime, >>> + int private) >>> +{ >>> + long int relowlevellock-futex.ht; >> Is there a reason why long int is used instead of int (which is also >> returned by this function)? > > The "long int" type for ret was in the original > lll_futex_clock_wait_bitset() macro in: > ./sysdeps/nptl/lowlevellock-futex.h (line 102). > > It is also returned when this macro is pasted. > > The reason may be that lll_syscall_futex() macro also uses long int to > get return value from INTERNAL_SYSCALL() (Line 70 in the above file). > > Then depending on architecture either long or int are used to get the > status of the syscall. > > > However, in the end the value returned by lll_futex_clock_wait_bitset() > is casted to int anyway: > > git grep -n "= lll_futex_clock_wait_bitset" > nptl/pthread_mutex_timedlock.c:271: > int err = lll_futex_clock_wait_bitset (&mutex->__data.__lock, > > sysdeps/nptl/futex-internal.h:288: > int err = lll_futex_clock_wait_bitset (futex_word, expected, > > sysdeps/nptl/futex-internal.h:324: > int err = lll_futex_clock_wait_bitset (futex_word, expected, > > Hence, IMHO it would be correct to change 'long int' to 'int'. Or am I > wrong? Yes, I think this is okay. > >>> + if (! lll_futex_supported_clockid (clockid)) >>> + { >>> + return -EINVAL; >>> + } >>> + >>> + const unsigned int clockbit = >>> + (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0; >>> + const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, >>> private); + >>> + ret = INTERNAL_SYSCALL_CALL (futex_time64, futexp, op, val, >>> + abstime, NULL /* Unused. */, >>> + FUTEX_BITSET_MATCH_ANY); >>> +#ifndef __ASSUME_TIME64_SYSCALLS >>> + if (ret == -ENOSYS) >>> + ret = __futex_clock_wait_bitset32 (futexp, val, clockid, >>> abstime, private); +#endif >>> + return ret; >>> +} >>> diff --git a/sysdeps/nptl/futex-internal.h >>> b/sysdeps/nptl/futex-internal.h index 8a5f62768f..cd356e4fa8 100644 >>> --- a/sysdeps/nptl/futex-internal.h >>> +++ b/sysdeps/nptl/futex-internal.h >>> @@ -603,4 +603,9 @@ __futex_clocklock64 (int *futex, clockid_t >>> clockid, return err; >>> } >>> >>> +int >>> +__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t >>> clockid, >>> + const struct __timespec64 *abstime, >>> + int private) attribute_hidden; >>> + >>> #endif /* futex-internal.h */ >>> >> > > > 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 >