timer: Added usleep[_range] timer
in [Kernel]
|
Prev: math-emu: correct test for downshifting fraction in _FP_FROM_INT()
Next: drivers/media/video: Remove dead CONFIG_OLPC_X0_1
From: Arjan van de Ven on 19 Jul 2010 18:30 On 7/19/2010 3:12 PM, Patrick Pannuto wrote: > >> I like the general idea, but I kinda doubt the "interruptible" version >> makes sense for this... >> do you have any users for that in mind? if not... can we leave the >> interruptible version out... makes it simpler by a lot. >> >> > Respun without interruptible... > > > From 08b717cf4f76d7578cb20c1b1444d89a9331ce02 Mon Sep 17 00:00:00 2001 > From: Patrick Pannuto<ppannuto(a)codeaurora.org> > Date: Mon, 19 Jul 2010 15:09:26 -0700 > Subject: [PATCH] timer: Added usleep[_range] timer > > usleep[_range] are finer precision implementations of msleep > and are designed to be drop-in replacements for udelay where > a precise sleep / busy-wait is unnecessary. They also allow > an easy interface to specify slack when a precise (ish) > wakeup is unnecessary to help minimize wakeups > > excellent Acked-by: Arjan van de Ven <arjan(a)linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 22 Jul 2010 19:20 On Mon, 19 Jul 2010 15:12:34 -0700 Patrick Pannuto <ppannuto(a)codeaurora.org> wrote: > Respun without interruptible... > And without all the nice changelog info. Oh well. > usleep[_range] are finer precision implementations of msleep > and are designed to be drop-in replacements for udelay where > a precise sleep / busy-wait is unnecessary. They also allow > an easy interface to specify slack when a precise (ish) > wakeup is unnecessary to help minimize wakeups > > Signed-off-by: Patrick Pannuto <ppannuto(a)codeaurora.org> > --- > include/linux/delay.h | 6 ++++++ > kernel/timer.c | 22 ++++++++++++++++++++++ > 2 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/include/linux/delay.h b/include/linux/delay.h > index fd832c6..0e303d1 100644 > --- a/include/linux/delay.h > +++ b/include/linux/delay.h > @@ -45,6 +45,12 @@ extern unsigned long lpj_fine; > void calibrate_delay(void); > void msleep(unsigned int msecs); > unsigned long msleep_interruptible(unsigned int msecs); > +void usleep_range(unsigned long min, unsigned long max); > + > +static inline void usleep(unsigned long usecs) > +{ > + usleep_range(usecs, usecs); > +} > > static inline void ssleep(unsigned int seconds) > { > diff --git a/kernel/timer.c b/kernel/timer.c > index ee305c8..4e6746f 100644 > --- a/kernel/timer.c > +++ b/kernel/timer.c > @@ -1750,3 +1750,25 @@ unsigned long msleep_interruptible(unsigned int msecs) > } > > EXPORT_SYMBOL(msleep_interruptible); > + > +static int __sched do_usleep_range(unsigned long min, unsigned long max) > +{ > + ktime_t kmin; > + unsigned long delta; > + > + kmin = ktime_set(0, min * NSEC_PER_USEC); > + delta = max - min; > + return schedule_hrtimeout_range(&kmin, delta, HRTIMER_MODE_REL); > +} > + > +/** > + * usleep_range - Drop in replacement for udelay where wakeup is flexible > + * @min: Minimum time in usecs to sleep > + * @max: Maximum time in usecs to sleep > + */ > +void usleep_range(unsigned long min, unsigned long max) > +{ > + __set_current_state(TASK_UNINTERRUPTIBLE); > + do_usleep_range(min, max); > +} > +EXPORT_SYMBOL(usleep_range); Fair enough, I guess. People can go over and look at the schedule_hrtimeout_range() documentation to work out why on earth they should specify a "range". Of course, nobody will actually think to _use_ this thing. Someone owes me a "usleep_range is preferred over usleep" checkpatch rule! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 28 Jul 2010 16:30 On Wed, 28 Jul 2010 12:33:04 -0700 Patrick Pannuto <ppannuto(a)codeaurora.org> wrote: > diff --git a/include/linux/delay.h b/include/linux/delay.h > index fd832c6..2538c95 100644 > --- a/include/linux/delay.h > +++ b/include/linux/delay.h > @@ -45,6 +45,12 @@ extern unsigned long lpj_fine; > void calibrate_delay(void); > void msleep(unsigned int msecs); > unsigned long msleep_interruptible(unsigned int msecs); > +void usleep_range(unsigned long min, unsigned long max); > + > +static inline void usleep(unsigned long usecs) > +{ > + usleep_range(usecs, usecs * 2); > +} > > static inline void ssleep(unsigned int seconds) > { > diff --git a/kernel/timer.c b/kernel/timer.c > index ee305c8..c2253dd 100644 > --- a/kernel/timer.c > +++ b/kernel/timer.c > @@ -1750,3 +1750,25 @@ unsigned long msleep_interruptible(unsigned int msecs) > } > > EXPORT_SYMBOL(msleep_interruptible); > + > +static int __sched do_usleep_range(unsigned long min, unsigned long max) > +{ > + ktime_t kmin; > + unsigned long delta; > + > + kmin = ktime_set(0, min * NSEC_PER_USEC); > + delta = (max - min) * NSEC_PER_USEC; > + return schedule_hrtimeout_range(&kmin, delta, HRTIMER_MODE_REL); > +} > + > +/** > + * usleep_range - Drop in replacement for udelay where wakeup is flexible > + * @min: Minimum time in usecs to sleep > + * @max: Maximum time in usecs to sleep > + */ > +void usleep_range(unsigned long min, unsigned long max) > +{ > + __set_current_state(TASK_UNINTERRUPTIBLE); > + do_usleep_range(min, max); > +} > +EXPORT_SYMBOL(usleep_range); This is different from the patch I merged and I'm not seeing any explanation for the change. The implementation of usleep() looks odd. The longer we sleep, the greater the possible inaccuracy. A code comment which explains the thinking and which warns people about the implications is needed. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Patrick Pannuto on 28 Jul 2010 16:50 > This is different from the patch I merged and I'm not seeing any > explanation for the change. > > The implementation of usleep() looks odd. The longer we sleep, the > greater the possible inaccuracy. A code comment which explains the > thinking and which warns people about the implications is needed. Yes it is different; the explanation was in the cover message. I should probably include a copy of the explanation in the commit message as well? It was becoming a very long commit message... // FROM COVER MESSAGE: This iteration is similar, with the notable difference that now usleep has a "built-in slack" of 200%. This is analogous to msleep, which has a built-in slack of 0.4% (since it relies on legacy timers, which have a built-in slack of 0.4%). 200% slack is significantly greater than 0.4%, but the scale of usleep is also significantly different than that of msleep, and I believe 200% to be a sane default. It is my opinion that this interface will most often mirror what developers actually intend - indeed some people who have begun trying to use the API raised this point -, however, I would like some input as it is possibly confusing that the API will "double your sleep" by default. The usleep_range API is still included, since it provides an interface to override the "default slack" of 200% by providing an explicit range, or to allow callers to specify an even larger slack if possible. The problem that was raised by a few people trying to use usleep here was that the API as written was very awkward -- there was never really a good reason to use "usleep" as it was written. The intention was to make usleep a usable / sensible API; the obvious alternative I see is to drop the usleep function entirely and only provide usleep_range - which would probably fit well in your request for callers to think about what they are doing, if providing a somewhat awkward API. The complaint was something to the effect of: "Well, I understand that I should probably give a range, but I have no idea what a good range would be. I really just want it to sleep for a little bit, but I probably shouldn't trigger an extra interrupt. Given the limitations, what's the point of even having a usleep call at all?" Thoughts? -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 28 Jul 2010 17:00
On Wed, 28 Jul 2010 13:47:46 -0700 Patrick Pannuto <ppannuto(a)codeaurora.org> wrote: > > This is different from the patch I merged and I'm not seeing any > > explanation for the change. > > > > The implementation of usleep() looks odd. The longer we sleep, the > > greater the possible inaccuracy. A code comment which explains the > > thinking and which warns people about the implications is needed. I wanna code comment! > Yes it is different; the explanation was in the cover message. I should > probably include a copy of the explanation in the commit message as > well? It was becoming a very long commit message... > > // FROM COVER MESSAGE: > This iteration is similar, with the notable difference that now > usleep has a "built-in slack" of 200%. This is analogous to msleep, > which has a built-in slack of 0.4% (since it relies on legacy timers, > which have a built-in slack of 0.4%). 200% slack is significantly > greater than 0.4%, but the scale of usleep is also significantly > different than that of msleep, and I believe 200% to be a sane > default. > > It is my opinion that this interface will most often mirror what > developers actually intend - indeed some people who have begun > trying to use the API raised this point -, however, I would like > some input as it is possibly confusing that the API will "double > your sleep" by default. > > The usleep_range API is still included, since it provides an > interface to override the "default slack" of 200% by providing > an explicit range, or to allow callers to specify an even larger > slack if possible. > > The problem that was raised by a few people trying to use usleep here > was that the API as written was very awkward -- there was never really > a good reason to use "usleep" as it was written. The intention was > to make usleep a usable / sensible API; the obvious alternative I see > is to drop the usleep function entirely and only provide usleep_range - > which would probably fit well in your request for callers to think > about what they are doing, if providing a somewhat awkward API. > > The complaint was something to the effect of: > > "Well, I understand that I should probably give a range, but I have > no idea what a good range would be. I really just want it to sleep > for a little bit, but I probably shouldn't trigger an extra interrupt. > Given the limitations, what's the point of even having a usleep call > at all?" > > > Thoughts? My main concern is that someone will type usleep(50) and won't realise that it goes and sleeps for 100 usecs and their code gets slow as a result. This sort of thing takes *years* to discover and fix. If we'd forced them to type usleep_range() instead, it would never have happened. Another question: what is the typical overhead of a usleep()? IOW, at what delay value does it make more sense to use udelay()? Another way of asking that would be "how long does a usleep(1) take"? If it reliably consumes 2us CPU time then we shouldn't do it. But it's not just CPU time, is it? A smart udelay() should put the CPU into a lower power state, so a udelay(3) might consume less energy than a usleep(2), because the usleep() does much more work in schedule() and friends? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |