[PATCH 6/7] mmc_spi: convert timeout handling to jiffies and avoid busy waiting

[PATCH 6/7] mmc_spi: convert timeout handling to jiffies and avoid busy waiting

[Wolfgang Mües]

From: Wolfgang Muees <wolfgang.mues@auerswald.de>

o SD/MMC card timeouts can be very high. So avoid busy-waiting,
  using the scheduler. Calculate all timeouts in jiffies units,
  because this will give us the correct sign when to involve
  the scheduler.

Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>

---
This is one of a line of patches to enhance the usability of
the mmc spi host port driver from "don't work with most SD cards"
to "work with nearly all SD cards" (including those ugly cards
with non-byte-aligned responses).

diff -uprN 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/host/mmc_spi.c 2_6_29_rc7_patch6_jiffies_and_scheduling/drivers/mmc/host/mmc_spi.c
--- 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/host/mmc_spi.c	2009-03-11 13:43:39.000000000 +0100
+++ 2_6_29_rc7_patch6_jiffies_and_scheduling/drivers/mmc/host/mmc_spi.c	2009-03-11 13:49:53.000000000 +0100
@@ -24,7 +24,7 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
-#include <linux/hrtimer.h>
+#include <linux/sched.h>
 #include <linux/delay.h>
 #include <linux/bio.h>
 #include <linux/dma-mapping.h>
@@ -95,7 +95,7 @@
  * reads which takes nowhere near that long.  Older cards may be able to use
  * shorter timeouts ... but why bother?
  */
-#define r1b_timeout		ktime_set(3, 0)
+#define r1b_timeout		(HZ * 3)
 
 
 /****************************************************************************/
@@ -183,12 +183,11 @@ mmc_spi_readbytes(struct mmc_spi_host *h
 	return status;
 }
 
-static int
-mmc_spi_skip(struct mmc_spi_host *host, ktime_t timeout, unsigned n, u8 byte)
+static int mmc_spi_skip(struct mmc_spi_host *host, unsigned long timeout,
+			unsigned n, u8 byte)
 {
 	u8		*cp = host->data->status;
-
-	timeout = ktime_add(timeout, ktime_get());
+	unsigned long start = jiffies;
 
 	while (1) {
 		int		status;
@@ -203,22 +202,26 @@ mmc_spi_skip(struct mmc_spi_host *host, 
 				return cp[i];
 		}
 
-		/* REVISIT investigate msleep() to avoid busy-wait I/O
-		 * in at least some cases.
-		 */
-		if (ktime_to_ns(ktime_sub(ktime_get(), timeout)) > 0)
+		if (time_is_before_jiffies(start + timeout))
 			break;
+
+		/* If we need long timeouts, we may release the CPU.
+		 * We use jiffies here because we want to have a relation
+		 * between elapsed time and the blocking of the scheduler.
+		 */
+		if (time_is_before_jiffies(start+1))
+			schedule();
 	}
 	return -ETIMEDOUT;
 }
 
 static inline int
-mmc_spi_wait_unbusy(struct mmc_spi_host *host, ktime_t timeout)
+mmc_spi_wait_unbusy(struct mmc_spi_host *host, unsigned long timeout)
 {
 	return mmc_spi_skip(host, timeout, sizeof(host->data->status), 0);
 }
 
-static int mmc_spi_readtoken(struct mmc_spi_host *host, ktime_t timeout)
+static int mmc_spi_readtoken(struct mmc_spi_host *host, unsigned long timeout)
 {
 	return mmc_spi_skip(host, timeout, 1, 0xff);
 }
@@ -607,7 +610,7 @@ mmc_spi_setup_data_message(
  */
 static int
 mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t,
-	ktime_t timeout)
+	unsigned long timeout)
 {
 	struct spi_device	*spi = host->spi;
 	int			status, i;
@@ -712,7 +715,7 @@ mmc_spi_writeblock(struct mmc_spi_host *
  */
 static int
 mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
-	ktime_t timeout)
+	unsigned long timeout)
 {
 	struct spi_device	*spi = host->spi;
 	int			status;
@@ -802,7 +805,7 @@ mmc_spi_data_do(struct mmc_spi_host *hos
 	unsigned		n_sg;
 	int			multiple = (data->blocks > 1);
 	u32			clock_rate;
-	ktime_t			timeout;
+	unsigned long		timeout;
 
 	if (data->flags & MMC_DATA_READ)
 		direction = DMA_FROM_DEVICE;
@@ -816,8 +819,11 @@ mmc_spi_data_do(struct mmc_spi_host *hos
 	else
 		clock_rate = spi->max_speed_hz;
 
-	timeout = ktime_add_ns(ktime_set(0, 0), data->timeout_ns +
-			data->timeout_clks * 1000000 / clock_rate);
+	timeout = data->timeout_ns +
+		  data->timeout_clks * 1000000 / clock_rate;
+	timeout = usecs_to_jiffies((unsigned int)(timeout / 1000));
+	if (!timeout)
+		timeout = 1;
 
 	/* Handle scatterlist segments one at a time, with synch for
 	 * each 512-byte block

---
regards

i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH 6/7] mmc_spi: convert timeout handling to jiffies and avoid busy waiting

[David Brownell]

On Wednesday 11 March 2009, Wolfgang Mües wrote:
> o SD/MMC card timeouts can be very high. So avoid busy-waiting,
>   using the scheduler. Calculate all timeouts in jiffies units,
>   because this will give us the correct sign when to involve
>   the scheduler.

Of these patches, this is the one that bothers me the most.

First, earlier versions used jiffies ... but switching to
ktime sped things up.  (I forget the details by now.)
So it's odd to think that switching again could improve
things.  At any rate, if that's worth doing it's worth
having as a separate patch.

Second, as someone previously pointed out, there's a comment
there about switching to sleep() calls ... did you explore
just kicking in schedule_hrtimeout() or somesuch, right at
that point?  Heck, just calling schedule() would cut the
busy-wait overhead...

Re: [PATCH 6/7] mmc_spi: convert timeout handling to jiffies and avoid busy waiting

[Wolfgang Mües]

David,

Am Donnerstag, 12. März 2009 schrieb David Brownell:
> > o SD/MMC card timeouts can be very high. So avoid busy-waiting,
> >   using the scheduler. Calculate all timeouts in jiffies units,
> >   because this will give us the correct sign when to involve
> >   the scheduler.
>
> Of these patches, this is the one that bothers me the most.
>
> First, earlier versions used jiffies ... but switching to
> ktime sped things up.  (I forget the details by now.)

I doubt that it was the switching to ktime_t that sped things up.
(In fact, I found that mmc_spi.c uses ktime_t from the first moment it was 
included in the kernel 2007). Maybe a earlier version used jiffies, but
I have not find it.

The computing power needed for jiffies (32 bit) can not be more than the 
computing power for ktime_t (2x32 or 64 bit).

In fact, ktime_t with its nanosecond resolution seems to be an overkill if 
there are timeouts in the area of 10 .. 3000 ms.

My goal in programming is to keep it as simple and lightwight as possible.

> So it's odd to think that switching again could improve
> things.

Using jiffies does not sped up. This can't be. Speed is a matter of the fast 
reaction of the SD card. All we can do in the driver is to poll often, so 
that we do not incure an additional delay here.

My rationale for using jiffies is:

o The creator of an (embedded) system is choosing a value for HZ. A jiffie may 
be 10ms or 1ms. The creator chooses this value due to the soft realtime 
requirements of the system. So the value of HZ is a good estimation of the 
expected reaction time of the system.

o So if I use the value of HZ and say: "if I do busy waiting and polling for 
less than a jiffie, it's giving me fastest possible reaction time, without 
violating the expectation of the overall reaction time of the system".

o If I have to wait for MORE than a jiffie, I start to release computing power 
to other tasks in the system, but continue to poll with a resolution of 
jiffies. So the worst additional delay I impose will be a jiffie, and not 
more. I use the fact that the scheduler prefers friendly processes which
call schedule() often.

> Second, as someone previously pointed out, there's a comment
> there about switching to sleep() calls ... did you explore
> just kicking in schedule_hrtimeout() or somesuch, right at
> that point?

If I use a xxx_timeout() function, there will be fewer pollings because until 
the xxx_timeout function returns, there will be no polling, even if the whole 
system is idle.

And the result of the schedule_hrtimeout() is that the task is in the running 
state afterwards. The next poll of the SD card will happen if the scheduler 
selects this task to actually run. This is no difference to schedule().

So I expect the schedule_hrtimeout() function to perform worse as schedule(), 
because of the fewer polls. (The system may be more idle and conserve power 
during the waiting-for-response-time).  

> Heck, just calling schedule() would cut the busy-wait overhead...

Yepp. That's my primary goal. Some soft realtime tasks need to run in my 
target system...

So the overall result of my patch is:

o if the system is idle (expect for the file-IO-process), SD card IO 
performance will be the same as before, because schedule() will return 
immediately.

o if there are other tasks in the running state beside the file-IO-process, 
these tasks will run (not blocked as before) during the busy time of the SD 
card. Throughput to SD card will suffer a bit, but not notable. (Because the 
long waiting times only kick in if the SD card has to flush its internal 
buffers).

I have a takeMS SDHC card (speed class 6). If you write a continous data 
stream to the card, there is ONE long waiting time of 900ms for each 10s of 
stream writing. This was the worst timing I observed.

Hey, if someone comes with a better patch, I will appreciate it!

best regards

i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH 6/7] mmc_spi: convert timeout handling to jiffies and avoid busy waiting

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On Wed, 11 Mar 2009 14:35:03 +0100
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> @@ -816,8 +819,11 @@ mmc_spi_data_do(struct mmc_spi_host *hos
>  	else
>  		clock_rate = spi->max_speed_hz;
>  
> -	timeout = ktime_add_ns(ktime_set(0, 0), data->timeout_ns +
> -			data->timeout_clks * 1000000 / clock_rate);
> +	timeout = data->timeout_ns +
> +		  data->timeout_clks * 1000000 / clock_rate;
> +	timeout = usecs_to_jiffies((unsigned int)(timeout / 1000));
> +	if (!timeout)
> +		timeout = 1;
>  

Does this round properly upwards? Having a timeout 6 ms shorter than it
should be can be a real problem (although the huge timeout your other
patch introduces means we probably won't be affected by it).

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]