From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751720AbcFWEsd (ORCPT ); Thu, 23 Jun 2016 00:48:33 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:42588 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750979AbcFWEsc (ORCPT ); Thu, 23 Jun 2016 00:48:32 -0400 Date: Wed, 22 Jun 2016 21:48:28 -0700 From: Darren Hart To: Matthieu CASTET Cc: linux-kernel@vger.kernel.org, Michael Kerrisk , Darren Hart , Peter Zijlstra , Davidlohr Bueso , Thomas Gleixner , Eric Dumazet Subject: Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op Message-ID: <20160623044828.GA3379@f23x64.localdomain> References: <20160620162652.2c4619c3@perruche.parrot.biz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160620162652.2c4619c3@perruche.parrot.biz> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 20, 2016 at 04:26:52PM +0200, Matthieu CASTET wrote: > Hi, > > the commit 337f13046ff03717a9e99675284a817527440a49 is saying that it > change to syscall to an equivalent to FUTEX_WAIT_BITSET | > FUTEX_CLOCK_REALTIME with a bitset of FUTEX_BITSET_MATCH_ANY. > > It seems wrong to me, because in case of FUTEX_WAIT, in > "SYSCALL_DEFINE6(futex", we convert relative timeout to absolute > timeout [1]. > > So FUTEX_CLOCK_REALTIME | FUTEX_WAIT is expecting a relative timeout > when FUTEX_WAIT_BITSET take an absolute timeout. > > To make it work you have to use something like the (untested) attached > patch. +Eric Dumazet Thanks for reporting Matthieu, FUTEX_WAIT traditionally used a relative timeout with CLOCK_MONOTONIC while FUTEX_WAIT_BITSET could use either ??? based on the FUTEX_CLOCK_ flag used. The man page is not particularly clear on this: http://man7.org/linux/man-pages/man2/futex.2.html " The FUTEX_WAIT_BITSET operation also interprets the timeout argument differently from FUTEX_WAIT. See the discussion of FUTEX_CLOCK_REALTIME, above. " Matthew Kerrisk: I think this language could be removed now that we support the FUTEX_CLOCK_REALTIME flag for both futex ops. As for the intended behavior of the FUTEX_CLOCK_REALTIME flag: FUTEX_CLOCK_REALTIME (since Linux 2.6.28) This option bit can be employed only with the FUTEX_WAIT_BITSET, FUTEX_WAIT_REQUEUE_PI, and FUTEX_WAIT (since Linux 4.5) operations. (NOTE: FUTEX_WAIT was recently added after the patch in question here) If this option is set, the kernel treats timeout as an absolute time based on CLOCK_REALTIME. If this option is not set, the kernel treats timeout as a relative time, measured against the CLOCK_MONOTONIC clock. This supports your argument Matthieu. The assumption of a relative timeout for FUTEX_WAIT in SYSCALL_DEFINE6 needs to be updated to account for FUTEX_WAIT now honoring the FUTEX_CLOCK_REALTIME flag, which treats the timeout as absolute. However, I don't think the patch below is correct. The existing logic determines the type of timeout based on the futex_op when it should instead determine the type of timeout based on the FUTEX_CLOCK_REALTIME flag. My reading of the man page is that FUTEX_WAIT_BITSET abides by the timeout interpretation defined by the FUTEX_CLOCK_REALTIME attribute, so SYSCALL_DEFINE6 was misbehaving for FUTEX_WAIT|FUTEX_CLOCK_REALTIME (where the timeout should have been treated as absolute) as well as for FUTEX_WAIT_BITSET|FUTEX_CLOCK_MONOTONIC (where the timeout should have been treated as relative). Consider the following: diff --git a/kernel/futex.c b/kernel/futex.c index 33664f7..fa2af29 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -3230,7 +3230,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, return -EINVAL; t = timespec_to_ktime(ts); - if (cmd == FUTEX_WAIT) + if (!(cmd & FUTEX_CLOCK_REALTIME)) t = ktime_add_safe(ktime_get(), t); tp = &t; } The concern for me is whether the code is incorrect, or if the man page is incorrect. Does existing userspace code expect the FUTEX_WAIT_BITSET op to always use an absolute timeout, regardless of the CLOCK used? > > Matthieu > > [1] > if (cmd == FUTEX_WAIT) > t = ktime_add_safe(ktime_get(), t); > diff --git a/kernel/futex.c b/kernel/futex.c > index 33664f7..4bee915 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -3230,7 +3230,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, > return -EINVAL; > > t = timespec_to_ktime(ts); > - if (cmd == FUTEX_WAIT) > + if (cmd == FUTEX_WAIT && !(op & FUTEX_CLOCK_REALTIME)) > t = ktime_add_safe(ktime_get(), t); > tp = &t; > } -- Darren Hart Intel Open Source Technology Center