Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430006 > unrolled thread

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

Started byThomas Gleixner <tglx@linutronix.de>
First post2016-06-23 19:30 +0200
Last post2016-07-06 21:10 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op Thomas Gleixner <tglx@linutronix.de> - 2016-06-23 19:30 +0200
    Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op Darren Hart <dvhart@infradead.org> - 2016-06-23 20:30 +0200
      Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2016-06-23 20:50 +0200
        Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op Darren Hart <dvhart@infradead.org> - 2016-06-23 22:00 +0200
          Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op Darren Hart <dvhart@infradead.org> - 2016-06-23 22:40 +0200
            Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op Thomas Gleixner <tglx@linutronix.de> - 2016-06-24 12:00 +0200
              Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2016-06-24 12:10 +0200
        Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op Thomas Gleixner <tglx@linutronix.de> - 2016-07-06 21:10 +0200

#1430006 — Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

FromThomas Gleixner <tglx@linutronix.de>
Date2016-06-23 19:30 +0200
SubjectRe: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op
Message-ID<rNgBX-4Mw-11@gated-at.bofh.it>
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.
 
Thanks,

	tglx

[toc] | [next] | [standalone]

#1430038

FromDarren Hart <dvhart@infradead.org>
Date2016-06-23 20:30 +0200
Message-ID<rNhy1-5wH-3@gated-at.bofh.it>
In reply to#1430006
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;
 	}
 	/*

And as a follow-on, what is the reason for FUTEX_LOCK_PI only using
CLOCK_REALTIME? It seems reasonable to me that a user may want to wait a
specific amount of time, regardless of wall time.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1430045

From"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Date2016-06-23 20:50 +0200
Message-ID<rNhRn-5Ej-3@gated-at.bofh.it>
In reply to#1430038
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.

> And as a follow-on, what is the reason for FUTEX_LOCK_PI only using
> CLOCK_REALTIME? It seems reasonable to me that a user may want to wait a
> specific amount of time, regardless of wall time.

Yes, that's another weird inconsistency.

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[toc] | [prev] | [next] | [standalone]

#1430114

FromDarren Hart <dvhart@infradead.org>
Date2016-06-23 22:00 +0200
Message-ID<rNiX7-6lq-1@gated-at.bofh.it>
In reply to#1430045
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.


> 
> > And as a follow-on, what is the reason for FUTEX_LOCK_PI only using
> > CLOCK_REALTIME? It seems reasonable to me that a user may want to wait a
> > specific amount of time, regardless of wall time.
> 
> Yes, that's another weird inconsistency.
> 
> Thanks,
> 
> Michael
> 
> 
> -- 
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
> 

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1430137

FromDarren Hart <dvhart@infradead.org>
Date2016-06-23 22:40 +0200
Message-ID<rNjzP-6Qf-13@gated-at.bofh.it>
In reply to#1430114
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

[toc] | [prev] | [next] | [standalone]

#1430547

FromThomas Gleixner <tglx@linutronix.de>
Date2016-06-24 12:00 +0200
Message-ID<rNw42-6qx-39@gated-at.bofh.it>
In reply to#1430137

[Multipart message — attachments visible in raw view] — view raw

On Fri, 24 Jun 2016, Michael Kerrisk (man-pages) wrote:
> By the way, I just realized something that wasn't initially obvious
> to me, and documented it in the futex(2) man page:
> 
>               Note:  for  FUTEX_WAIT,  timeout is interpreted as a
>               relative value.  This differs from other futex oper‐
>               ations,  where timeout is interpreted as an absolute
>               value.  To obtain the equivalent of FUTEX_WAIT  with
>               an  absolute  timeout, employ FUTEX_WAIT_BITSET with
>               val3 specified as FUTEX_BITSET_MATCH_ANY.
> 
> Okay?

Yes.

[toc] | [prev] | [next] | [standalone]

#1430557

From"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Date2016-06-24 12:10 +0200
Message-ID<rNwdH-6IP-7@gated-at.bofh.it>
In reply to#1430547
On 06/24/2016 11:52 AM, Thomas Gleixner wrote:
> On Fri, 24 Jun 2016, Michael Kerrisk (man-pages) wrote:
>> By the way, I just realized something that wasn't initially obvious
>> to me, and documented it in the futex(2) man page:
>>
>>               Note:  for  FUTEX_WAIT,  timeout is interpreted as a
>>               relative value.  This differs from other futex oper‐
>>               ations,  where timeout is interpreted as an absolute
>>               value.  To obtain the equivalent of FUTEX_WAIT  with
>>               an  absolute  timeout, employ FUTEX_WAIT_BITSET with
>>               val3 specified as FUTEX_BITSET_MATCH_ANY.
>>
>> Okay?
>
> Yes.

Thanks, Thomas.

Cheers,

Michael



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[toc] | [prev] | [next] | [standalone]

#1437918

FromThomas Gleixner <tglx@linutronix.de>
Date2016-07-06 21:10 +0200
Message-ID<rS0mR-4Im-17@gated-at.bofh.it>
In reply to#1430045
On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:
> On 06/23/2016 08:28 PM, Darren Hart wrote:
> > And as a follow-on, what is the reason for FUTEX_LOCK_PI only using
> > CLOCK_REALTIME? It seems reasonable to me that a user may want to wait a
> > specific amount of time, regardless of wall time.
> 
> Yes, that's another weird inconsistency.

The reason is that phtread_mutex_timedlock() uses absolute timeouts based on
CLOCK_REALTIME. glibc folks asked to make that the default behaviour back then
when we added LOCK_PI.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web