mdelay is a busy-wait loop which is wasteful. If at all possible,
callers should use msleep instead of mdelay.
The only time mdelay is really appropriate is in atomic context,
however, delays of 1ms+ in atomic context are rather expensive, so
a warning for this case is probably appropriate as well to encourage
people to move such expensive delays outside of atomic context
Signed-off-by: Israel Schlesinger <[email protected]>
---
scripts/checkpatch.pl | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd88f11..b3f7f71 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2570,6 +2570,11 @@ sub process {
}
}
+# check the patch for use of mdelay
+ if ($line =~ /\bmdelay\(/) {
+ WARN("use of mdelay() found: msleep() is the preferred API.\n" . $line );
+ }
+
# warn about #ifdefs in C files
# if ($line =~ /^.\s*\#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
# print "#ifdef in C files should be avoided\n";
--
1.7.0.2
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Tue, 2010-07-27 at 10:11 -0700, Israel Schlesinger wrote:
> + if ($line =~ /\bmdelay\(/) {
That should be:
if ($line =~ /\bmdelay\s*\(/) {
On Tue, 27 Jul 2010 10:11:11 -0700
Israel Schlesinger <[email protected]> wrote:
> mdelay is a busy-wait loop which is wasteful. If at all possible,
> callers should use msleep instead of mdelay.
>
> The only time mdelay is really appropriate is in atomic context,
> however, delays of 1ms+ in atomic context are rather expensive, so
> a warning for this case is probably appropriate as well to encourage
> people to move such expensive delays outside of atomic context
Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave
a 1ms delay. My patch to fix msleep() at that time didn't get in due
to concerns about the cost of using hrtimers. Perhaps msleep() has
gotten better, but, if not, actually getting a 1ms delay remains a
valid reason for using mdelay() instead IMO. It made a difference of a
few seconds at open time for a driver I was doing at the time.
jon
On 07/27/2010 10:31 AM, Jonathan Corbet wrote:
> On Tue, 27 Jul 2010 10:11:11 -0700
> Israel Schlesinger <[email protected]> wrote:
>
>> mdelay is a busy-wait loop which is wasteful. If at all possible,
>> callers should use msleep instead of mdelay.
>>
>> The only time mdelay is really appropriate is in atomic context,
>> however, delays of 1ms+ in atomic context are rather expensive, so
>> a warning for this case is probably appropriate as well to encourage
>> people to move such expensive delays outside of atomic context
>
> Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave
> a 1ms delay. My patch to fix msleep() at that time didn't get in due
> to concerns about the cost of using hrtimers. Perhaps msleep() has
> gotten better, but, if not, actually getting a 1ms delay remains a
> valid reason for using mdelay() instead IMO. It made a difference of a
> few seconds at open time for a driver I was doing at the time.
>
> jon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Check out the recently added usleep in -tip, and the checkpatch patch
pending in my queue that fixes that case (I'll send in a few hours ;) )
-pat
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Tue, 27 Jul 2010 10:32:54 -0700
Patrick Pannuto <[email protected]> wrote:
> On 07/27/2010 10:31 AM, Jonathan Corbet wrote:
> > On Tue, 27 Jul 2010 10:11:11 -0700
> > Israel Schlesinger <[email protected]> wrote:
> >
> >> mdelay is a busy-wait loop which is wasteful. If at all possible,
> >> callers should use msleep instead of mdelay.
> >>
> >> The only time mdelay is really appropriate is in atomic context,
> >> however, delays of 1ms+ in atomic context are rather expensive, so
> >> a warning for this case is probably appropriate as well to encourage
> >> people to move such expensive delays outside of atomic context
> >
> > Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave
> > a 1ms delay. My patch to fix msleep() at that time didn't get in due
> > to concerns about the cost of using hrtimers. Perhaps msleep() has
> > gotten better, but, if not, actually getting a 1ms delay remains a
> > valid reason for using mdelay() instead IMO. It made a difference of a
> > few seconds at open time for a driver I was doing at the time.
> >
> > jon
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
> Check out the recently added usleep in -tip, and the checkpatch patch
> pending in my queue that fixes that case (I'll send in a few hours ;) )
>
The message should point people at usleep_range(), I'd suggest. It's a
more power-friendly way of sleeping.
That assumes that the below patch gets merged - the people who handle
timer-related things are presently, err, asleep.
From: Patrick Pannuto <[email protected]>
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 <[email protected]>
Acked-by: Arjan van de Ven <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
include/linux/delay.h | 6 ++++++
kernel/timer.c | 22 ++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff -puN include/linux/delay.h~timers-add-usleep-timer include/linux/delay.h
--- a/include/linux/delay.h~timers-add-usleep-timer
+++ a/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 -puN kernel/timer.c~timers-add-usleep-timer kernel/timer.c
--- a/kernel/timer.c~timers-add-usleep-timer
+++ a/kernel/timer.c
@@ -1763,3 +1763,25 @@ unsigned long msleep_interruptible(unsig
}
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);
_
On Tue, Jul 27, 2010 at 12:16:10PM -0700, Andrew Morton wrote:
> On Tue, 27 Jul 2010 10:32:54 -0700
> Patrick Pannuto <[email protected]> wrote:
>
> > On 07/27/2010 10:31 AM, Jonathan Corbet wrote:
> > > On Tue, 27 Jul 2010 10:11:11 -0700
> > > Israel Schlesinger <[email protected]> wrote:
> > >
> > >> mdelay is a busy-wait loop which is wasteful. If at all possible,
> > >> callers should use msleep instead of mdelay.
> > >>
> > >> The only time mdelay is really appropriate is in atomic context,
> > >> however, delays of 1ms+ in atomic context are rather expensive, so
> > >> a warning for this case is probably appropriate as well to encourage
> > >> people to move such expensive delays outside of atomic context
> > >
> > > Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave
> > > a 1ms delay. My patch to fix msleep() at that time didn't get in due
> > > to concerns about the cost of using hrtimers. Perhaps msleep() has
> > > gotten better, but, if not, actually getting a 1ms delay remains a
> > > valid reason for using mdelay() instead IMO. It made a difference of a
> > > few seconds at open time for a driver I was doing at the time.
> > >
> > > jon
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> >
> > Check out the recently added usleep in -tip, and the checkpatch patch
> > pending in my queue that fixes that case (I'll send in a few hours ;) )
> >
>
> The message should point people at usleep_range(), I'd suggest. It's a
> more power-friendly way of sleeping.
>
> That assumes that the below patch gets merged - the people who handle
> timer-related things are presently, err, asleep.
>
>
> From: Patrick Pannuto <[email protected]>
>
> 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 <[email protected]>
> Acked-by: Arjan van de Ven <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> include/linux/delay.h | 6 ++++++
> kernel/timer.c | 22 ++++++++++++++++++++++
> 2 files changed, 28 insertions(+)
>
> diff -puN include/linux/delay.h~timers-add-usleep-timer include/linux/delay.h
> --- a/include/linux/delay.h~timers-add-usleep-timer
> +++ a/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 -puN kernel/timer.c~timers-add-usleep-timer kernel/timer.c
> --- a/kernel/timer.c~timers-add-usleep-timer
> +++ a/kernel/timer.c
> @@ -1763,3 +1763,25 @@ unsigned long msleep_interruptible(unsig
> }
>
> 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;
If this interface is taking a min and max in micro-seconds, then does
not the delta need also to be converted to nano-seconds?
schedule_hrtimeout_range seems to call hrtimer_set_expires_range_ns
which seems to generally be called with 'delta_ns'. Something like:
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);
-apw