checkpatch: Add warnings for use of mdelay()

PATCH Superseded Israel Schlesinger 6 participants 6 messages Jul 27, 2010 – Jul 28, 2010

Subsystems (2)

Checkpatch High-resolution timers, timer wheel, clockevents

lore

Share & tools

lore

Israel Schlesinger

author

2010-07-27 17:11 UTC #0 lore raw

PATCH 1/1 checkpatch: Add warnings for use of mdelay()

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(-)

scripts/checkpatch.pl +5 -0

index bd88f11..b3f7f71 100755
@@ -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";

Signature · 5 lines

1.7.0.2

-- 
Employee of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Joe Perches

maintainer

2010-07-27 17:22 UTC #1 lore raw

scripts/checkpatch.pl sub process { · line 2574

Quoted 1 line · Israel Schlesinger

scripts/checkpatch.pl +1 -0
+		if ($line =~ /\bmdelay\(/) {

That should be:

		if ($line =~ /\bmdelay\s*\(/) {

Jonathan Corbet

2010-07-27 17:31 UTC #2 lore raw

In reply to Israel Schlesinger

Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…

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

Patrick Pannuto

2010-07-27 17:32 UTC #3 lore raw

Quoted 18 lines · Jonathan Corbet

Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…
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.

Quoted 6 lines

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

Signature · 2 lines

Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Andrew Morton

Acked-by Arjan van de Ven

2010-07-27 19:17 UTC #4 lore raw

Quoted 26 lines · On Tue, 27 Jul 2010 10:32:54 -0700 Patrick Pannuto <[email protected]>…

Quoted 18 lines · Jonathan Corbet
Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…
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.
Quoted 6 lines
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/

Quoted 3 lines

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(+)

include/linux/delay.h renamed from include/linux/delay.h~timers-add-usleep-timer +6 -0

diff -puN include/linux/delay.h~timers-add-usleep-timer 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)
 {

kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +22 -0

diff -puN kernel/timer.c~timers-add-usleep-timer 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);

Andy Whitcroft

maintainer

2010-07-28 09:44 UTC #5 lore raw

Quoted 79 lines · Andrew Morton

Quoted 26 lines · On Tue, 27 Jul 2010 10:32:54 -0700 Patrick Pannuto <[email protected]>…
Quoted 18 lines · Jonathan Corbet
Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…
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.
Quoted 6 lines
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/
Quoted 3 lines
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
include/linux/delay.h renamed from include/linux/delay.h~timers-add-usleep-timer +6 -0
@@ -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
kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +0 -0
@@ -1763,3 +1763,25 @@ unsigned long msleep_interruptible(unsig
 }

kernel/timer.c unsigned long msleep_interruptible(unsig · line 1773

Quoted 10 lines

kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +8 -0

 
 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;

Quoted 4 lines

kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +4 -0
+	return schedule_hrtimeout_range(&kmin, delta, HRTIMER_MODE_REL);
+}
+
+/**

kernel/timer.c unsigned long msleep_interruptible(unsig · line 1787

Quoted 10 lines

kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +10 -0

+ * 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