* [PATCH] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT @ 2011-04-14 16:58 Darren Hart 2011-04-14 17:09 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Darren Hart @ 2011-04-14 16:58 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: Darren Hart, Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Eric Dumazet, John Kacur The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to restart futex_wait() without a timeout after a signal. Signed-off-by: Darren Hart <dvhart@linux.intel.com> Reported-by: Tim Smith <tsmith201104@yahoo.com> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@elte.hu> CC: Eric Dumazet <eric.dumazet@gmail.com> CC: John Kacur <jkacur@redhat.com> --- kernel/futex.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index bda4157..6eac6b6 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout, return -ENOSYS; } + if (timeout) + flags |= FLAGS_HAS_TIMEOUT; + switch (cmd) { case FUTEX_WAIT: val3 = FUTEX_BITSET_MATCH_ANY; -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT 2011-04-14 16:58 [PATCH] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT Darren Hart @ 2011-04-14 17:09 ` Eric Dumazet 2011-04-14 18:45 ` [PATCH V2] " Darren Hart 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-04-14 17:09 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, Thomas Gleixner, Peter Zijlstra, Ingo Molnar, John Kacur Le jeudi 14 avril 2011 à 09:58 -0700, Darren Hart a écrit : > The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to > restart futex_wait() without a timeout after a signal. > > Signed-off-by: Darren Hart <dvhart@linux.intel.com> > Reported-by: Tim Smith <tsmith201104@yahoo.com> > Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@elte.hu> > CC: Eric Dumazet <eric.dumazet@gmail.com> > CC: John Kacur <jkacur@redhat.com> > --- > kernel/futex.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/kernel/futex.c b/kernel/futex.c > index bda4157..6eac6b6 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout, > return -ENOSYS; > } > > + if (timeout) > + flags |= FLAGS_HAS_TIMEOUT; > + > switch (cmd) { > case FUTEX_WAIT: > val3 = FUTEX_BITSET_MATCH_ANY; Hmm, could you please add in the changelog the commit id introducing the bug ? I suspect its b41277dc7a18ee332d, added in 2.6.38 ? Thanks ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT 2011-04-14 17:09 ` Eric Dumazet @ 2011-04-14 18:45 ` Darren Hart 2011-04-14 18:56 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Darren Hart @ 2011-04-14 18:45 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: Darren Hart, Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Eric Dumazet, John Kacur The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to restart futex_wait() without a timeout after a signal. Commit b41277dc7a18ee332d in 2.6.38 introduced the regression by accidentally removing the the FLAGS_HAS_TIMEOUT assignment from futex_wait() during the setup of the restart block. It makes more sense to set the flag earlier during do_futex() where the other flags are set, although futex_wait is the only user of FLAGS_HAS_TIMEOUT as it is the only op using a relative timeout (an unfortunately preexisting condition). Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922 V2: Added references to commit message. Signed-off-by: Darren Hart <dvhart@linux.intel.com> Reported-by: Tim Smith <tsmith201104@yahoo.com> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@elte.hu> CC: Eric Dumazet <eric.dumazet@gmail.com> CC: John Kacur <jkacur@redhat.com> --- kernel/futex.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index bda4157..6eac6b6 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout, return -ENOSYS; } + if (timeout) + flags |= FLAGS_HAS_TIMEOUT; + switch (cmd) { case FUTEX_WAIT: val3 = FUTEX_BITSET_MATCH_ANY; -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT 2011-04-14 18:45 ` [PATCH V2] " Darren Hart @ 2011-04-14 18:56 ` Eric Dumazet 2011-04-14 19:11 ` Darren Hart 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-04-14 18:56 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, Thomas Gleixner, Peter Zijlstra, Ingo Molnar, John Kacur Le jeudi 14 avril 2011 à 11:45 -0700, Darren Hart a écrit : > The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to > restart futex_wait() without a timeout after a signal. > > Commit b41277dc7a18ee332d in 2.6.38 introduced the regression by accidentally > removing the the FLAGS_HAS_TIMEOUT assignment from futex_wait() during the setup > of the restart block. It makes more sense to set the flag earlier during > do_futex() where the other flags are set, although futex_wait is the only user > of FLAGS_HAS_TIMEOUT as it is the only op using a relative timeout (an > unfortunately preexisting condition). > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922 > > V2: Added references to commit message. > > Signed-off-by: Darren Hart <dvhart@linux.intel.com> > Reported-by: Tim Smith <tsmith201104@yahoo.com> > Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@elte.hu> > CC: Eric Dumazet <eric.dumazet@gmail.com> > CC: John Kacur <jkacur@redhat.com> > --- > kernel/futex.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/kernel/futex.c b/kernel/futex.c > index bda4157..6eac6b6 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout, > return -ENOSYS; > } > > + if (timeout) > + flags |= FLAGS_HAS_TIMEOUT; > + > switch (cmd) { > case FUTEX_WAIT: > val3 = FUTEX_BITSET_MATCH_ANY; I dont agree with your fix. You add yet another test in futex fastpath. Prior commit b41277dc7a18ee332d, this was done only in case of restart. Could we instead use diff --git a/kernel/futex.c b/kernel/futex.c index dfb924f..fe28dc2 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -1886,7 +1886,7 @@ retry: restart->futex.val = val; restart->futex.time = abs_time->tv64; restart->futex.bitset = bitset; - restart->futex.flags = flags; + restart->futex.flags = flags | FLAGS_HAS_TIMEOUT; ret = -ERESTART_RESTARTBLOCK; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT 2011-04-14 18:56 ` Eric Dumazet @ 2011-04-14 19:11 ` Darren Hart 2011-04-14 19:48 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Darren Hart @ 2011-04-14 19:11 UTC (permalink / raw) To: Eric Dumazet Cc: Linux Kernel Mailing List, Thomas Gleixner, Peter Zijlstra, Ingo Molnar, John Kacur On 04/14/2011 11:56 AM, Eric Dumazet wrote: > Le jeudi 14 avril 2011 à 11:45 -0700, Darren Hart a écrit : >> The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to >> restart futex_wait() without a timeout after a signal. >> >> Commit b41277dc7a18ee332d in 2.6.38 introduced the regression by accidentally >> removing the the FLAGS_HAS_TIMEOUT assignment from futex_wait() during the setup >> of the restart block. It makes more sense to set the flag earlier during >> do_futex() where the other flags are set, although futex_wait is the only user >> of FLAGS_HAS_TIMEOUT as it is the only op using a relative timeout (an >> unfortunately preexisting condition). >> >> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922 >> >> V2: Added references to commit message. >> >> Signed-off-by: Darren Hart <dvhart@linux.intel.com> >> Reported-by: Tim Smith <tsmith201104@yahoo.com> >> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@elte.hu> >> CC: Eric Dumazet <eric.dumazet@gmail.com> >> CC: John Kacur <jkacur@redhat.com> >> --- >> kernel/futex.c | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/kernel/futex.c b/kernel/futex.c >> index bda4157..6eac6b6 100644 >> --- a/kernel/futex.c >> +++ b/kernel/futex.c >> @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout, >> return -ENOSYS; >> } >> >> + if (timeout) >> + flags |= FLAGS_HAS_TIMEOUT; >> + >> switch (cmd) { >> case FUTEX_WAIT: >> val3 = FUTEX_BITSET_MATCH_ANY; > > > I dont agree with your fix. You add yet another test in futex fastpath. I would say anything calling SYS_FUTEX is the futex slow path. The fast path is cmpxchg in user space. > > Prior commit b41277dc7a18ee332d, this was done only in case of restart. It was. My thinking was that it was inconsistent to have the FLAGS_HAS_TIMEOUT only available if a signal was received and a restart was required. This is the only place it is currently needed, but the inconsistency concerns me. > > Could we instead use > > diff --git a/kernel/futex.c b/kernel/futex.c > index dfb924f..fe28dc2 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -1886,7 +1886,7 @@ retry: > restart->futex.val = val; > restart->futex.time = abs_time->tv64; > restart->futex.bitset = bitset; > - restart->futex.flags = flags; > + restart->futex.flags = flags | FLAGS_HAS_TIMEOUT; > > ret = -ERESTART_RESTARTBLOCK; How about the following, it reuses an existing if block and ensure the FLAGS_HAS_TIMEOUT is always set if a timeout is used. It means the FLAG_HAS_TIMEOUT is not available in the other futex_* routines with timeouts (futex_lock_pi and futex_wait_requeue_pi), but they use absolute timeouts and don't need it for restart - I can agree to that, although I'm not keen on FLAG_HAS_TIMEOUT not being set whenever timeout is. That could be added in the same way to the other functions if needed in the future. diff --git a/kernel/futex.c b/kernel/futex.c index 6eac6b6..0a7eefd 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -1846,6 +1846,7 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val, hrtimer_init_sleeper(to, current); hrtimer_set_expires_range_ns(&to->timer, *abs_time, current->timer_slack_ns); + flags |= FLAGS_HAS_TIMEOUT; } retry: -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT 2011-04-14 19:11 ` Darren Hart @ 2011-04-14 19:48 ` Eric Dumazet 2011-04-14 20:45 ` Darren Hart 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-04-14 19:48 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, Thomas Gleixner, Peter Zijlstra, Ingo Molnar, John Kacur Le jeudi 14 avril 2011 à 12:11 -0700, Darren Hart a écrit : > I would say anything calling SYS_FUTEX is the futex slow path. The fast > path is cmpxchg in user space. > Thats not a good reason to make it slower than necessary... > It was. My thinking was that it was inconsistent to have the > FLAGS_HAS_TIMEOUT only available if a signal was received and a restart > was required. This is the only place it is currently needed, but the > inconsistency concerns me. > I dont call this inconsistency, but right place for the code. > How about the following, it reuses an existing if block and ensure the > FLAGS_HAS_TIMEOUT is always set if a timeout is used. It means the > FLAG_HAS_TIMEOUT is not available in the other futex_* routines with > timeouts (futex_lock_pi and futex_wait_requeue_pi), but they use absolute > timeouts and don't need it for restart - I can agree to that, although > I'm not keen on FLAG_HAS_TIMEOUT not being set whenever timeout is. That > could be added in the same way to the other functions if needed in the > future. I dont understand why you insist setting in fast path a flag that is useless, unless we hit restart logic [ What I call the slow path in futex syscall ] It seems more natural and efficient to me to go back to previous code. Maybe rename FLAG_HAS_TIMEOUT to FLAG_HAS_TIMEOUT_ON_RESTART if you want, to make clear what is the meaning of this flag. Now if you have plans to use this flag in futex code, outside of restart logic, please share them with us :) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT 2011-04-14 19:48 ` Eric Dumazet @ 2011-04-14 20:45 ` Darren Hart 0 siblings, 0 replies; 7+ messages in thread From: Darren Hart @ 2011-04-14 20:45 UTC (permalink / raw) To: Eric Dumazet Cc: Linux Kernel Mailing List, Thomas Gleixner, Peter Zijlstra, Ingo Molnar, John Kacur On 04/14/2011 12:48 PM, Eric Dumazet wrote: > Le jeudi 14 avril 2011 à 12:11 -0700, Darren Hart a écrit : > >> I would say anything calling SYS_FUTEX is the futex slow path. The fast >> path is cmpxchg in user space. >> > > Thats not a good reason to make it slower than necessary... > >> It was. My thinking was that it was inconsistent to have the >> FLAGS_HAS_TIMEOUT only available if a signal was received and a restart >> was required. This is the only place it is currently needed, but the >> inconsistency concerns me. >> > > I dont call this inconsistency, but right place for the code. > >> How about the following, it reuses an existing if block and ensure the >> FLAGS_HAS_TIMEOUT is always set if a timeout is used. It means the >> FLAG_HAS_TIMEOUT is not available in the other futex_* routines with >> timeouts (futex_lock_pi and futex_wait_requeue_pi), but they use absolute >> timeouts and don't need it for restart - I can agree to that, although >> I'm not keen on FLAG_HAS_TIMEOUT not being set whenever timeout is. That >> could be added in the same way to the other functions if needed in the >> future. > > I dont understand why you insist setting in fast path a flag that is > useless, unless we hit restart logic [ What I call the slow path in > futex syscall ] I'm not particularly attached to this approach, I just felt it made more sense. Your initial objection was to the test in the do_futex(), so I avoided the test by moving it into futex_wait(). The addition of an |= to an existing test block didn't seem significant to me in this path. But, it isn't important enough to me to argue the point. > > It seems more natural and efficient to me to go back to previous code. > > Maybe rename FLAG_HAS_TIMEOUT to FLAG_HAS_TIMEOUT_ON_RESTART if you > want, to make clear what is the meaning of this flag. > > Now if you have plans to use this flag in futex code, outside of restart > logic, please share them with us :) Nope, no plans, and there is value in simply restoring the original behavior, especially as this should go to stable as well. I've resubmitted the patch with the "restart-block only approach" and included "stable". Thanks for the feedback, -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-04-14 20:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-14 16:58 [PATCH] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT Darren Hart 2011-04-14 17:09 ` Eric Dumazet 2011-04-14 18:45 ` [PATCH V2] " Darren Hart 2011-04-14 18:56 ` Eric Dumazet 2011-04-14 19:11 ` Darren Hart 2011-04-14 19:48 ` Eric Dumazet 2011-04-14 20:45 ` Darren Hart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).