lkml.org 
[lkml]   [2016]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op
From
Date
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/

\
 
 \ /
  Last update: 2016-06-23 13:41    [W:0.119 / U:1.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site