Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-07-06 Thread Thomas Gleixner 
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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-07-06 Thread Thomas Gleixner 
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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-24 Thread Michael Kerrisk (man-pages) 

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/

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-24 Thread Michael Kerrisk (man-pages) 

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/

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-24 Thread Thomas Gleixner 
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.

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-24 Thread Thomas Gleixner 
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.

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-24 Thread Michael Kerrisk (man-pages) 

On 06/23/2016 09:53 PM, Darren Hart wrote:

On Thu, Jun 23, 2016 at 08:35:15PM +0200, Michael Kerrisk (man-pages) wrote:

Hi Darren,

On 06/23/2016 06:16 PM, Darren Hart wrote:

On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:

On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:

On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
Once upon a time, you told me the following:

On 15 May 2014 at 16:14, Thomas Gleixner  wrote:

On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:

And that universe would love to have your documentation of
FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),


I give you almost the full treatment, but I leave REQUEUE_PI to Darren
and FUTEX_WAKE_OP to Jakub. :)
[...]
FUTEX_CLOCK_REALTIME

This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
and FUTEX_WAIT_REQUEUE_PI

If set the kernel treats the user space supplied timeout as
absolute time based on CLOCK_REALTIME.

If not set the kernel treats the user space supplied timeout
as relative time.

Unfortunately, I should have checked the code more carefully...


Me too :)


Seems to be going around...




Looking more carefully at the code, I see understand the situation
is the following:

FUTEX_LOCK_PI
Always uses CLOCK_REALTIME
'timeout' is absolute


Yes.


FUTEX_WAIT_REQUEUE_PI
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT_BITSET
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is relative


Yes.


I've amended the man page to describe those details.


OK, that confirms my question, timeout interpretation as relative or absolute is
based on the op code, not the CLOCK flag.




The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.


When you say that the "flag was added", which flag do you mean? Or, did you
mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
time".


I didn't express myself clearly. When Darren added the support for
CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
support. Anything else does not make sense.


I sent that patch because reading the new man page it struck me as strange that
FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were not,
especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
ALL.

I didn't realize the impact to relative/absolute interpretation of the timeout
value at the time.

I think it was a mistake to introduce a change that made FUTEX_WAIT interpret
the timeout differently based on the CLOCK flag,


I'm missing something. Where does it do that? As far as I can tell FUTEX_WAIT
always interprets the clock as relative, regardless of presence/absence of
FUTEX_CLOCK_REALTIME? Am I missing something?


No you're not. The code as it stands today is always relative, but it gets the
base time from the wrong clock source in the case of FUTEX_CLOCK_REALTIME.


Ahh yes, I'd clicked to that, but forgot to say so.


I was stating that I think it would be a mistake to add absolute timeout to
FUTEX_WAIT based on the FUTEX_CLOCK_REALTIME flag, which is how Thomas describes
above his interpretation of my earlier change.


Got it now. Thanks for the clarification, Darren.

Cheers

Michael



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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-24 Thread Michael Kerrisk (man-pages) 

On 06/23/2016 09:53 PM, Darren Hart wrote:

On Thu, Jun 23, 2016 at 08:35:15PM +0200, Michael Kerrisk (man-pages) wrote:

Hi Darren,

On 06/23/2016 06:16 PM, Darren Hart wrote:

On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:

On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:

On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
Once upon a time, you told me the following:

On 15 May 2014 at 16:14, Thomas Gleixner  wrote:

On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:

And that universe would love to have your documentation of
FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),


I give you almost the full treatment, but I leave REQUEUE_PI to Darren
and FUTEX_WAKE_OP to Jakub. :)
[...]
FUTEX_CLOCK_REALTIME

This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
and FUTEX_WAIT_REQUEUE_PI

If set the kernel treats the user space supplied timeout as
absolute time based on CLOCK_REALTIME.

If not set the kernel treats the user space supplied timeout
as relative time.

Unfortunately, I should have checked the code more carefully...


Me too :)


Seems to be going around...




Looking more carefully at the code, I see understand the situation
is the following:

FUTEX_LOCK_PI
Always uses CLOCK_REALTIME
'timeout' is absolute


Yes.


FUTEX_WAIT_REQUEUE_PI
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT_BITSET
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is relative


Yes.


I've amended the man page to describe those details.


OK, that confirms my question, timeout interpretation as relative or absolute is
based on the op code, not the CLOCK flag.




The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.


When you say that the "flag was added", which flag do you mean? Or, did you
mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
time".


I didn't express myself clearly. When Darren added the support for
CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
support. Anything else does not make sense.


I sent that patch because reading the new man page it struck me as strange that
FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were not,
especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
ALL.

I didn't realize the impact to relative/absolute interpretation of the timeout
value at the time.

I think it was a mistake to introduce a change that made FUTEX_WAIT interpret
the timeout differently based on the CLOCK flag,


I'm missing something. Where does it do that? As far as I can tell FUTEX_WAIT
always interprets the clock as relative, regardless of presence/absence of
FUTEX_CLOCK_REALTIME? Am I missing something?


No you're not. The code as it stands today is always relative, but it gets the
base time from the wrong clock source in the case of FUTEX_CLOCK_REALTIME.


Ahh yes, I'd clicked to that, but forgot to say so.


I was stating that I think it would be a mistake to add absolute timeout to
FUTEX_WAIT based on the FUTEX_CLOCK_REALTIME flag, which is how Thomas describes
above his interpretation of my earlier change.


Got it now. Thanks for the clarification, Darren.

Cheers

Michael



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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread 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 = 
> > >   }
> > >   /*
> > 
> > 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 = 
+   }
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 = 
}
/*

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread 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 = 
> > >   }
> > >   /*
> > 
> > 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 = 
+   }
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 = 
}
/*

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
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 = 
> > }
> > /*
> 
> 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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
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 = 
> > }
> > /*
> 
> 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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
On Thu, Jun 23, 2016 at 08:35:15PM +0200, Michael Kerrisk (man-pages) wrote:
> Hi Darren,
> 
> On 06/23/2016 06:16 PM, Darren Hart wrote:
> > On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:
> > > On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:
> > > > On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
> > > > Once upon a time, you told me the following:
> > > > 
> > > > On 15 May 2014 at 16:14, Thomas Gleixner  wrote:
> > > > > On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:
> > > > > > And that universe would love to have your documentation of
> > > > > > FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),
> > > > > 
> > > > > I give you almost the full treatment, but I leave REQUEUE_PI to Darren
> > > > > and FUTEX_WAKE_OP to Jakub. :)
> > > > > [...]
> > > > > FUTEX_CLOCK_REALTIME
> > > > > 
> > > > > This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
> > > > > and FUTEX_WAIT_REQUEUE_PI
> > > > > 
> > > > > If set the kernel treats the user space supplied timeout as
> > > > > absolute time based on CLOCK_REALTIME.
> > > > > 
> > > > > If not set the kernel treats the user space supplied timeout
> > > > > as relative time.
> > > > Unfortunately, I should have checked the code more carefully...
> > > 
> > > Me too :)
> > 
> > Seems to be going around...
> > 
> > > 
> > > > Looking more carefully at the code, I see understand the situation
> > > > is the following:
> > > > 
> > > > FUTEX_LOCK_PI
> > > > Always uses CLOCK_REALTIME
> > > > 'timeout' is absolute
> > > 
> > > Yes.
> > > 
> > > > FUTEX_WAIT_REQUEUE_PI
> > > > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > > > determined by presence or absence of
> > > > FUTEX_CLOCK_REALTIME flag
> > > > 'timeout' is absolute
> > > 
> > > Yes
> > > 
> > > > FUTEX_WAIT_BITSET
> > > > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > > > determined by presence or absence of
> > > > FUTEX_CLOCK_REALTIME flag
> > > > 'timeout' is absolute
> > > 
> > > Yes
> > > 
> > > > FUTEX_WAIT
> > > > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > > > determined by presence or absence of
> > > > FUTEX_CLOCK_REALTIME flag
> > > > 'timeout' is relative
> > > 
> > > Yes.
> > > 
> > > > I've amended the man page to describe those details.
> > 
> > OK, that confirms my question, timeout interpretation as relative or 
> > absolute is
> > based on the op code, not the CLOCK flag.
> > 
> > > > 
> > > > > The flag was explicitely added to allow FUTEX_WAIT to hand in 
> > > > > absolute time.
> > > > 
> > > > When you say that the "flag was added", which flag do you mean? Or, did 
> > > > you
> > > > mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in 
> > > > absolute
> > > > time".
> > > 
> > > I didn't express myself clearly. When Darren added the support for
> > > CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
> > > support. Anything else does not make sense.
> > 
> > I sent that patch because reading the new man page it struck me as strange 
> > that
> > FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were 
> > not,
> > especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
> > ALL.
> > 
> > I didn't realize the impact to relative/absolute interpretation of the 
> > timeout
> > value at the time.
> > 
> > I think it was a mistake to introduce a change that made FUTEX_WAIT 
> > interpret
> > the timeout differently based on the CLOCK flag,
> 
> I'm missing something. Where does it do that? As far as I can tell FUTEX_WAIT
> always interprets the clock as relative, regardless of presence/absence of
> FUTEX_CLOCK_REALTIME? Am I missing something?

No you're not. The code as it stands today is always relative, but it gets the
base time from the wrong clock source in the case of FUTEX_CLOCK_REALTIME.

I was stating that I think it would be a mistake to add absolute timeout to
FUTEX_WAIT based on the FUTEX_CLOCK_REALTIME flag, which is how Thomas describes
above his interpretation of my earlier change.

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
On Thu, Jun 23, 2016 at 08:35:15PM +0200, Michael Kerrisk (man-pages) wrote:
> Hi Darren,
> 
> On 06/23/2016 06:16 PM, Darren Hart wrote:
> > On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:
> > > On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:
> > > > On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
> > > > Once upon a time, you told me the following:
> > > > 
> > > > On 15 May 2014 at 16:14, Thomas Gleixner  wrote:
> > > > > On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:
> > > > > > And that universe would love to have your documentation of
> > > > > > FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),
> > > > > 
> > > > > I give you almost the full treatment, but I leave REQUEUE_PI to Darren
> > > > > and FUTEX_WAKE_OP to Jakub. :)
> > > > > [...]
> > > > > FUTEX_CLOCK_REALTIME
> > > > > 
> > > > > This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
> > > > > and FUTEX_WAIT_REQUEUE_PI
> > > > > 
> > > > > If set the kernel treats the user space supplied timeout as
> > > > > absolute time based on CLOCK_REALTIME.
> > > > > 
> > > > > If not set the kernel treats the user space supplied timeout
> > > > > as relative time.
> > > > Unfortunately, I should have checked the code more carefully...
> > > 
> > > Me too :)
> > 
> > Seems to be going around...
> > 
> > > 
> > > > Looking more carefully at the code, I see understand the situation
> > > > is the following:
> > > > 
> > > > FUTEX_LOCK_PI
> > > > Always uses CLOCK_REALTIME
> > > > 'timeout' is absolute
> > > 
> > > Yes.
> > > 
> > > > FUTEX_WAIT_REQUEUE_PI
> > > > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > > > determined by presence or absence of
> > > > FUTEX_CLOCK_REALTIME flag
> > > > 'timeout' is absolute
> > > 
> > > Yes
> > > 
> > > > FUTEX_WAIT_BITSET
> > > > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > > > determined by presence or absence of
> > > > FUTEX_CLOCK_REALTIME flag
> > > > 'timeout' is absolute
> > > 
> > > Yes
> > > 
> > > > FUTEX_WAIT
> > > > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > > > determined by presence or absence of
> > > > FUTEX_CLOCK_REALTIME flag
> > > > 'timeout' is relative
> > > 
> > > Yes.
> > > 
> > > > I've amended the man page to describe those details.
> > 
> > OK, that confirms my question, timeout interpretation as relative or 
> > absolute is
> > based on the op code, not the CLOCK flag.
> > 
> > > > 
> > > > > The flag was explicitely added to allow FUTEX_WAIT to hand in 
> > > > > absolute time.
> > > > 
> > > > When you say that the "flag was added", which flag do you mean? Or, did 
> > > > you
> > > > mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in 
> > > > absolute
> > > > time".
> > > 
> > > I didn't express myself clearly. When Darren added the support for
> > > CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
> > > support. Anything else does not make sense.
> > 
> > I sent that patch because reading the new man page it struck me as strange 
> > that
> > FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were 
> > not,
> > especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
> > ALL.
> > 
> > I didn't realize the impact to relative/absolute interpretation of the 
> > timeout
> > value at the time.
> > 
> > I think it was a mistake to introduce a change that made FUTEX_WAIT 
> > interpret
> > the timeout differently based on the CLOCK flag,
> 
> I'm missing something. Where does it do that? As far as I can tell FUTEX_WAIT
> always interprets the clock as relative, regardless of presence/absence of
> FUTEX_CLOCK_REALTIME? Am I missing something?

No you're not. The code as it stands today is always relative, but it gets the
base time from the wrong clock source in the case of FUTEX_CLOCK_REALTIME.

I was stating that I think it would be a mistake to add absolute timeout to
FUTEX_WAIT based on the FUTEX_CLOCK_REALTIME flag, which is how Thomas describes
above his interpretation of my earlier change.

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

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 = 
}
/*


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/

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

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 = 
}
/*


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/

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

Hi Darren,

On 06/23/2016 06:16 PM, Darren Hart wrote:

On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:

On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:

On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
Once upon a time, you told me the following:

On 15 May 2014 at 16:14, Thomas Gleixner  wrote:

On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:

And that universe would love to have your documentation of
FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),


I give you almost the full treatment, but I leave REQUEUE_PI to Darren
and FUTEX_WAKE_OP to Jakub. :)
[...]
FUTEX_CLOCK_REALTIME

This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
and FUTEX_WAIT_REQUEUE_PI

If set the kernel treats the user space supplied timeout as
absolute time based on CLOCK_REALTIME.

If not set the kernel treats the user space supplied timeout
as relative time.

Unfortunately, I should have checked the code more carefully...


Me too :)


Seems to be going around...




Looking more carefully at the code, I see understand the situation
is the following:

FUTEX_LOCK_PI
Always uses CLOCK_REALTIME
'timeout' is absolute


Yes.


FUTEX_WAIT_REQUEUE_PI
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT_BITSET
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is relative


Yes.


I've amended the man page to describe those details.


OK, that confirms my question, timeout interpretation as relative or absolute is
based on the op code, not the CLOCK flag.




The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.


When you say that the "flag was added", which flag do you mean? Or, did you
mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
time".


I didn't express myself clearly. When Darren added the support for
CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
support. Anything else does not make sense.


I sent that patch because reading the new man page it struck me as strange that
FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were not,
especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
ALL.

I didn't realize the impact to relative/absolute interpretation of the timeout
value at the time.

I think it was a mistake to introduce a change that made FUTEX_WAIT interpret
the timeout differently based on the CLOCK flag,


I'm missing something. Where does it do that? As far as I can tell FUTEX_WAIT
always interprets the clock as relative, regardless of presence/absence of
FUTEX_CLOCK_REALTIME? Am I missing something?


while that interpretation is
independent of the CLOCK flag for all other op codes.

In my opinion, we should treat the timeout value as relative for FUTEX_WAIT
regardless of the CLOCK used.


I realize it's historical, but it is really weird that FUTEX_WAIT interprets
time timeout (relative vs absolute) differently from all of the other
operations.

That would require a change to the man page to eliminate the relative/absolute
language in the FUTEX_CLOCK_REALTIME definition and explicit definitions of the
interpretation for each op code (as Matthew explains above).

Do we agree on that?


Yes.

The man page changes are already in Git. My earlier reply contained the
commit ref:
http://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit/?id=8064bfa5369c6856f606004d02e48ab275e05bed

Cheers,

Michael


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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

Hi Darren,

On 06/23/2016 06:16 PM, Darren Hart wrote:

On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:

On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:

On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
Once upon a time, you told me the following:

On 15 May 2014 at 16:14, Thomas Gleixner  wrote:

On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:

And that universe would love to have your documentation of
FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),


I give you almost the full treatment, but I leave REQUEUE_PI to Darren
and FUTEX_WAKE_OP to Jakub. :)
[...]
FUTEX_CLOCK_REALTIME

This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
and FUTEX_WAIT_REQUEUE_PI

If set the kernel treats the user space supplied timeout as
absolute time based on CLOCK_REALTIME.

If not set the kernel treats the user space supplied timeout
as relative time.

Unfortunately, I should have checked the code more carefully...


Me too :)


Seems to be going around...




Looking more carefully at the code, I see understand the situation
is the following:

FUTEX_LOCK_PI
Always uses CLOCK_REALTIME
'timeout' is absolute


Yes.


FUTEX_WAIT_REQUEUE_PI
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT_BITSET
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute


Yes


FUTEX_WAIT
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is relative


Yes.


I've amended the man page to describe those details.


OK, that confirms my question, timeout interpretation as relative or absolute is
based on the op code, not the CLOCK flag.




The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.


When you say that the "flag was added", which flag do you mean? Or, did you
mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
time".


I didn't express myself clearly. When Darren added the support for
CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
support. Anything else does not make sense.


I sent that patch because reading the new man page it struck me as strange that
FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were not,
especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
ALL.

I didn't realize the impact to relative/absolute interpretation of the timeout
value at the time.

I think it was a mistake to introduce a change that made FUTEX_WAIT interpret
the timeout differently based on the CLOCK flag,


I'm missing something. Where does it do that? As far as I can tell FUTEX_WAIT
always interprets the clock as relative, regardless of presence/absence of
FUTEX_CLOCK_REALTIME? Am I missing something?


while that interpretation is
independent of the CLOCK flag for all other op codes.

In my opinion, we should treat the timeout value as relative for FUTEX_WAIT
regardless of the CLOCK used.


I realize it's historical, but it is really weird that FUTEX_WAIT interprets
time timeout (relative vs absolute) differently from all of the other
operations.

That would require a change to the man page to eliminate the relative/absolute
language in the FUTEX_CLOCK_REALTIME definition and explicit definitions of the
interpretation for each op code (as Matthew explains above).

Do we agree on that?


Yes.

The man page changes are already in Git. My earlier reply contained the
commit ref:
http://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit/?id=8064bfa5369c6856f606004d02e48ab275e05bed

Cheers,

Michael


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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
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 = 
}
/*

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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
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 = 
}
/*

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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Thomas Gleixner 
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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Thomas Gleixner 
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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:
> On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:
> > On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
> > Once upon a time, you told me the following:
> > 
> > On 15 May 2014 at 16:14, Thomas Gleixner  wrote:
> > > On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:
> > > > And that universe would love to have your documentation of
> > > > FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),
> > > 
> > > I give you almost the full treatment, but I leave REQUEUE_PI to Darren
> > > and FUTEX_WAKE_OP to Jakub. :)
> > > [...]
> > > FUTEX_CLOCK_REALTIME
> > > 
> > > This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
> > > and FUTEX_WAIT_REQUEUE_PI
> > > 
> > > If set the kernel treats the user space supplied timeout as
> > > absolute time based on CLOCK_REALTIME.
> > > 
> > > If not set the kernel treats the user space supplied timeout
> > > as relative time.
> > Unfortunately, I should have checked the code more carefully...
> 
> Me too :)

Seems to be going around...

>  
> > Looking more carefully at the code, I see understand the situation
> > is the following:
> > 
> > FUTEX_LOCK_PI
> > Always uses CLOCK_REALTIME
> > 'timeout' is absolute
> 
> Yes.
>  
> > FUTEX_WAIT_REQUEUE_PI
> > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > determined by presence or absence of
> > FUTEX_CLOCK_REALTIME flag
> > 'timeout' is absolute
> 
> Yes
> 
> > FUTEX_WAIT_BITSET
> > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > determined by presence or absence of
> > FUTEX_CLOCK_REALTIME flag
> > 'timeout' is absolute
> 
> Yes
>  
> > FUTEX_WAIT
> > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > determined by presence or absence of
> > FUTEX_CLOCK_REALTIME flag
> > 'timeout' is relative
> 
> Yes.
> 
> > I've amended the man page to describe those details.

OK, that confirms my question, timeout interpretation as relative or absolute is
based on the op code, not the CLOCK flag.

> > 
> > > The flag was explicitely added to allow FUTEX_WAIT to hand in absolute 
> > > time.
> > 
> > When you say that the "flag was added", which flag do you mean? Or, did you
> > mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
> > time".
> 
> I didn't express myself clearly. When Darren added the support for
> CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
> support. Anything else does not make sense.

I sent that patch because reading the new man page it struck me as strange that
FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were not,
especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
ALL.

I didn't realize the impact to relative/absolute interpretation of the timeout
value at the time.

I think it was a mistake to introduce a change that made FUTEX_WAIT interpret
the timeout differently based on the CLOCK flag, while that interpretation is
independent of the CLOCK flag for all other op codes.

In my opinion, we should treat the timeout value as relative for FUTEX_WAIT
regardless of the CLOCK used.

That would require a change to the man page to eliminate the relative/absolute
language in the FUTEX_CLOCK_REALTIME definition and explicit definitions of the
interpretation for each op code (as Matthew explains above).

Do we agree on that?

> 
> Thanks,
> 
>   tglx
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Darren Hart 
On Thu, Jun 23, 2016 at 03:40:36PM +0200, Thomas Gleixner wrote:
> On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:
> > On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
> > Once upon a time, you told me the following:
> > 
> > On 15 May 2014 at 16:14, Thomas Gleixner  wrote:
> > > On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:
> > > > And that universe would love to have your documentation of
> > > > FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),
> > > 
> > > I give you almost the full treatment, but I leave REQUEUE_PI to Darren
> > > and FUTEX_WAKE_OP to Jakub. :)
> > > [...]
> > > FUTEX_CLOCK_REALTIME
> > > 
> > > This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
> > > and FUTEX_WAIT_REQUEUE_PI
> > > 
> > > If set the kernel treats the user space supplied timeout as
> > > absolute time based on CLOCK_REALTIME.
> > > 
> > > If not set the kernel treats the user space supplied timeout
> > > as relative time.
> > Unfortunately, I should have checked the code more carefully...
> 
> Me too :)

Seems to be going around...

>  
> > Looking more carefully at the code, I see understand the situation
> > is the following:
> > 
> > FUTEX_LOCK_PI
> > Always uses CLOCK_REALTIME
> > 'timeout' is absolute
> 
> Yes.
>  
> > FUTEX_WAIT_REQUEUE_PI
> > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > determined by presence or absence of
> > FUTEX_CLOCK_REALTIME flag
> > 'timeout' is absolute
> 
> Yes
> 
> > FUTEX_WAIT_BITSET
> > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > determined by presence or absence of
> > FUTEX_CLOCK_REALTIME flag
> > 'timeout' is absolute
> 
> Yes
>  
> > FUTEX_WAIT
> > Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
> > determined by presence or absence of
> > FUTEX_CLOCK_REALTIME flag
> > 'timeout' is relative
> 
> Yes.
> 
> > I've amended the man page to describe those details.

OK, that confirms my question, timeout interpretation as relative or absolute is
based on the op code, not the CLOCK flag.

> > 
> > > The flag was explicitely added to allow FUTEX_WAIT to hand in absolute 
> > > time.
> > 
> > When you say that the "flag was added", which flag do you mean? Or, did you
> > mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
> > time".
> 
> I didn't express myself clearly. When Darren added the support for
> CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
> support. Anything else does not make sense.

I sent that patch because reading the new man page it struck me as strange that
FUTEX_WAIT was restricted to CLOCK_MONOTONIC and the other op codes were not,
especially since FUTEX_WAIT is a just FUTEX_WAIT_BITSET with the mask set to
ALL.

I didn't realize the impact to relative/absolute interpretation of the timeout
value at the time.

I think it was a mistake to introduce a change that made FUTEX_WAIT interpret
the timeout differently based on the CLOCK flag, while that interpretation is
independent of the CLOCK flag for all other op codes.

In my opinion, we should treat the timeout value as relative for FUTEX_WAIT
regardless of the CLOCK used.

That would require a change to the man page to eliminate the relative/absolute
language in the FUTEX_CLOCK_REALTIME definition and explicit definitions of the
interpretation for each op code (as Matthew explains above).

Do we agree on that?

> 
> Thanks,
> 
>   tglx
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Thomas Gleixner 
On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:
> On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
> Once upon a time, you told me the following:
> 
> On 15 May 2014 at 16:14, Thomas Gleixner  wrote:
> > On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:
> > > And that universe would love to have your documentation of
> > > FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),
> > 
> > I give you almost the full treatment, but I leave REQUEUE_PI to Darren
> > and FUTEX_WAKE_OP to Jakub. :)
> > [...]
> > FUTEX_CLOCK_REALTIME
> > 
> > This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
> > and FUTEX_WAIT_REQUEUE_PI
> > 
> > If set the kernel treats the user space supplied timeout as
> > absolute time based on CLOCK_REALTIME.
> > 
> > If not set the kernel treats the user space supplied timeout
> > as relative time.
> Unfortunately, I should have checked the code more carefully...

Me too :)
 
> Looking more carefully at the code, I see understand the situation
> is the following:
> 
> FUTEX_LOCK_PI
>   Always uses CLOCK_REALTIME
>   'timeout' is absolute

Yes.
 
> FUTEX_WAIT_REQUEUE_PI
>   Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
>   determined by presence or absence of
>   FUTEX_CLOCK_REALTIME flag
>   'timeout' is absolute

Yes

> FUTEX_WAIT_BITSET
>   Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
>   determined by presence or absence of
>   FUTEX_CLOCK_REALTIME flag
>   'timeout' is absolute

Yes
 
> FUTEX_WAIT
>   Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
>   determined by presence or absence of
>   FUTEX_CLOCK_REALTIME flag
>   'timeout' is relative

Yes.

> I've amended the man page to describe those details.
> 
> > The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.
> 
> When you say that the "flag was added", which flag do you mean? Or, did you
> mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
> time".

I didn't express myself clearly. When Darren added the support for
CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
support. Anything else does not make sense.

Thanks,

tglx

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Thomas Gleixner 
On Thu, 23 Jun 2016, Michael Kerrisk (man-pages) wrote:
> On 06/23/2016 09:18 AM, Thomas Gleixner wrote:
> Once upon a time, you told me the following:
> 
> On 15 May 2014 at 16:14, Thomas Gleixner  wrote:
> > On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:
> > > And that universe would love to have your documentation of
> > > FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),
> > 
> > I give you almost the full treatment, but I leave REQUEUE_PI to Darren
> > and FUTEX_WAKE_OP to Jakub. :)
> > [...]
> > FUTEX_CLOCK_REALTIME
> > 
> > This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
> > and FUTEX_WAIT_REQUEUE_PI
> > 
> > If set the kernel treats the user space supplied timeout as
> > absolute time based on CLOCK_REALTIME.
> > 
> > If not set the kernel treats the user space supplied timeout
> > as relative time.
> Unfortunately, I should have checked the code more carefully...

Me too :)
 
> Looking more carefully at the code, I see understand the situation
> is the following:
> 
> FUTEX_LOCK_PI
>   Always uses CLOCK_REALTIME
>   'timeout' is absolute

Yes.
 
> FUTEX_WAIT_REQUEUE_PI
>   Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
>   determined by presence or absence of
>   FUTEX_CLOCK_REALTIME flag
>   'timeout' is absolute

Yes

> FUTEX_WAIT_BITSET
>   Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
>   determined by presence or absence of
>   FUTEX_CLOCK_REALTIME flag
>   'timeout' is absolute

Yes
 
> FUTEX_WAIT
>   Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
>   determined by presence or absence of
>   FUTEX_CLOCK_REALTIME flag
>   'timeout' is relative

Yes.

> I've amended the man page to describe those details.
> 
> > The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.
> 
> When you say that the "flag was added", which flag do you mean? Or, did you
> mean: "applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute
> time".

I didn't express myself clearly. When Darren added the support for
CLOCK_REALTIME to FUTEX_WAIT I think he wanted to add absolute timeout
support. Anything else does not make sense.

Thanks,

tglx

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

On 06/23/2016 09:18 AM, Thomas Gleixner wrote:

On Wed, 22 Jun 2016, Darren Hart wrote:

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.


No.


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);


That breaks LOCK_PI, REQUEUE_PI and FUTEX_WAIT_BITSET


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?


FUTEX_WAIT_BITSET, LOCK_PI and REQUEUE_PI always expect absolute time in
CLOCK_REALTIME independent of the CLOCK_REALTIME flag.


Once upon a time, you told me the following:

On 15 May 2014 at 16:14, Thomas Gleixner  wrote:

On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:

And that universe would love to have your documentation of
FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),


I give you almost the full treatment, but I leave REQUEUE_PI to Darren
and FUTEX_WAKE_OP to Jakub. :)
[...]
FUTEX_CLOCK_REALTIME

This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
and FUTEX_WAIT_REQUEUE_PI

If set the kernel treats the user space supplied timeout as
absolute time based on CLOCK_REALTIME.

If not set the kernel treats the user space supplied timeout
as relative time.


Unfortunately, I should have checked the code more carefully...

Looking more carefully at the code, I see understand the situation
is the following:

FUTEX_LOCK_PI
Always uses CLOCK_REALTIME
'timeout' is absolute

FUTEX_WAIT_REQUEUE_PI
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute

FUTEX_WAIT_BITSET
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute

FUTEX_WAIT
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is relative

Right?

I've amended the man page to describe those details.


The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.


When you say that the "flag was added", which flag do you mean? Or, did you 
mean:
"applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute time".


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 = 
}


So this patch is correct and if the man page is unclear about it then we need
to fix that.


So, my fixes to the man page just now are at
http://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit/?id=8064bfa5369c6856f606004d02e48ab275e05bed

If Matthieu's patch is applied, obviously a further fix will
be needed needed in the description of FUTEX_WAIT.

Cheers,

Michael

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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

Hi Darren,

On 06/23/2016 06:48 AM, Darren Hart wrote:

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.


Done.


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 = 
}

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?


So, there clearly seem to be some things broken in the man page. See the
reply I sent to tglx.

Cheers,

Michael



[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 = 
}






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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

On 06/23/2016 09:18 AM, Thomas Gleixner wrote:

On Wed, 22 Jun 2016, Darren Hart wrote:

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.


No.


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);


That breaks LOCK_PI, REQUEUE_PI and FUTEX_WAIT_BITSET


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?


FUTEX_WAIT_BITSET, LOCK_PI and REQUEUE_PI always expect absolute time in
CLOCK_REALTIME independent of the CLOCK_REALTIME flag.


Once upon a time, you told me the following:

On 15 May 2014 at 16:14, Thomas Gleixner  wrote:

On Thu, 15 May 2014, Michael Kerrisk (man-pages) wrote:

And that universe would love to have your documentation of
FUTEX_WAKE_BITSET and FUTEX_WAIT_BITSET ;-),


I give you almost the full treatment, but I leave REQUEUE_PI to Darren
and FUTEX_WAKE_OP to Jakub. :)
[...]
FUTEX_CLOCK_REALTIME

This option bit can be ored on the futex ops FUTEX_WAIT_BITSET
and FUTEX_WAIT_REQUEUE_PI

If set the kernel treats the user space supplied timeout as
absolute time based on CLOCK_REALTIME.

If not set the kernel treats the user space supplied timeout
as relative time.


Unfortunately, I should have checked the code more carefully...

Looking more carefully at the code, I see understand the situation
is the following:

FUTEX_LOCK_PI
Always uses CLOCK_REALTIME
'timeout' is absolute

FUTEX_WAIT_REQUEUE_PI
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute

FUTEX_WAIT_BITSET
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is absolute

FUTEX_WAIT
Choice of clock (CLOCK_REALTIME vs CLOCK_MONOTONIC) is
determined by presence or absence of
FUTEX_CLOCK_REALTIME flag
'timeout' is relative

Right?

I've amended the man page to describe those details.


The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.


When you say that the "flag was added", which flag do you mean? Or, did you 
mean:
"applying Matthieu's patch will allow FUTEX_WAIT to hand in absolute time".


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 = 
}


So this patch is correct and if the man page is unclear about it then we need
to fix that.


So, my fixes to the man page just now are at
http://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit/?id=8064bfa5369c6856f606004d02e48ab275e05bed

If Matthieu's patch is applied, obviously a further fix will
be needed needed in the description of FUTEX_WAIT.

Cheers,

Michael

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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Michael Kerrisk (man-pages) 

Hi Darren,

On 06/23/2016 06:48 AM, Darren Hart wrote:

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.


Done.


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 = 
}

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?


So, there clearly seem to be some things broken in the man page. See the
reply I sent to tglx.

Cheers,

Michael



[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 = 
}






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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Thomas Gleixner 
On Wed, 22 Jun 2016, Darren Hart wrote:
> 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.

No.
 
> 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);

That breaks LOCK_PI, REQUEUE_PI and FUTEX_WAIT_BITSET

> 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?

FUTEX_WAIT_BITSET, LOCK_PI and REQUEUE_PI always expect absolute time in
CLOCK_REALTIME independent of the CLOCK_REALTIME flag.

The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.

> > 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 = 
> > }

So this patch is correct and if the man page is unclear about it then we need
to fix that.

Thanks,

tglx

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-23 Thread Thomas Gleixner 
On Wed, 22 Jun 2016, Darren Hart wrote:
> 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.

No.
 
> 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);

That breaks LOCK_PI, REQUEUE_PI and FUTEX_WAIT_BITSET

> 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?

FUTEX_WAIT_BITSET, LOCK_PI and REQUEUE_PI always expect absolute time in
CLOCK_REALTIME independent of the CLOCK_REALTIME flag.

The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.

> > 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 = 
> > }

So this patch is correct and if the man page is unclear about it then we need
to fix that.

Thanks,

tglx

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-22 Thread Darren Hart 
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 = 
}

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 = 
>   }


-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-22 Thread Darren Hart 
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 = 
}

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 = 
>   }


-- 
Darren Hart
Intel Open Source Technology Center

futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-20 Thread Matthieu CASTET 
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.

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 = 
 	}

futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2016-06-20 Thread Matthieu CASTET 
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.

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 = 
 	}

[tip:locking/core] futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2015-12-20 Thread tip-bot for Darren Hart 
Commit-ID:  337f13046ff03717a9e99675284a817527440a49
Gitweb: http://git.kernel.org/tip/337f13046ff03717a9e99675284a817527440a49
Author: Darren Hart 
AuthorDate: Fri, 18 Dec 2015 13:36:37 -0800
Committer:  Thomas Gleixner 
CommitDate: Sun, 20 Dec 2015 12:43:25 +0100

futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

While reviewing Michael Kerrisk's recent futex manpage update, I noticed
that we allow the FUTEX_CLOCK_REALTIME flag for FUTEX_WAIT_BITSET but
not for FUTEX_WAIT.

FUTEX_WAIT is treated as a simple version for FUTEX_WAIT_BITSET
internally (with a bitmask of FUTEX_BITSET_MATCH_ANY). As such, I cannot
come up with a reason for this exclusion for FUTEX_WAIT.

This change does modify the behavior of the futex syscall, changing a
call with FUTEX_WAIT | FUTEX_CLOCK_REALTIME from returning -ENOSYS, to be
equivalent to FUTEX_WAIT_BITSET | FUTEX_CLOCK_REALTIME with a bitset of
FUTEX_BITSET_MATCH_ANY.

Reported-by: Michael Kerrisk 
Signed-off-by: Darren Hart 
Cc: Peter Zijlstra 
Cc: Davidlohr Bueso 
Link: 
http://lkml.kernel.org/r/9f3bdc116d79d23f5ee72ceb9a2a857f5ff8fa29.1450474525.git.dvh...@linux.intel.com
Signed-off-by: Thomas Gleixner 
---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 461d438..8a310e2 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3084,7 +3084,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t 
*timeout,
 
if (op & FUTEX_CLOCK_REALTIME) {
flags |= FLAGS_CLOCKRT;
-   if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
+   if (cmd != FUTEX_WAIT && cmd != FUTEX_WAIT_BITSET && \
+   cmd != FUTEX_WAIT_REQUEUE_PI)
return -ENOSYS;
}
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[tip:locking/core] futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2015-12-20 Thread tip-bot for Darren Hart 
Commit-ID:  337f13046ff03717a9e99675284a817527440a49
Gitweb: http://git.kernel.org/tip/337f13046ff03717a9e99675284a817527440a49
Author: Darren Hart <dvh...@linux.intel.com>
AuthorDate: Fri, 18 Dec 2015 13:36:37 -0800
Committer:  Thomas Gleixner <t...@linutronix.de>
CommitDate: Sun, 20 Dec 2015 12:43:25 +0100

futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

While reviewing Michael Kerrisk's recent futex manpage update, I noticed
that we allow the FUTEX_CLOCK_REALTIME flag for FUTEX_WAIT_BITSET but
not for FUTEX_WAIT.

FUTEX_WAIT is treated as a simple version for FUTEX_WAIT_BITSET
internally (with a bitmask of FUTEX_BITSET_MATCH_ANY). As such, I cannot
come up with a reason for this exclusion for FUTEX_WAIT.

This change does modify the behavior of the futex syscall, changing a
call with FUTEX_WAIT | FUTEX_CLOCK_REALTIME from returning -ENOSYS, to be
equivalent to FUTEX_WAIT_BITSET | FUTEX_CLOCK_REALTIME with a bitset of
FUTEX_BITSET_MATCH_ANY.

Reported-by: Michael Kerrisk <mtk.manpa...@gmail.com>
Signed-off-by: Darren Hart <dvh...@linux.intel.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Davidlohr Bueso <d...@stgolabs.net>
Link: 
http://lkml.kernel.org/r/9f3bdc116d79d23f5ee72ceb9a2a857f5ff8fa29.1450474525.git.dvh...@linux.intel.com
Signed-off-by: Thomas Gleixner <t...@linutronix.de>
---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 461d438..8a310e2 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3084,7 +3084,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t 
*timeout,
 
if (op & FUTEX_CLOCK_REALTIME) {
flags |= FLAGS_CLOCKRT;
-   if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
+   if (cmd != FUTEX_WAIT && cmd != FUTEX_WAIT_BITSET && \
+   cmd != FUTEX_WAIT_REQUEUE_PI)
return -ENOSYS;
}
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2015-12-18 Thread Davidlohr Bueso 

On Fri, 18 Dec 2015, Darren Hart wrote:


While reviewing Michael Kerrisk's recent futex manpage update, I noticed
that we allow the FUTEX_CLOCK_REALTIME flag for FUTEX_WAIT_BITSET but
not for FUTEX_WAIT.

FUTEX_WAIT is treated as a simple version for FUTEX_WAIT_BITSET
internally (with a bitmask of FUTEX_BITSET_MATCH_ANY). As such, I cannot
come up with a reason for this exclusion for FUTEX_WAIT.

This change does modify the behavior of the futex syscall, changing a
call with FUTEX_WAIT | FUTEX_CLOCK_REALTIME from returning -ENOSYS, to be
equivalent to FUTEX_WAIT_BITSET | FUTEX_CLOCK_REALTIME with a bitset of
FUTEX_BITSET_MATCH_ANY.

Cc: Thomas Gleixner 
Cc: Peter Zijlstra 
Cc: Davidlohr Bueso 
Reported-by: Michael Kerrisk 
Signed-off-by: Darren Hart 


Acked-by: Davidlohr Bueso 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2015-12-18 Thread Darren Hart 
While reviewing Michael Kerrisk's recent futex manpage update, I noticed
that we allow the FUTEX_CLOCK_REALTIME flag for FUTEX_WAIT_BITSET but
not for FUTEX_WAIT.

FUTEX_WAIT is treated as a simple version for FUTEX_WAIT_BITSET
internally (with a bitmask of FUTEX_BITSET_MATCH_ANY). As such, I cannot
come up with a reason for this exclusion for FUTEX_WAIT.

This change does modify the behavior of the futex syscall, changing a
call with FUTEX_WAIT | FUTEX_CLOCK_REALTIME from returning -ENOSYS, to be
equivalent to FUTEX_WAIT_BITSET | FUTEX_CLOCK_REALTIME with a bitset of
FUTEX_BITSET_MATCH_ANY.

Cc: Thomas Gleixner 
Cc: Peter Zijlstra 
Cc: Davidlohr Bueso 
Reported-by: Michael Kerrisk 
Signed-off-by: Darren Hart 
---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 684d754..3c8c6d6 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3046,7 +3046,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t 
*timeout,
 
if (op & FUTEX_CLOCK_REALTIME) {
flags |= FLAGS_CLOCKRT;
-   if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
+   if (cmd != FUTEX_WAIT && cmd != FUTEX_WAIT_BITSET && \
+   cmd != FUTEX_WAIT_REQUEUE_PI)
return -ENOSYS;
}
 
-- 
2.1.4


-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2015-12-18 Thread Davidlohr Bueso 

On Fri, 18 Dec 2015, Darren Hart wrote:


While reviewing Michael Kerrisk's recent futex manpage update, I noticed
that we allow the FUTEX_CLOCK_REALTIME flag for FUTEX_WAIT_BITSET but
not for FUTEX_WAIT.

FUTEX_WAIT is treated as a simple version for FUTEX_WAIT_BITSET
internally (with a bitmask of FUTEX_BITSET_MATCH_ANY). As such, I cannot
come up with a reason for this exclusion for FUTEX_WAIT.

This change does modify the behavior of the futex syscall, changing a
call with FUTEX_WAIT | FUTEX_CLOCK_REALTIME from returning -ENOSYS, to be
equivalent to FUTEX_WAIT_BITSET | FUTEX_CLOCK_REALTIME with a bitset of
FUTEX_BITSET_MATCH_ANY.

Cc: Thomas Gleixner 
Cc: Peter Zijlstra 
Cc: Davidlohr Bueso 
Reported-by: Michael Kerrisk 
Signed-off-by: Darren Hart 


Acked-by: Davidlohr Bueso 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

2015-12-18 Thread Darren Hart 
While reviewing Michael Kerrisk's recent futex manpage update, I noticed
that we allow the FUTEX_CLOCK_REALTIME flag for FUTEX_WAIT_BITSET but
not for FUTEX_WAIT.

FUTEX_WAIT is treated as a simple version for FUTEX_WAIT_BITSET
internally (with a bitmask of FUTEX_BITSET_MATCH_ANY). As such, I cannot
come up with a reason for this exclusion for FUTEX_WAIT.

This change does modify the behavior of the futex syscall, changing a
call with FUTEX_WAIT | FUTEX_CLOCK_REALTIME from returning -ENOSYS, to be
equivalent to FUTEX_WAIT_BITSET | FUTEX_CLOCK_REALTIME with a bitset of
FUTEX_BITSET_MATCH_ANY.

Cc: Thomas Gleixner 
Cc: Peter Zijlstra 
Cc: Davidlohr Bueso 
Reported-by: Michael Kerrisk 
Signed-off-by: Darren Hart 
---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 684d754..3c8c6d6 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3046,7 +3046,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t 
*timeout,
 
if (op & FUTEX_CLOCK_REALTIME) {
flags |= FLAGS_CLOCKRT;
-   if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
+   if (cmd != FUTEX_WAIT && cmd != FUTEX_WAIT_BITSET && \
+   cmd != FUTEX_WAIT_REQUEUE_PI)
return -ENOSYS;
}
 
-- 
2.1.4


-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/