Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[Darren Hart]

On Thu, Jun 23, 2016 at 12:55:15PM -0700, Darren Hart wrote:
> On Thu, Jun 23, 2016 at 08:41:09PM +0200, Michael Kerrisk (man-pages) wrote:
> > On 06/23/2016 08:28 PM, Darren Hart wrote:
> > > On Thu, Jun 23, 2016 at 07:26:52PM +0200, Thomas Gleixner wrote:
> > > > On Thu, 23 Jun 2016, Darren Hart wrote:
> > > > > On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:
> > > > > In my opinion, we should treat the timeout value as relative for FUTEX_WAIT
> > > > > regardless of the CLOCK used.
> > > > 
> > > > Which requires even more changes as you have to select which clock you are
> > > > using for adding the base time.
> > > 
> > > Right, something like the following?
> > > 
> > > 
> > > diff --git a/kernel/futex.c b/kernel/futex.c
> > > index 33664f7..c39d807 100644
> > > --- a/kernel/futex.c
> > > +++ b/kernel/futex.c
> > > @@ -3230,8 +3230,12 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> > >  			return -EINVAL;
> > > 
> > >  		t = timespec_to_ktime(ts);
> > > -		if (cmd == FUTEX_WAIT)
> > > -			t = ktime_add_safe(ktime_get(), t);
> > > +		if (cmd == FUTEX_WAIT) {
> > > +			if (cmd & FUTEX_CLOCK_REALTIME)
> > > +				t = ktime_add_safe(ktime_get_real(), t);
> > > +			else
> > > +				t = ktime_add_safe(ktime_get(), t);
> > > +		}
> > >  		tp = &t;
> > >  	}
> > >  	/*
> > 
> > Just in the interests of readability/maintainability, might it not
> > make some sense to recode the timeout handling for FUTEX_WAIT
> > within futex_wait(). I think that part of the reason we're in this
> > mess of inconsistency is that timeout interpretation is being handled
> > at too many different points in the code.
> 
> 
> I agree, that is indeed why I missed it in my original patch.

Or perhaps in do_futex() which is where the majority of the argument
interpretation is done, and which already has a switch statement for all op
codes. Maybe something like this:


diff --git a/kernel/futex.c b/kernel/futex.c
index 33664f7..c666715 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3157,6 +3157,7 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 {
 	int cmd = op & FUTEX_CMD_MASK;
 	unsigned int flags = 0;
+	ktime_t t;
 
 	if (!(op & FUTEX_PRIVATE_FLAG))
 		flags |= FLAGS_SHARED;
@@ -3181,6 +3182,18 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 	switch (cmd) {
 	case FUTEX_WAIT:
 		val3 = FUTEX_BITSET_MATCH_ANY;
+		/*
+		 * The user-facing FUTEX_WAIT op interface receives a relative
+		 * timeout. The kernel-side futex_wait() function accepts an
+		 * absolute timeout. Convert the relative timeout to absolute.
+		 */
+		if (timeout) {
+			if (op & FUTEX_CLOCK_REALTIME)
+				t = ktime_add_safe(ktime_get_real(), *timeout);
+			else
+				t = ktime_add_safe(ktime_get(), *timeout);
+			timeout = &t;
+		}
 	case FUTEX_WAIT_BITSET:
 		return futex_wait(uaddr, flags, val, timeout, val3);
 	case FUTEX_WAKE:
@@ -3230,8 +3243,6 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 			return -EINVAL;
 
 		t = timespec_to_ktime(ts);
-		if (cmd == FUTEX_WAIT)
-			t = ktime_add_safe(ktime_get(), t);
 		tp = &t;
 	}
 	/*

-- 
Darren Hart
Intel Open Source Technology Center