[PATCH 0/2] Add test to validate udelay

[PATCH 0/2] Add test to validate udelay

[David Riley]

This change adds a module and a script that makes use of it to
validate that udelay delays for at least as long as requested
(as compared to ktime).

David Riley (2):
  kernel: time: Add udelay_test module to validate udelay
  tools: add script to test udelay

 kernel/time/Kconfig       |    7 ++
 kernel/time/Makefile      |    1 +
 kernel/time/udelay_test.c |  166 +++++++++++++++++++++++++++++++++++++++++++++
 tools/time/udelay_test.sh |   66 ++++++++++++++++++
 4 files changed, 240 insertions(+)
 create mode 100644 kernel/time/udelay_test.c
 create mode 100644 tools/time/udelay_test.sh

-- 
1.7.9.5

[PATCH 1/2] kernel: time: Add udelay_test module to validate udelay

[David Riley]

Create a module that allows udelay() to be executed to ensure that
it is delaying at least as long as requested.

Signed-off-by: David Riley <davidriley@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/194223
Reviewed-by: Doug Anderson <dianders@chromium.org>
---
 kernel/time/Kconfig       |    7 ++
 kernel/time/Makefile      |    1 +
 kernel/time/udelay_test.c |  166 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+)
 create mode 100644 kernel/time/udelay_test.c

diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index f448513..c6af048 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -202,3 +202,10 @@ config HIGH_RES_TIMERS
 
 endmenu
 endif
+
+config UDELAY_TEST
+	tristate "udelay test driver"
+	default n
+	help
+	  This builds the "udelay_test" module that helps to make sure
+	  that udelay() is working properly.
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index 57a413f..dca9175 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_TICK_ONESHOT)			+= tick-oneshot.o
 obj-$(CONFIG_TICK_ONESHOT)			+= tick-sched.o
 obj-$(CONFIG_TIMER_STATS)			+= timer_stats.o
 obj-$(CONFIG_DEBUG_FS)				+= timekeeping_debug.o
+obj-$(CONFIG_UDELAY_TEST)			+= udelay_test.o
diff --git a/kernel/time/udelay_test.c b/kernel/time/udelay_test.c
new file mode 100644
index 0000000..c0eccef
--- /dev/null
+++ b/kernel/time/udelay_test.c
@@ -0,0 +1,166 @@
+/*
+ * udelay() test kernel module
+ *
+ * Test is executed by writing and reading to /sys/kernel/debug/udelay_test
+ * Tests are configured by writing: USECS ITERATIONS
+ * Tests are executed by reading from the same file.
+ * Specifying usecs of 0 or negative values will run multiples tests.
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/ktime.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+
+#define DEFAULT_ITERATIONS 100
+
+#define DEBUGFS_FILENAME "udelay_test"
+
+static DEFINE_MUTEX(udelay_test_lock);
+static struct dentry *udelay_test_debugfs_file;
+static int udelay_test_usecs;
+static int udelay_test_iterations = DEFAULT_ITERATIONS;
+
+static int udelay_test_single(struct seq_file *s, int usecs, int iters)
+{
+	int min = 0, max = 0, fail_count = 0;
+	long long sum = 0;
+	long long avg;
+	int i;
+
+	if (iters <= 0)
+		return 0;
+
+	for (i = 0; i < iters; ++i) {
+		struct timespec ts1, ts2;
+		int time_passed;
+
+		ktime_get_ts(&ts1);
+		udelay(usecs);
+		ktime_get_ts(&ts2);
+		time_passed = timespec_to_ns(&ts2) - timespec_to_ns(&ts1);
+
+		if (i == 0 || time_passed < min)
+			min = time_passed;
+		if (i == 0 || time_passed > max)
+			max = time_passed;
+		if (time_passed / 1000 < usecs)
+			++fail_count;
+		sum += time_passed;
+	}
+
+	avg = sum;
+	do_div(avg, iters);
+	seq_printf(s, "%d usecs x %d: exp=%d min=%d avg=%lld max=%d", usecs,
+			iters, usecs * 1000, min, avg, max);
+	if (fail_count)
+		seq_printf(s, " FAIL=%d", fail_count);
+	seq_puts(s, "\n");
+
+	return 0;
+}
+
+static int udelay_test_show(struct seq_file *s, void *v)
+{
+	int usecs;
+	int iters;
+	int ret = 0;
+
+	mutex_lock(&udelay_test_lock);
+	usecs = udelay_test_usecs;
+	iters = udelay_test_iterations;
+	mutex_unlock(&udelay_test_lock);
+
+	if (usecs > 0) {
+		return udelay_test_single(s, usecs, iters);
+	} else if (usecs == 0) {
+		struct timespec ts;
+		ktime_get_ts(&ts);
+		seq_printf(s, "udelay() test (lpj=%ld kt=%ld.%09ld)\n",
+				loops_per_jiffy, ts.tv_sec, ts.tv_nsec);
+		seq_puts(s, "usage:\n");
+		seq_puts(s, "echo USECS [ITERS] > " DEBUGFS_FILENAME "\n");
+		seq_puts(s, "cat " DEBUGFS_FILENAME "\n");
+	}
+
+	return ret;
+}
+
+static int udelay_test_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, udelay_test_show, inode->i_private);
+}
+
+static ssize_t udelay_test_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *pos)
+{
+	char lbuf[32];
+	int ret;
+	int usecs;
+	int iters;
+
+	if (count >= sizeof(lbuf))
+		return -EINVAL;
+
+	if (copy_from_user(lbuf, buf, count))
+		return -EFAULT;
+	lbuf[count] = '\0';
+
+	ret = sscanf(lbuf, "%d %d", &usecs, &iters);
+	if (ret < 1)
+		return -EINVAL;
+	else if (ret < 2)
+		iters = DEFAULT_ITERATIONS;
+
+	mutex_lock(&udelay_test_lock);
+	udelay_test_usecs = usecs;
+	udelay_test_iterations = iters;
+	mutex_unlock(&udelay_test_lock);
+
+	return count;
+}
+
+static const struct file_operations udelay_test_debugfs_ops = {
+	.owner = THIS_MODULE,
+	.open = udelay_test_open,
+	.read = seq_read,
+	.write = udelay_test_write,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static int __init udelay_test_init(void)
+{
+	mutex_lock(&udelay_test_lock);
+	udelay_test_debugfs_file = debugfs_create_file(DEBUGFS_FILENAME,
+			S_IRUSR, NULL, NULL, &udelay_test_debugfs_ops);
+	mutex_unlock(&udelay_test_lock);
+
+	return 0;
+}
+
+module_init(udelay_test_init);
+
+static void __exit udelay_test_exit(void)
+{
+	mutex_lock(&udelay_test_lock);
+	debugfs_remove(udelay_test_debugfs_file);
+	mutex_unlock(&udelay_test_lock);
+}
+
+module_exit(udelay_test_exit);
+
+MODULE_AUTHOR("David Riley <davidriley@chromium.org>");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH 2/2] tools: add script to test udelay

[David Riley]

This script makes use of the udelay_test module to exercise udelay()
and ensure that it is delaying long enough (as compared to ktime).

Signed-off-by: David Riley <davidriley@chromium.org>
---
 tools/time/udelay_test.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 tools/time/udelay_test.sh

diff --git a/tools/time/udelay_test.sh b/tools/time/udelay_test.sh
new file mode 100644
index 0000000..12d46b9
--- /dev/null
+++ b/tools/time/udelay_test.sh
@@ -0,0 +1,66 @@
+#!/bin/bash
+
+# udelay() test script
+#
+# Test is executed by writing and reading to /sys/kernel/debug/udelay_test
+# and exercises a variety of delays to ensure that udelay() is delaying
+# at least as long as requested (as compared to ktime).
+#
+# Copyright (C) 2014 Google, Inc.
+#
+# This software is licensed under the terms of the GNU General Public
+# License version 2, as published by the Free Software Foundation, and
+# may be copied, distributed, and modified under those terms.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+MODULE_NAME=udelay_test
+UDELAY_PATH=/sys/kernel/debug/udelay_test
+
+setup()
+{
+	/sbin/modprobe -q $MODULE_NAME
+	tmp_file=`mktemp`
+}
+
+test_one()
+{
+	delay=$1
+	echo $delay > $UDELAY_PATH
+	tee -a $tmp_file < $UDELAY_PATH
+}
+
+cleanup()
+{
+	if [ -f $tmp_file ]; then
+		rm $tmp_file
+	fi
+	/sbin/modprobe -q -r $MODULE_NAME
+}
+
+trap cleanup EXIT
+setup
+
+# Delay for a variety of times.
+# 1..200, 200..500 (by 10), 500..2000 (by 100)
+for (( delay = 1; delay < 200; delay += 1 )); do
+	test_one $delay
+done
+for (( delay = 200; delay < 500; delay += 10 )); do
+	test_one $delay
+done
+for (( delay = 500; delay <= 2000; delay += 100 )); do
+	test_one $delay
+done
+
+# Search for failures
+count=`grep -c FAIL $tmp_file`
+if [ $? -eq "0" ]; then
+	echo "ERROR: $count delays failed to delay long enough"
+	retcode=1
+fi
+
+exit $retcode
-- 
1.7.9.5

Re: [PATCH 2/2] tools: add script to test udelay

[Doug Anderson]

David,

On Tue, May 6, 2014 at 5:12 PM, David Riley <davidriley@chromium.org> wrote:
> This script makes use of the udelay_test module to exercise udelay()
> and ensure that it is delaying long enough (as compared to ktime).
>
> Signed-off-by: David Riley <davidriley@chromium.org>
> ---
>  tools/time/udelay_test.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 tools/time/udelay_test.sh

Potentially this could be created with execute permissions?

Other than that, this looks good to me.

Reviewed-by: Doug Anderson <dianders@chromium.org>

Re: [PATCH 0/2] Add test to validate udelay

[John Stultz]

On 05/06/2014 05:12 PM, David Riley wrote:
> This change adds a module and a script that makes use of it to
> validate that udelay delays for at least as long as requested
> (as compared to ktime).

Interesting.

So fundamentally, udelay is a good bit fuzzier accuracy wise then
ktime_get(), as it may be backed by relatively coarsely calibrated delay
loops, or very rough tsc freq estimates.

ktime_get on the other hand is as fine grained as we can be, and is ntp
corrected, so that a second can really be a second.

So your comparing the fast and loose interface so we can delay a bit
before hitting some hardware again with a fairly precise interface.
Thus  I'd not be surprised if your test failed on various hardware. I'd
really only trust udelay to be roughly accurate, so you might want to
consider adding some degree of acceptable error to the test.

Really, I'm curious about the backstory that made you generate the test?
I assume something bit you where udelay was way off? Or were you using
udelay for some sort of accuracy sensitive use?

thanks
-john

Re: [PATCH 0/2] Add test to validate udelay

[Doug Anderson]

John,

On Tue, May 6, 2014 at 5:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> On 05/06/2014 05:12 PM, David Riley wrote:
>> This change adds a module and a script that makes use of it to
>> validate that udelay delays for at least as long as requested
>> (as compared to ktime).
>
> Interesting.
>
> So fundamentally, udelay is a good bit fuzzier accuracy wise then
> ktime_get(), as it may be backed by relatively coarsely calibrated delay
> loops, or very rough tsc freq estimates.
>
> ktime_get on the other hand is as fine grained as we can be, and is ntp
> corrected, so that a second can really be a second.
>
> So your comparing the fast and loose interface so we can delay a bit
> before hitting some hardware again with a fairly precise interface.
> Thus  I'd not be surprised if your test failed on various hardware. I'd
> really only trust udelay to be roughly accurate, so you might want to
> consider adding some degree of acceptable error to the test.

My understanding is that udelay should be >= the true delay.
Specifically it tends to be used when talking to hardware.  We used it
to ensure a minimum delay between SPI transactions when talking to a
slow embedded controller.  I think the regulator code uses udelay() to
wait for voltage to ramp up, for instance.  Waiting too long isn't
terrible, but too short is bad.

That being said, I think if udelay was within 1% we're probably OK.  I
believe I have seen systems where udelay is marginally shorter than it
ought to be and it didn't upset me too much.


> Really, I'm curious about the backstory that made you generate the test?
> I assume something bit you where udelay was way off? Or were you using
> udelay for some sort of accuracy sensitive use?

Several times we've seen cases where udelay() was pretty broken with
cpufreq if you were actually implementing udelay() with
loops_per_jiffy.  I believe it may also be broken upstream on
multicore systems, though now that ARM arch timers are there maybe we
don't care as much?

Specifically, there is a lot of confusion between the global loops per
jiffy and the per CPU one.  On ARM I think we always use the global
one and we attempt to scale it as cpufreq changes.  ...but...

* cores tend scale together and there's a single global.  That means
you might have started the delay loop at one freq and ended it at
another (if another CPU changes the freq).

* I believe there's some strange issues in terms of how the loops per
jiffy variable is initialized and how the "original CPU freq" is.  I
know we ran into issues on big.LITTLE where the LITTLE cores came up
and clobbered the loops_per_jiffy variable but it was still doing math
based on the big cores.


-Doug

Re: [PATCH 0/2] Add test to validate udelay

[David Riley]

On Tue, May 6, 2014 at 9:19 PM, Doug Anderson <dianders@chromium.org> wrote:
> John,
>
> On Tue, May 6, 2014 at 5:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/06/2014 05:12 PM, David Riley wrote:
>>> This change adds a module and a script that makes use of it to
>>> validate that udelay delays for at least as long as requested
>>> (as compared to ktime).
>>
>> Interesting.
>>
>> So fundamentally, udelay is a good bit fuzzier accuracy wise then
>> ktime_get(), as it may be backed by relatively coarsely calibrated delay
>> loops, or very rough tsc freq estimates.
>>
>> ktime_get on the other hand is as fine grained as we can be, and is ntp
>> corrected, so that a second can really be a second.
>>
>> So your comparing the fast and loose interface so we can delay a bit
>> before hitting some hardware again with a fairly precise interface.
>> Thus  I'd not be surprised if your test failed on various hardware. I'd
>> really only trust udelay to be roughly accurate, so you might want to
>> consider adding some degree of acceptable error to the test.
>
> My understanding is that udelay should be >= the true delay.
> Specifically it tends to be used when talking to hardware.  We used it
> to ensure a minimum delay between SPI transactions when talking to a
> slow embedded controller.  I think the regulator code uses udelay() to
> wait for voltage to ramp up, for instance.  Waiting too long isn't
> terrible, but too short is bad.
>
> That being said, I think if udelay was within 1% we're probably OK.  I
> believe I have seen systems where udelay is marginally shorter than it
> ought to be and it didn't upset me too much.

This message from Thomas Gleixner seems to back up udelay guaranteeing
a minimum delay (as compared to ktime_get) :
http://lkml.iu.edu//hypermail/linux/kernel/1203.1/01034.html  So it
feels right that that anything shorter should be considered a failure.
 If the system still works, that's fine, but udelay() isn't meeting
it's guarantees.

>
>
>> Really, I'm curious about the backstory that made you generate the test?
>> I assume something bit you where udelay was way off? Or were you using
>> udelay for some sort of accuracy sensitive use?
>
> Several times we've seen cases where udelay() was pretty broken with
> cpufreq if you were actually implementing udelay() with
> loops_per_jiffy.  I believe it may also be broken upstream on
> multicore systems, though now that ARM arch timers are there maybe we
> don't care as much?
>
> Specifically, there is a lot of confusion between the global loops per
> jiffy and the per CPU one.  On ARM I think we always use the global
> one and we attempt to scale it as cpufreq changes.  ...but...
>
> * cores tend scale together and there's a single global.  That means
> you might have started the delay loop at one freq and ended it at
> another (if another CPU changes the freq).
>
> * I believe there's some strange issues in terms of how the loops per
> jiffy variable is initialized and how the "original CPU freq" is.  I
> know we ran into issues on big.LITTLE where the LITTLE cores came up
> and clobbered the loops_per_jiffy variable but it was still doing math
> based on the big cores.
>
>
> -Doug

Re: [PATCH 0/2] Add test to validate udelay

[John Stultz]

On 05/06/2014 09:19 PM, Doug Anderson wrote:
> John,
>
> On Tue, May 6, 2014 at 5:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/06/2014 05:12 PM, David Riley wrote:
>>> This change adds a module and a script that makes use of it to
>>> validate that udelay delays for at least as long as requested
>>> (as compared to ktime).
>> Interesting.
>>
>> So fundamentally, udelay is a good bit fuzzier accuracy wise then
>> ktime_get(), as it may be backed by relatively coarsely calibrated delay
>> loops, or very rough tsc freq estimates.
>>
>> ktime_get on the other hand is as fine grained as we can be, and is ntp
>> corrected, so that a second can really be a second.
>>
>> So your comparing the fast and loose interface so we can delay a bit
>> before hitting some hardware again with a fairly precise interface.
>> Thus  I'd not be surprised if your test failed on various hardware. I'd
>> really only trust udelay to be roughly accurate, so you might want to
>> consider adding some degree of acceptable error to the test.
> My understanding is that udelay should be >= the true delay.
> Specifically it tends to be used when talking to hardware.  We used it
> to ensure a minimum delay between SPI transactions when talking to a
> slow embedded controller.  I think the regulator code uses udelay() to
> wait for voltage to ramp up, for instance.  Waiting too long isn't
> terrible, but too short is bad.
>
> That being said, I think if udelay was within 1% we're probably OK.  I
> believe I have seen systems where udelay is marginally shorter than it
> ought to be and it didn't upset me too much.

Yea,  udelay() should clearly delay for greater then or equal to the
time requested. The worry I have is that we're measuring that time with
two different clocks, so the larger the delay the more likely the time
error between those two clocks will be apparent (particularly with one
being very coarsely calibrated and the other being ntp corrected).

But these sorts of differences should be well within 0.1%.  So for most
reasonable delays this should be mostly hidden by the measuring overhead
and not likely a problem with your test. I just wanted to be sure we
weren't in a situation where folks were expecting udelay to be ntp
corrected ;)



>
>> Really, I'm curious about the backstory that made you generate the test?
>> I assume something bit you where udelay was way off? Or were you using
>> udelay for some sort of accuracy sensitive use?
> Several times we've seen cases where udelay() was pretty broken with
> cpufreq if you were actually implementing udelay() with
> loops_per_jiffy.  I believe it may also be broken upstream on
> multicore systems, though now that ARM arch timers are there maybe we
> don't care as much?
>
> Specifically, there is a lot of confusion between the global loops per
> jiffy and the per CPU one.  On ARM I think we always use the global
> one and we attempt to scale it as cpufreq changes.  ...but...
>
> * cores tend scale together and there's a single global.  That means
> you might have started the delay loop at one freq and ended it at
> another (if another CPU changes the freq).

Good point. The loops based delay would clearly be broken w/ ASMP unless
we use per-cpu values that are scaled(and as you point out, we don't
scale the value mid-delay). Time based counters for udelay() - like the
arch timer you mention - are a much better way to work around this.


> * I believe there's some strange issues in terms of how the loops per
> jiffy variable is initialized and how the "original CPU freq" is.  I
> know we ran into issues on big.LITTLE where the LITTLE cores came up
> and clobbered the loops_per_jiffy variable but it was still doing math
> based on the big cores.

Hrm. I don't have a theory on this right now, but clearly there are
issues to be resolved, so having your tests included would be a good
thing to help find these issues.

So no objections from me. Thanks for the extra context!

thanks
-john

Re: [PATCH 0/2] Add test to validate udelay

[Doug Anderson]

John,

On Wed, May 7, 2014 at 11:10 AM, John Stultz <john.stultz@linaro.org> wrote:
> On 05/06/2014 09:19 PM, Doug Anderson wrote:
>> John,
>>
>> On Tue, May 6, 2014 at 5:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>> Really, I'm curious about the backstory that made you generate the test?
>>> I assume something bit you where udelay was way off? Or were you using
>>> udelay for some sort of accuracy sensitive use?
>> Several times we've seen cases where udelay() was pretty broken with
>> cpufreq if you were actually implementing udelay() with
>> loops_per_jiffy.  I believe it may also be broken upstream on
>> multicore systems, though now that ARM arch timers are there maybe we
>> don't care as much?
>>
>> Specifically, there is a lot of confusion between the global loops per
>> jiffy and the per CPU one.  On ARM I think we always use the global
>> one and we attempt to scale it as cpufreq changes.  ...but...
>>
>> * cores tend scale together and there's a single global.  That means
>> you might have started the delay loop at one freq and ended it at
>> another (if another CPU changes the freq).
>
> Good point. The loops based delay would clearly be broken w/ ASMP unless
> we use per-cpu values that are scaled(and as you point out, we don't
> scale the value mid-delay). Time based counters for udelay() - like the
> arch timer you mention - are a much better way to work around this.

Locally we have this:
* https://chromium-review.googlesource.com/189885
  ARM: Don't ever downscale loops_per_jiffy in SMP systems

I didn't think upstream would really want this given the move to arch
timers, but I'm happy to post it.


>> * I believe there's some strange issues in terms of how the loops per
>> jiffy variable is initialized and how the "original CPU freq" is.  I
>> know we ran into issues on big.LITTLE where the LITTLE cores came up
>> and clobbered the loops_per_jiffy variable but it was still doing math
>> based on the big cores.
>
> Hrm. I don't have a theory on this right now, but clearly there are
> issues to be resolved, so having your tests included would be a good
> thing to help find these issues.

Locally we added:
* https://chromium-review.googlesource.com/189725
  init: Don't decrease loops_per_jiffy when a CPU comes up

...and that "fixed" things for us.  Specifically what happened was:

- A15s boot up at 1.8GHz (though they can actually go up to 1.9/2.0).

- A7s boot up at ~600MHz and clobbered loops_per_jiffy with something tiny.

- In the first "cpufreq_callback" in "arch/arm/kernel/smp.c", we
stored the current _A15_ frequency upon the first cpufreq transition
and the current _A7_ loops per jiffy (since the A7s clobbered it).


It seemed like our situation was so messed up that upstream probably
wouldn't want the fix (using loops_per_jiffy in an HMP system is
pretty insane), but again I'm happy to send it up.


We also took out some IKS (big.LITTLE) attempt to fix up
loops_per_jiffy that was massively flawed in a number of ways
<https://chromium-review.googlesource.com/189728> but that's not
upstream.


-Doug

Re: [PATCH 0/2] Add test to validate udelay

[John Stultz]

On 05/07/2014 11:32 AM, Doug Anderson wrote:
> John,
>
> On Wed, May 7, 2014 at 11:10 AM, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/06/2014 09:19 PM, Doug Anderson wrote:
>>> John,
>>>
>>> On Tue, May 6, 2014 at 5:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> Really, I'm curious about the backstory that made you generate the test?
>>>> I assume something bit you where udelay was way off? Or were you using
>>>> udelay for some sort of accuracy sensitive use?
>>> Several times we've seen cases where udelay() was pretty broken with
>>> cpufreq if you were actually implementing udelay() with
>>> loops_per_jiffy.  I believe it may also be broken upstream on
>>> multicore systems, though now that ARM arch timers are there maybe we
>>> don't care as much?
>>>
>>> Specifically, there is a lot of confusion between the global loops per
>>> jiffy and the per CPU one.  On ARM I think we always use the global
>>> one and we attempt to scale it as cpufreq changes.  ...but...
>>>
>>> * cores tend scale together and there's a single global.  That means
>>> you might have started the delay loop at one freq and ended it at
>>> another (if another CPU changes the freq).
>> Good point. The loops based delay would clearly be broken w/ ASMP unless
>> we use per-cpu values that are scaled(and as you point out, we don't
>> scale the value mid-delay). Time based counters for udelay() - like the
>> arch timer you mention - are a much better way to work around this.
> Locally we have this:
> * https://chromium-review.googlesource.com/189885
>   ARM: Don't ever downscale loops_per_jiffy in SMP systems
>
> I didn't think upstream would really want this given the move to arch
> timers, but I'm happy to post it.

Might be good just to get the discussion going. I agree its probably not
the best solution, and likely the timer delay is the right path, but but
I think we need to have some rails in place so that other folks don't
trip over this.

Maybe a combination of your change and something where on systems that
see cpufreq changes (or cores with different frequencies) complain
loudly if they're not configured to use the delay timer?


>>> * I believe there's some strange issues in terms of how the loops per
>>> jiffy variable is initialized and how the "original CPU freq" is.  I
>>> know we ran into issues on big.LITTLE where the LITTLE cores came up
>>> and clobbered the loops_per_jiffy variable but it was still doing math
>>> based on the big cores.
>> Hrm. I don't have a theory on this right now, but clearly there are
>> issues to be resolved, so having your tests included would be a good
>> thing to help find these issues.
> Locally we added:
> * https://chromium-review.googlesource.com/189725
>   init: Don't decrease loops_per_jiffy when a CPU comes up
>
> ...and that "fixed" things for us.  Specifically what happened was:
>
> - A15s boot up at 1.8GHz (though they can actually go up to 1.9/2.0).
>
> - A7s boot up at ~600MHz and clobbered loops_per_jiffy with something tiny.
>
> - In the first "cpufreq_callback" in "arch/arm/kernel/smp.c", we
> stored the current _A15_ frequency upon the first cpufreq transition
> and the current _A7_ loops per jiffy (since the A7s clobbered it).
>
>
> It seemed like our situation was so messed up that upstream probably
> wouldn't want the fix (using loops_per_jiffy in an HMP system is
> pretty insane), but again I'm happy to send it up.

Yea. Again, might be good to send it out just so more folks are aware
(particularly outside the arm world) that ASMP systems are here and the
generic delay infrastructure has assumptions that are not compatible.

thanks
-john

Re: [PATCH 0/2] Add test to validate udelay

[Doug Anderson]

John,

On Wed, May 7, 2014 at 3:46 PM, John Stultz <john.stultz@linaro.org> wrote:
> On 05/07/2014 11:32 AM, Doug Anderson wrote:
>> John,
>>
>> On Wed, May 7, 2014 at 11:10 AM, John Stultz <john.stultz@linaro.org> wrote:
>>> On 05/06/2014 09:19 PM, Doug Anderson wrote:
>>>> John,
>>>>
>>>> On Tue, May 6, 2014 at 5:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>> Really, I'm curious about the backstory that made you generate the test?
>>>>> I assume something bit you where udelay was way off? Or were you using
>>>>> udelay for some sort of accuracy sensitive use?
>>>> Several times we've seen cases where udelay() was pretty broken with
>>>> cpufreq if you were actually implementing udelay() with
>>>> loops_per_jiffy.  I believe it may also be broken upstream on
>>>> multicore systems, though now that ARM arch timers are there maybe we
>>>> don't care as much?
>>>>
>>>> Specifically, there is a lot of confusion between the global loops per
>>>> jiffy and the per CPU one.  On ARM I think we always use the global
>>>> one and we attempt to scale it as cpufreq changes.  ...but...
>>>>
>>>> * cores tend scale together and there's a single global.  That means
>>>> you might have started the delay loop at one freq and ended it at
>>>> another (if another CPU changes the freq).
>>> Good point. The loops based delay would clearly be broken w/ ASMP unless
>>> we use per-cpu values that are scaled(and as you point out, we don't
>>> scale the value mid-delay). Time based counters for udelay() - like the
>>> arch timer you mention - are a much better way to work around this.
>> Locally we have this:
>> * https://chromium-review.googlesource.com/189885
>>   ARM: Don't ever downscale loops_per_jiffy in SMP systems
>>
>> I didn't think upstream would really want this given the move to arch
>> timers, but I'm happy to post it.
>
> Might be good just to get the discussion going. I agree its probably not
> the best solution, and likely the timer delay is the right path, but but
> I think we need to have some rails in place so that other folks don't
> trip over this.
>
> Maybe a combination of your change and something where on systems that
> see cpufreq changes (or cores with different frequencies) complain
> loudly if they're not configured to use the delay timer?

I thew the patch up there.  I didn't add any loud complaining, but I
certainly can if people think that the patches are useful and they'd
like the complaining.

https://patchwork.kernel.org/patch/4132521/


>>>> * I believe there's some strange issues in terms of how the loops per
>>>> jiffy variable is initialized and how the "original CPU freq" is.  I
>>>> know we ran into issues on big.LITTLE where the LITTLE cores came up
>>>> and clobbered the loops_per_jiffy variable but it was still doing math
>>>> based on the big cores.
>>> Hrm. I don't have a theory on this right now, but clearly there are
>>> issues to be resolved, so having your tests included would be a good
>>> thing to help find these issues.
>> Locally we added:
>> * https://chromium-review.googlesource.com/189725
>>   init: Don't decrease loops_per_jiffy when a CPU comes up
>>
>> ...and that "fixed" things for us.  Specifically what happened was:
>>
>> - A15s boot up at 1.8GHz (though they can actually go up to 1.9/2.0).
>>
>> - A7s boot up at ~600MHz and clobbered loops_per_jiffy with something tiny.
>>
>> - In the first "cpufreq_callback" in "arch/arm/kernel/smp.c", we
>> stored the current _A15_ frequency upon the first cpufreq transition
>> and the current _A7_ loops per jiffy (since the A7s clobbered it).
>>
>>
>> It seemed like our situation was so messed up that upstream probably
>> wouldn't want the fix (using loops_per_jiffy in an HMP system is
>> pretty insane), but again I'm happy to send it up.
>
> Yea. Again, might be good to send it out just so more folks are aware
> (particularly outside the arm world) that ASMP systems are here and the
> generic delay infrastructure has assumptions that are not compatible.

Here ya go: <https://patchwork.kernel.org/patch/4132651/>

-Doug

[PATCH v2 0/2] Add test to validate udelay

[David Riley]

This change adds a module and a script that makes use of it to
validate that udelay delays for at least as long as requested
(as compared to ktime).

Changes since v1:
- allow udelay() to be 0.5% faster than requested as per feedback

David Riley (2):
  kernel: time: Add udelay_test module to validate udelay
  tools: add script to test udelay

 kernel/time/Kconfig       |    7 ++
 kernel/time/Makefile      |    1 +
 kernel/time/udelay_test.c |  169 +++++++++++++++++++++++++++++++++++++++++++++
 tools/time/udelay_test.sh |   66 ++++++++++++++++++
 4 files changed, 243 insertions(+)
 create mode 100644 kernel/time/udelay_test.c
 create mode 100644 tools/time/udelay_test.sh

-- 
1.7.9.5

[PATCH v2 1/2] kernel: time: Add udelay_test module to validate udelay

[David Riley]

Create a module that allows udelay() to be executed to ensure that
it is delaying at least as long as requested (with a little bit of
error allowed).

Signed-off-by: David Riley <davidriley@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
---
 kernel/time/Kconfig       |    7 ++
 kernel/time/Makefile      |    1 +
 kernel/time/udelay_test.c |  169 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 kernel/time/udelay_test.c

diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index f448513..c6af048 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -202,3 +202,10 @@ config HIGH_RES_TIMERS
 
 endmenu
 endif
+
+config UDELAY_TEST
+	tristate "udelay test driver"
+	default n
+	help
+	  This builds the "udelay_test" module that helps to make sure
+	  that udelay() is working properly.
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index 57a413f..dca9175 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_TICK_ONESHOT)			+= tick-oneshot.o
 obj-$(CONFIG_TICK_ONESHOT)			+= tick-sched.o
 obj-$(CONFIG_TIMER_STATS)			+= timer_stats.o
 obj-$(CONFIG_DEBUG_FS)				+= timekeeping_debug.o
+obj-$(CONFIG_UDELAY_TEST)			+= udelay_test.o
diff --git a/kernel/time/udelay_test.c b/kernel/time/udelay_test.c
new file mode 100644
index 0000000..52fef5a
--- /dev/null
+++ b/kernel/time/udelay_test.c
@@ -0,0 +1,169 @@
+/*
+ * udelay() test kernel module
+ *
+ * Test is executed by writing and reading to /sys/kernel/debug/udelay_test
+ * Tests are configured by writing: USECS ITERATIONS
+ * Tests are executed by reading from the same file.
+ * Specifying usecs of 0 or negative values will run multiples tests.
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/ktime.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+
+#define DEFAULT_ITERATIONS 100
+
+#define DEBUGFS_FILENAME "udelay_test"
+
+static DEFINE_MUTEX(udelay_test_lock);
+static struct dentry *udelay_test_debugfs_file;
+static int udelay_test_usecs;
+static int udelay_test_iterations = DEFAULT_ITERATIONS;
+
+static int udelay_test_single(struct seq_file *s, int usecs, int iters)
+{
+	int min = 0, max = 0, fail_count = 0;
+	long long sum = 0;
+	long long avg;
+	int i;
+	/* Allow udelay to be up to 0.5% fast */
+	int allowed_error_ns = usecs * 5;
+
+	if (iters <= 0)
+		return 0;
+
+	for (i = 0; i < iters; ++i) {
+		struct timespec ts1, ts2;
+		int time_passed;
+
+		ktime_get_ts(&ts1);
+		udelay(usecs);
+		ktime_get_ts(&ts2);
+		time_passed = timespec_to_ns(&ts2) - timespec_to_ns(&ts1);
+
+		if (i == 0 || time_passed < min)
+			min = time_passed;
+		if (i == 0 || time_passed > max)
+			max = time_passed;
+		if ((time_passed + allowed_error_ns) / 1000 < usecs)
+			++fail_count;
+		sum += time_passed;
+	}
+
+	avg = sum;
+	do_div(avg, iters);
+	seq_printf(s, "%d usecs x %d: exp=%d allowed=%d min=%d avg=%lld max=%d",
+			usecs, iters, usecs * 1000,
+			(usecs * 1000) - allowed_error_ns, min, avg, max);
+	if (fail_count)
+		seq_printf(s, " FAIL=%d", fail_count);
+	seq_puts(s, "\n");
+
+	return 0;
+}
+
+static int udelay_test_show(struct seq_file *s, void *v)
+{
+	int usecs;
+	int iters;
+	int ret = 0;
+
+	mutex_lock(&udelay_test_lock);
+	usecs = udelay_test_usecs;
+	iters = udelay_test_iterations;
+	mutex_unlock(&udelay_test_lock);
+
+	if (usecs > 0) {
+		return udelay_test_single(s, usecs, iters);
+	} else if (usecs == 0) {
+		struct timespec ts;
+		ktime_get_ts(&ts);
+		seq_printf(s, "udelay() test (lpj=%ld kt=%ld.%09ld)\n",
+				loops_per_jiffy, ts.tv_sec, ts.tv_nsec);
+		seq_puts(s, "usage:\n");
+		seq_puts(s, "echo USECS [ITERS] > " DEBUGFS_FILENAME "\n");
+		seq_puts(s, "cat " DEBUGFS_FILENAME "\n");
+	}
+
+	return ret;
+}
+
+static int udelay_test_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, udelay_test_show, inode->i_private);
+}
+
+static ssize_t udelay_test_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *pos)
+{
+	char lbuf[32];
+	int ret;
+	int usecs;
+	int iters;
+
+	if (count >= sizeof(lbuf))
+		return -EINVAL;
+
+	if (copy_from_user(lbuf, buf, count))
+		return -EFAULT;
+	lbuf[count] = '\0';
+
+	ret = sscanf(lbuf, "%d %d", &usecs, &iters);
+	if (ret < 1)
+		return -EINVAL;
+	else if (ret < 2)
+		iters = DEFAULT_ITERATIONS;
+
+	mutex_lock(&udelay_test_lock);
+	udelay_test_usecs = usecs;
+	udelay_test_iterations = iters;
+	mutex_unlock(&udelay_test_lock);
+
+	return count;
+}
+
+static const struct file_operations udelay_test_debugfs_ops = {
+	.owner = THIS_MODULE,
+	.open = udelay_test_open,
+	.read = seq_read,
+	.write = udelay_test_write,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static int __init udelay_test_init(void)
+{
+	mutex_lock(&udelay_test_lock);
+	udelay_test_debugfs_file = debugfs_create_file(DEBUGFS_FILENAME,
+			S_IRUSR, NULL, NULL, &udelay_test_debugfs_ops);
+	mutex_unlock(&udelay_test_lock);
+
+	return 0;
+}
+
+module_init(udelay_test_init);
+
+static void __exit udelay_test_exit(void)
+{
+	mutex_lock(&udelay_test_lock);
+	debugfs_remove(udelay_test_debugfs_file);
+	mutex_unlock(&udelay_test_lock);
+}
+
+module_exit(udelay_test_exit);
+
+MODULE_AUTHOR("David Riley <davidriley@chromium.org>");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH v2 2/2] tools: add script to test udelay

[David Riley]

This script makes use of the udelay_test module to exercise udelay()
and ensure that it is delaying long enough (as compared to ktime).

Signed-off-by: David Riley <davidriley@chromium.org>
---
 tools/time/udelay_test.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 tools/time/udelay_test.sh

diff --git a/tools/time/udelay_test.sh b/tools/time/udelay_test.sh
new file mode 100644
index 0000000..12d46b9
--- /dev/null
+++ b/tools/time/udelay_test.sh
@@ -0,0 +1,66 @@
+#!/bin/bash
+
+# udelay() test script
+#
+# Test is executed by writing and reading to /sys/kernel/debug/udelay_test
+# and exercises a variety of delays to ensure that udelay() is delaying
+# at least as long as requested (as compared to ktime).
+#
+# Copyright (C) 2014 Google, Inc.
+#
+# This software is licensed under the terms of the GNU General Public
+# License version 2, as published by the Free Software Foundation, and
+# may be copied, distributed, and modified under those terms.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+MODULE_NAME=udelay_test
+UDELAY_PATH=/sys/kernel/debug/udelay_test
+
+setup()
+{
+	/sbin/modprobe -q $MODULE_NAME
+	tmp_file=`mktemp`
+}
+
+test_one()
+{
+	delay=$1
+	echo $delay > $UDELAY_PATH
+	tee -a $tmp_file < $UDELAY_PATH
+}
+
+cleanup()
+{
+	if [ -f $tmp_file ]; then
+		rm $tmp_file
+	fi
+	/sbin/modprobe -q -r $MODULE_NAME
+}
+
+trap cleanup EXIT
+setup
+
+# Delay for a variety of times.
+# 1..200, 200..500 (by 10), 500..2000 (by 100)
+for (( delay = 1; delay < 200; delay += 1 )); do
+	test_one $delay
+done
+for (( delay = 200; delay < 500; delay += 10 )); do
+	test_one $delay
+done
+for (( delay = 500; delay <= 2000; delay += 100 )); do
+	test_one $delay
+done
+
+# Search for failures
+count=`grep -c FAIL $tmp_file`
+if [ $? -eq "0" ]; then
+	echo "ERROR: $count delays failed to delay long enough"
+	retcode=1
+fi
+
+exit $retcode
-- 
1.7.9.5

Re: [PATCH v2 0/2] Add test to validate udelay

[David Riley]

Hi,

I was wondering if there were any comments to this patch or if it was
picked up somewhere?

Thanks,
Dave

On Wed, May 14, 2014 at 3:30 PM, David Riley <davidriley@chromium.org> wrote:
> This change adds a module and a script that makes use of it to
> validate that udelay delays for at least as long as requested
> (as compared to ktime).
>
> Changes since v1:
> - allow udelay() to be 0.5% faster than requested as per feedback
>
> David Riley (2):
>   kernel: time: Add udelay_test module to validate udelay
>   tools: add script to test udelay
>
>  kernel/time/Kconfig       |    7 ++
>  kernel/time/Makefile      |    1 +
>  kernel/time/udelay_test.c |  169 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/time/udelay_test.sh |   66 ++++++++++++++++++
>  4 files changed, 243 insertions(+)
>  create mode 100644 kernel/time/udelay_test.c
>  create mode 100644 tools/time/udelay_test.sh
>
> --
> 1.7.9.5
>

Re: [PATCH v2 0/2] Add test to validate udelay

[John Stultz]

On Mon, Jun 9, 2014 at 4:41 PM, David Riley <davidriley@google.com> wrote:
> Hi,
>
> I was wondering if there were any comments to this patch or if it was
> picked up somewhere?

So I guess it got left in a bit of an ambiguous spot.  The basic point
of this test is to verify there is a sane counter time-based delay on
freq shifting systems. You've addressed my only false positive
concern, and so I'm not opposed to including it.

However, there was the following discussion of why this test was
wanted, and that was due to a system that didn't have a counter time
based delay (instead using the loop delay) which was running into
problems with cpufreq changes. That discussion wandered a bit, but the
consensus was "don't do that".

I sort of mixed the messages and associated that feedback with this
patch as well, so my apologies.

Just to be clear, it might be good to more clearly target this test as
a validation to ensure systems don't use those bad configs. So If you
want to resend with that extra context in the commit message, I'll go
ahead and queue it (looking at 3.17)

thanks
-john