futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[Matthieu CASTET]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

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

[-- Attachment #2: diff --]
[-- Type: text/x-patch, Size: 394 bytes --]

diff --git a/kernel/futex.c b/kernel/futex.c
index 33664f7..4bee915 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3230,7 +3230,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 			return -EINVAL;
 
 		t = timespec_to_ktime(ts);
-		if (cmd == FUTEX_WAIT)
+		if (cmd == FUTEX_WAIT && !(op & FUTEX_CLOCK_REALTIME))
 			t = ktime_add_safe(ktime_get(), t);
 		tp = &t;
 	}

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[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 = &t;
 	}

The concern for me is whether the code is incorrect, or if the man page is
incorrect. Does existing userspace code expect the FUTEX_WAIT_BITSET op to
always use an absolute timeout, regardless of the CLOCK used?





> 
> Matthieu
> 
> [1]
>         if (cmd == FUTEX_WAIT)
>             t = ktime_add_safe(ktime_get(), t);

> diff --git a/kernel/futex.c b/kernel/futex.c
> index 33664f7..4bee915 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3230,7 +3230,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
>  			return -EINVAL;
>  
>  		t = timespec_to_ktime(ts);
> -		if (cmd == FUTEX_WAIT)
> +		if (cmd == FUTEX_WAIT && !(op & FUTEX_CLOCK_REALTIME))
>  			t = ktime_add_safe(ktime_get(), t);
>  		tp = &t;
>  	}


-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[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 = &t;
> >  	}

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

[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 <tglx@linutronix.de> 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 = &t;
>>>  	}
>
> 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

[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 <tglx@linutronix.de> 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

[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 <tglx@linutronix.de> 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

[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

[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 = &t;
 	}
 	/*

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

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[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 = &t;
>  	}
>  	/*

Just in the interests of readability/maintainability, might it not
make some sense to recode the timeout handling for FUTEX_WAIT
within futex_wait(). I think that part of the reason we're in this
mess of inconsistency is that timeout interpretation is being handled
at too many different points in the code.

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

Yes, that's another weird inconsistency.

Thanks,

Michael


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

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[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 = &t;
> >  	}
> >  	/*
> 
> Just in the interests of readability/maintainability, might it not
> make some sense to recode the timeout handling for FUTEX_WAIT
> within futex_wait(). I think that part of the reason we're in this
> mess of inconsistency is that timeout interpretation is being handled
> at too many different points in the code.


I agree, that is indeed why I missed it in my original patch.


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

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[Darren Hart]

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

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


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

-- 
Darren Hart
Intel Open Source Technology Center

Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 603 bytes --]

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

[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

[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

[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 <tglx@linutronix.de> 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

[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 <tglx@linutronix.de> 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

[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 <tglx@linutronix.de> 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

[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 = &t;
>  	}
>
> The concern for me is whether the code is incorrect, or if the man page is
> incorrect. Does existing userspace code expect the FUTEX_WAIT_BITSET op to
> always use an absolute timeout, regardless of the CLOCK used?

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 = &t;
>>  	}
>
>


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