Re: [PATCH/RFC] wait_for_completion_timeout() considered harmful.
On Tue, 19 Nov 2013, Peter Zijlstra wrote: > On Tue, Nov 19, 2013 at 07:58:51PM +1100, NeilBrown wrote: > > /* > > * TODO: Make sure that we wait at least required delay but why we > > * have to extend it one tick more? > > */ > > schedule_timeout_interruptible(msecs_to_jiffies(delay) + 2); > > What makes me sad is that clearly people knew stuff was broken but > somehow it never got properly fixed. > > Yes, changing something like this is risky, but I prefer to fix the > implementation to the sane and documented behaviour and fix up whatever > fallout that generates. The end result is saner code in general and less > new bugs. The underlying issue of the timer wheel is that it is driven by a coarse grained tick. So a schedule_timeout(1) is guaranteed to sleep until the next tick arrives. There is no way to figure out whether the timer was started right at the beginning of a tick period or right at the end. This has been the case from day one of the timer wheel. The user space interfaces [clock_]nanosleep and the various posix timers had countermeasures based on gettimeofday() to guarantee the correct behaviour. glibc still has some extra checks around some timeout related syscalls for that reason. We killed that mess when we converted them over to hrtimers. So if you really want to make sure that you slept at minimum the given number of ticks with the timer wheel, then you have only one choice: Add unconditionally 1 to the given number of ticks I don't think that this will cause a massive fallout as the timing of the timer_list timers already varies by an order of magnitude due to the HZ settings. So anything which relies on the timer wheel in terms of precision is hosed already. So adding 1 will just hose it some more. Thanks, tglx -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Tue, Nov 19, 2013 at 07:58:51PM +1100, NeilBrown wrote: > /* >* TODO: Make sure that we wait at least required delay but why we >* have to extend it one tick more? >*/ > schedule_timeout_interruptible(msecs_to_jiffies(delay) + 2); What makes me sad is that clearly people knew stuff was broken but somehow it never got properly fixed. Yes, changing something like this is risky, but I prefer to fix the implementation to the sane and documented behaviour and fix up whatever fallout that generates. The end result is saner code in general and less new bugs. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Tue, 19 Nov 2013 09:25:48 +0100 Peter Zijlstra wrote: > On Tue, Nov 19, 2013 at 10:44:38AM +1100, NeilBrown wrote: > > We have loops that have > > timeout = schedule_timeout(timeout) > > in the middle and if we change the semantics of schedule_timeout() to round > > up, those loops could wait quite a bit longer than expected. > > Depends on what you expect; most of these functions have documentation > that says they will sleep at least timeout amount of time. > > schedule_timeout()'s version looks like: > > * Make the current task sleep until @timeout jiffies have > * elapsed. > > Clearly it doesn't do that currently, so adding 1 will actually make it > do what it says on the tin. Hmm.. maybe you are right, and I now see that my argument about loops isn't nearly as convincing as it seemed at the time. However there is still the risk that someone wrote code depending on the current behaviour rather than the current documentation. msleep() is one such example (it has a "+1") but that is easily found and fixed. I went hunting for code that uses a timeout of '1' which would be affected the most (I found no use cases for '0'). Two interesting examples: drivers/video/via/via-core.c: /* * Now we just wait until the interrupt handler says * we're done. Except that, actually, we need to wait a little * longer: the interrupts seem to jump the gun a little and we * get corrupted frames sometimes. */ wait_for_completion_timeout(_dma_completion, 1); msleep(1); Maybe the 'msleep(1)' was only needed because of this 'bug' - Jon might know. In that case the comment and msleep could get removed. drivers/misc/sgi-xp/xpc_channel.c: /* * Wait for a message entry to become available for the specified channel, * but don't wait any longer than 1 jiffy. */ . atomic_inc(>n_on_msg_allocate_wq); ret = interruptible_sleep_on_timeout(>msg_allocate_wq, 1); atomic_dec(>n_on_msg_allocate_wq); If interruptible_sleep_on_timeout were affected by the proposed change, then the code would no longer match the comment. That code is five years old ... I wonder Dean remembers if it is important ... or is even still at SGI. Also git grep '_timeout[_a-z]*(.*msecs_to_jiffies(1)' finds 10 matches which would be significantly affected. I can't easily say if it would be for the better or not. Another random piece of code that maybe could get simplified: drivers/iio/light/tsl2563.c: /* * TODO: Make sure that we wait at least required delay but why we * have to extend it one tick more? */ schedule_timeout_interruptible(msecs_to_jiffies(delay) + 2); NeilBrown signature.asc Description: PGP signature
Re: [PATCH/RFC] wait_for_completion_timeout() considered harmful.
On Mon, Nov 18, 2013 at 05:49:02PM -0700, Jonathan Corbet wrote: > On Tue, 19 Nov 2013 00:42:09 +0100 > Peter Zijlstra wrote: > > > I briefly talked to Thomas about this earlier today and we need to fix > > this at a lower level -- the quick 'n dirty solution is to add 1 jiffy > > down in the timer-wheel when we enqueue these things. > > That can lead to situations like the one I encountered years ago where > msleep(1) would snooze for 20ms. No, for up to 20ms, since you get part of the first jiffy, which was the whole problem (assuming we're talking HZ=100 here). > I didn't get much love for my idea of > switching msleep() to hrtimers back then, but I still think it might be be > better to provide the resolution that the interface appears to promise. It would actually make more sense to use hrtimers here than it does to use the old timers since hrtimers has much better gurantees but also because the timer is guaranteed to fire. The use case where the old timers really beat hrtimers hands down is for timeout timers that never actually fire. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Tue, Nov 19, 2013 at 10:44:38AM +1100, NeilBrown wrote: > We have loops that have > timeout = schedule_timeout(timeout) > in the middle and if we change the semantics of schedule_timeout() to round > up, those loops could wait quite a bit longer than expected. Depends on what you expect; most of these functions have documentation that says they will sleep at least timeout amount of time. schedule_timeout()'s version looks like: * Make the current task sleep until @timeout jiffies have * elapsed. Clearly it doesn't do that currently, so adding 1 will actually make it do what it says on the tin. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Tue, Nov 19, 2013 at 10:44:38AM +1100, NeilBrown wrote: We have loops that have timeout = schedule_timeout(timeout) in the middle and if we change the semantics of schedule_timeout() to round up, those loops could wait quite a bit longer than expected. Depends on what you expect; most of these functions have documentation that says they will sleep at least timeout amount of time. schedule_timeout()'s version looks like: * Make the current task sleep until @timeout jiffies have * elapsed. Clearly it doesn't do that currently, so adding 1 will actually make it do what it says on the tin. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Mon, Nov 18, 2013 at 05:49:02PM -0700, Jonathan Corbet wrote: On Tue, 19 Nov 2013 00:42:09 +0100 Peter Zijlstra pet...@infradead.org wrote: I briefly talked to Thomas about this earlier today and we need to fix this at a lower level -- the quick 'n dirty solution is to add 1 jiffy down in the timer-wheel when we enqueue these things. That can lead to situations like the one I encountered years ago where msleep(1) would snooze for 20ms. No, for up to 20ms, since you get part of the first jiffy, which was the whole problem (assuming we're talking HZ=100 here). I didn't get much love for my idea of switching msleep() to hrtimers back then, but I still think it might be be better to provide the resolution that the interface appears to promise. It would actually make more sense to use hrtimers here than it does to use the old timers since hrtimers has much better gurantees but also because the timer is guaranteed to fire. The use case where the old timers really beat hrtimers hands down is for timeout timers that never actually fire. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Tue, 19 Nov 2013 09:25:48 +0100 Peter Zijlstra pet...@infradead.org wrote: On Tue, Nov 19, 2013 at 10:44:38AM +1100, NeilBrown wrote: We have loops that have timeout = schedule_timeout(timeout) in the middle and if we change the semantics of schedule_timeout() to round up, those loops could wait quite a bit longer than expected. Depends on what you expect; most of these functions have documentation that says they will sleep at least timeout amount of time. schedule_timeout()'s version looks like: * Make the current task sleep until @timeout jiffies have * elapsed. Clearly it doesn't do that currently, so adding 1 will actually make it do what it says on the tin. Hmm.. maybe you are right, and I now see that my argument about loops isn't nearly as convincing as it seemed at the time. However there is still the risk that someone wrote code depending on the current behaviour rather than the current documentation. msleep() is one such example (it has a +1) but that is easily found and fixed. I went hunting for code that uses a timeout of '1' which would be affected the most (I found no use cases for '0'). Two interesting examples: drivers/video/via/via-core.c: /* * Now we just wait until the interrupt handler says * we're done. Except that, actually, we need to wait a little * longer: the interrupts seem to jump the gun a little and we * get corrupted frames sometimes. */ wait_for_completion_timeout(viafb_dma_completion, 1); msleep(1); Maybe the 'msleep(1)' was only needed because of this 'bug' - Jon might know. In that case the comment and msleep could get removed. drivers/misc/sgi-xp/xpc_channel.c: /* * Wait for a message entry to become available for the specified channel, * but don't wait any longer than 1 jiffy. */ . atomic_inc(ch-n_on_msg_allocate_wq); ret = interruptible_sleep_on_timeout(ch-msg_allocate_wq, 1); atomic_dec(ch-n_on_msg_allocate_wq); If interruptible_sleep_on_timeout were affected by the proposed change, then the code would no longer match the comment. That code is five years old ... I wonder Dean remembers if it is important ... or is even still at SGI. Also git grep '_timeout[_a-z]*(.*msecs_to_jiffies(1)' finds 10 matches which would be significantly affected. I can't easily say if it would be for the better or not. Another random piece of code that maybe could get simplified: drivers/iio/light/tsl2563.c: /* * TODO: Make sure that we wait at least required delay but why we * have to extend it one tick more? */ schedule_timeout_interruptible(msecs_to_jiffies(delay) + 2); NeilBrown signature.asc Description: PGP signature
Re: [PATCH/RFC] wait_for_completion_timeout() considered harmful.
On Tue, Nov 19, 2013 at 07:58:51PM +1100, NeilBrown wrote: /* * TODO: Make sure that we wait at least required delay but why we * have to extend it one tick more? */ schedule_timeout_interruptible(msecs_to_jiffies(delay) + 2); What makes me sad is that clearly people knew stuff was broken but somehow it never got properly fixed. Yes, changing something like this is risky, but I prefer to fix the implementation to the sane and documented behaviour and fix up whatever fallout that generates. The end result is saner code in general and less new bugs. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Tue, 19 Nov 2013, Peter Zijlstra wrote: On Tue, Nov 19, 2013 at 07:58:51PM +1100, NeilBrown wrote: /* * TODO: Make sure that we wait at least required delay but why we * have to extend it one tick more? */ schedule_timeout_interruptible(msecs_to_jiffies(delay) + 2); What makes me sad is that clearly people knew stuff was broken but somehow it never got properly fixed. Yes, changing something like this is risky, but I prefer to fix the implementation to the sane and documented behaviour and fix up whatever fallout that generates. The end result is saner code in general and less new bugs. The underlying issue of the timer wheel is that it is driven by a coarse grained tick. So a schedule_timeout(1) is guaranteed to sleep until the next tick arrives. There is no way to figure out whether the timer was started right at the beginning of a tick period or right at the end. This has been the case from day one of the timer wheel. The user space interfaces [clock_]nanosleep and the various posix timers had countermeasures based on gettimeofday() to guarantee the correct behaviour. glibc still has some extra checks around some timeout related syscalls for that reason. We killed that mess when we converted them over to hrtimers. So if you really want to make sure that you slept at minimum the given number of ticks with the timer wheel, then you have only one choice: Add unconditionally 1 to the given number of ticks I don't think that this will cause a massive fallout as the timing of the timer_list timers already varies by an order of magnitude due to the HZ settings. So anything which relies on the timer wheel in terms of precision is hosed already. So adding 1 will just hose it some more. Thanks, tglx -- 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/RFC] wait_for_completion_timeout() considered harmful.
* Jonathan Corbet wrote: > On Tue, 19 Nov 2013 00:42:09 +0100 > Peter Zijlstra wrote: > > > I briefly talked to Thomas about this earlier today and we need to > > fix this at a lower level -- the quick 'n dirty solution is to add > > 1 jiffy down in the timer-wheel when we enqueue these things. > > That can lead to situations like the one I encountered years ago > where msleep(1) would snooze for 20ms. I didn't get much love for > my idea of switching msleep() to hrtimers back then, but I still > think it might be be better to provide the resolution that the > interface appears to promise. That looks like a sensible approach - mind resending that patch? We can put it into the timer tree and see what happens. Thanks, Ingo -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Tue, 19 Nov 2013 00:42:09 +0100 Peter Zijlstra wrote: > I briefly talked to Thomas about this earlier today and we need to fix > this at a lower level -- the quick 'n dirty solution is to add 1 jiffy > down in the timer-wheel when we enqueue these things. That can lead to situations like the one I encountered years ago where msleep(1) would snooze for 20ms. I didn't get much love for my idea of switching msleep() to hrtimers back then, but I still think it might be be better to provide the resolution that the interface appears to promise. jon -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Mon, 18 Nov 2013 15:27:46 -0800 Andrew Morton wrote: > On Sun, 17 Nov 2013 08:06:03 +1100 NeilBrown wrote: > > > It would be reasonable to assume that > > > > wait_for_completion_timeout(>auxadc_done, > > msecs_to_jiffies(5)); > > > > would wait at least 5 msecs for the auxadc_done to complete. But it does > > not. > > With a HZ of 200 or less, msecs_to_jiffies(5) has value '1', and so this > > will only wait until the next "timer tick", which could happen immediately. > > > > This can lead to incorrect results - and has done so in out-of-tree patches > > for drivers/misc/bmp085.c which uses a very similar construct to enable > > interrupt > > based result collection. > > > > The documentation for several *_timeout* functions claim they will wait for > > "timeout jiffies" to have elapsed where this is not the case. They will > > actually wait for "timeout" jiffies to have started implying an elapsed time > > between (timeout-1) and (timeout). > > > > This patch corrects some of this documentation, and adds a collection of > > wait_for_completion*_msecs() > > interfaces which wait at least the given number of milliseconds for the > > completion (or a signal). > > Mutter. wait_for_x(..., 5ms) should wait for a minimum of 5ms, no matter > what. > > So I'd suggest we make that happen, rather than adding some new interfaces? I thought of that. It would certainly be nice. However what we have is XXX_timeout(, jiffies). And if we decided that XXX_timeout(, msecs_to_jiffies(5)) would only timeout after at least 5ms, then schedule_timeout(1) would have to wait at least one full jiffie, which is quite different to what it currently does. We have loops that have timeout = schedule_timeout(timeout) in the middle and if we change the semantics of schedule_timeout() to round up, those loops could wait quite a bit longer than expected. So I think that we do need to add new interfaces just like msleep() was introduced a while back to fix all the various misuses of schedule_timeout(msecs_to_jiffies(XX))). Possibly we can also discard old bad interfaces. Maybe the *_timeout() interfaces should become *_until() where the jiffies number isn't a count but is a value that we wait for "jiffies" to exceed. I don't think there is a really easy solution, but thanks for pushing the discussion along towards trying to understand one. NeilBrown signature.asc Description: PGP signature
Re: [PATCH/RFC] wait_for_completion_timeout() considered harmful.
On Mon, Nov 18, 2013 at 03:27:46PM -0800, Andrew Morton wrote: > On Sun, 17 Nov 2013 08:06:03 +1100 NeilBrown wrote: > > > It would be reasonable to assume that > > > > wait_for_completion_timeout(>auxadc_done, > > msecs_to_jiffies(5)); > > > > would wait at least 5 msecs for the auxadc_done to complete. But it does > > not. > > With a HZ of 200 or less, msecs_to_jiffies(5) has value '1', and so this > > will only wait until the next "timer tick", which could happen immediately. > > > > This can lead to incorrect results - and has done so in out-of-tree patches > > for drivers/misc/bmp085.c which uses a very similar construct to enable > > interrupt > > based result collection. > > > > The documentation for several *_timeout* functions claim they will wait for > > "timeout jiffies" to have elapsed where this is not the case. They will > > actually wait for "timeout" jiffies to have started implying an elapsed time > > between (timeout-1) and (timeout). > > > > This patch corrects some of this documentation, and adds a collection of > > wait_for_completion*_msecs() > > interfaces which wait at least the given number of milliseconds for the > > completion (or a signal). > > Mutter. wait_for_x(..., 5ms) should wait for a minimum of 5ms, no matter > what. > > So I'd suggest we make that happen, rather than adding some new interfaces? Yeah, also the completion interface is just one of many that's buggered this way. I briefly talked to Thomas about this earlier today and we need to fix this at a lower level -- the quick 'n dirty solution is to add 1 jiffy down in the timer-wheel when we enqueue these things. And yes, we very much don't want to create new interfaces with similar but slightly different semantics, that's just asking for trouble. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Sun, 17 Nov 2013 08:06:03 +1100 NeilBrown wrote: > It would be reasonable to assume that > > wait_for_completion_timeout(>auxadc_done, msecs_to_jiffies(5)); > > would wait at least 5 msecs for the auxadc_done to complete. But it does not. > With a HZ of 200 or less, msecs_to_jiffies(5) has value '1', and so this > will only wait until the next "timer tick", which could happen immediately. > > This can lead to incorrect results - and has done so in out-of-tree patches > for drivers/misc/bmp085.c which uses a very similar construct to enable > interrupt > based result collection. > > The documentation for several *_timeout* functions claim they will wait for > "timeout jiffies" to have elapsed where this is not the case. They will > actually wait for "timeout" jiffies to have started implying an elapsed time > between (timeout-1) and (timeout). > > This patch corrects some of this documentation, and adds a collection of > wait_for_completion*_msecs() > interfaces which wait at least the given number of milliseconds for the > completion (or a signal). Mutter. wait_for_x(..., 5ms) should wait for a minimum of 5ms, no matter what. So I'd suggest we make that happen, rather than adding some new interfaces? -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Sun, 17 Nov 2013 08:06:03 +1100 NeilBrown ne...@suse.de wrote: It would be reasonable to assume that wait_for_completion_timeout(wm8350-auxadc_done, msecs_to_jiffies(5)); would wait at least 5 msecs for the auxadc_done to complete. But it does not. With a HZ of 200 or less, msecs_to_jiffies(5) has value '1', and so this will only wait until the next timer tick, which could happen immediately. This can lead to incorrect results - and has done so in out-of-tree patches for drivers/misc/bmp085.c which uses a very similar construct to enable interrupt based result collection. The documentation for several *_timeout* functions claim they will wait for timeout jiffies to have elapsed where this is not the case. They will actually wait for timeout jiffies to have started implying an elapsed time between (timeout-1) and (timeout). This patch corrects some of this documentation, and adds a collection of wait_for_completion*_msecs() interfaces which wait at least the given number of milliseconds for the completion (or a signal). Mutter. wait_for_x(..., 5ms) should wait for a minimum of 5ms, no matter what. So I'd suggest we make that happen, rather than adding some new interfaces? -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Mon, Nov 18, 2013 at 03:27:46PM -0800, Andrew Morton wrote: On Sun, 17 Nov 2013 08:06:03 +1100 NeilBrown ne...@suse.de wrote: It would be reasonable to assume that wait_for_completion_timeout(wm8350-auxadc_done, msecs_to_jiffies(5)); would wait at least 5 msecs for the auxadc_done to complete. But it does not. With a HZ of 200 or less, msecs_to_jiffies(5) has value '1', and so this will only wait until the next timer tick, which could happen immediately. This can lead to incorrect results - and has done so in out-of-tree patches for drivers/misc/bmp085.c which uses a very similar construct to enable interrupt based result collection. The documentation for several *_timeout* functions claim they will wait for timeout jiffies to have elapsed where this is not the case. They will actually wait for timeout jiffies to have started implying an elapsed time between (timeout-1) and (timeout). This patch corrects some of this documentation, and adds a collection of wait_for_completion*_msecs() interfaces which wait at least the given number of milliseconds for the completion (or a signal). Mutter. wait_for_x(..., 5ms) should wait for a minimum of 5ms, no matter what. So I'd suggest we make that happen, rather than adding some new interfaces? Yeah, also the completion interface is just one of many that's buggered this way. I briefly talked to Thomas about this earlier today and we need to fix this at a lower level -- the quick 'n dirty solution is to add 1 jiffy down in the timer-wheel when we enqueue these things. And yes, we very much don't want to create new interfaces with similar but slightly different semantics, that's just asking for trouble. -- 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/RFC] wait_for_completion_timeout() considered harmful.
On Mon, 18 Nov 2013 15:27:46 -0800 Andrew Morton a...@linux-foundation.org wrote: On Sun, 17 Nov 2013 08:06:03 +1100 NeilBrown ne...@suse.de wrote: It would be reasonable to assume that wait_for_completion_timeout(wm8350-auxadc_done, msecs_to_jiffies(5)); would wait at least 5 msecs for the auxadc_done to complete. But it does not. With a HZ of 200 or less, msecs_to_jiffies(5) has value '1', and so this will only wait until the next timer tick, which could happen immediately. This can lead to incorrect results - and has done so in out-of-tree patches for drivers/misc/bmp085.c which uses a very similar construct to enable interrupt based result collection. The documentation for several *_timeout* functions claim they will wait for timeout jiffies to have elapsed where this is not the case. They will actually wait for timeout jiffies to have started implying an elapsed time between (timeout-1) and (timeout). This patch corrects some of this documentation, and adds a collection of wait_for_completion*_msecs() interfaces which wait at least the given number of milliseconds for the completion (or a signal). Mutter. wait_for_x(..., 5ms) should wait for a minimum of 5ms, no matter what. So I'd suggest we make that happen, rather than adding some new interfaces? I thought of that. It would certainly be nice. However what we have is XXX_timeout(, jiffies). And if we decided that XXX_timeout(, msecs_to_jiffies(5)) would only timeout after at least 5ms, then schedule_timeout(1) would have to wait at least one full jiffie, which is quite different to what it currently does. We have loops that have timeout = schedule_timeout(timeout) in the middle and if we change the semantics of schedule_timeout() to round up, those loops could wait quite a bit longer than expected. So I think that we do need to add new interfaces just like msleep() was introduced a while back to fix all the various misuses of schedule_timeout(msecs_to_jiffies(XX))). Possibly we can also discard old bad interfaces. Maybe the *_timeout() interfaces should become *_until() where the jiffies number isn't a count but is a value that we wait for jiffies to exceed. I don't think there is a really easy solution, but thanks for pushing the discussion along towards trying to understand one. NeilBrown signature.asc Description: PGP signature
Re: [PATCH/RFC] wait_for_completion_timeout() considered harmful.
On Tue, 19 Nov 2013 00:42:09 +0100 Peter Zijlstra pet...@infradead.org wrote: I briefly talked to Thomas about this earlier today and we need to fix this at a lower level -- the quick 'n dirty solution is to add 1 jiffy down in the timer-wheel when we enqueue these things. That can lead to situations like the one I encountered years ago where msleep(1) would snooze for 20ms. I didn't get much love for my idea of switching msleep() to hrtimers back then, but I still think it might be be better to provide the resolution that the interface appears to promise. jon -- 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/RFC] wait_for_completion_timeout() considered harmful.
* Jonathan Corbet cor...@lwn.net wrote: On Tue, 19 Nov 2013 00:42:09 +0100 Peter Zijlstra pet...@infradead.org wrote: I briefly talked to Thomas about this earlier today and we need to fix this at a lower level -- the quick 'n dirty solution is to add 1 jiffy down in the timer-wheel when we enqueue these things. That can lead to situations like the one I encountered years ago where msleep(1) would snooze for 20ms. I didn't get much love for my idea of switching msleep() to hrtimers back then, but I still think it might be be better to provide the resolution that the interface appears to promise. That looks like a sensible approach - mind resending that patch? We can put it into the timer tree and see what happens. Thanks, Ingo -- 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/
