Messages in this thread Patch in this message | | | Date | Thu, 23 Jun 2016 13:31:44 -0700 | From | Darren Hart <> | Subject | Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op |
| |
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
| |