[PATCH] Fixes for several performance issues
Korolev, Alexey
alexey.korolev at intel.com
Wed Jun 14 07:12:47 EDT 2006
Hi All,
The current implementation has some performance and stability issues in
CFI drivers.
I found the following bugs:
1. Very low write performance on Sibley (perf tests demonstrated write
perf less than 100Kb/sec when it should be over 400Kb/sec):
The root of the issue is in the following code:
static inline void cfi_udelay(int us)
{
if (us >= 1000) {
msleep((us+999)/1000);
} else {
udelay(us);
cond_resched();
}
}
According to spec Sibley buffer write time is 2048us. It is greater than
1000us.
So we call msleep which has resolution 10000 us.
2. Erase speed is rather low on SND configurations on Sibley and
probably other NOR chips.
3. As Davide mentioned block locking works too slow in XIP mode. There
is another problem here:
(usec >= 1000000/HZ) is condition for sending CPU to idle state.
When zero timeout is passed from do_xxlock_oneblock to
xip_wait_for_operation it automatically sets the usec = 500000 - and we
send cpu to idle when it is unnecessary.
It also causes the issue with very low write performance on XIP on
several kind of NOR chips (passing zero timeout from do_write_buffer
after sending write_cmd command).
4. JFFS2 tests with CPU loading application cause "block erase error:
(status timeout)".
The reason of the failure is following:
During erase the process goes to sleep for rather long time.
CPU load application takes the control. Timeout tuning code in CFI
driver reduces erase timeout.
It happens until erase timeout is reduced to very low value.
This case maximum wait timeout will be set to HZ/2 which is lower than
erase timeout.
If nobody loads CPU next erase will wait for 0.5 sec (HZ/2) and return
with error flag.
The patch at the end of the letter removes all these issue.
The issue 3 may be easily fixed by modifying cpu idle conditions.
To solve the issues 1,2 and 4 I did the following:
1) Removed the timeout tuning from inval_cache_and_wait_for_operation.
IMO there are very few advantages in it.
Time out for msleep and "udelay + cond_resched" may vary significanlty.
Chip operations timeout may also vary.
So I guess that chip op timeout tuning is not a very good definition for
chip operations.
2) Waiting conditions in inval_cache_and_wait_for_operation now is based
on timer resolution
If timeout is lower than timer resolution then we do in cycle
"Checking the status"
udelay(1);
cond_resched();
Udelay is just a cycle so substitution of udelay(500) for 500 udelay(1)
will not affect on system performance negatively but it will provide
much higher resolution. It fixes the issue with very low performance on
Sibley. It increases write speed on it over 4 times.
If timeout is greater than timer resolution (probably erase operation)
We do the following
sleep for half of operation timeout
and do in cycle the following
"Checking the status"
sleep for timer resolution
It adds insignificant CPU overhead but it provides higher resolution and
resolves the issue of low erase speed
(Without patch: Erase Speed: 246 KB/sec With patch: Erase Speed: 318
KB/sec)
Please find the patch below:
--- a/drivers/mtd/chips/cfi_cmdset_0001.c 2006-06-13
21:52:19.809272680 +0400
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c 2006-06-13
21:53:49.959567752 +0400
@@ -907,7 +907,7 @@
static int __xipram xip_wait_for_operation(
struct map_info *map, struct flchip *chip,
- unsigned long adr, int *chip_op_time )
+ unsigned long adr, int chip_op_time )
{
struct cfi_private *cfi = map->fldrv_priv;
struct cfi_pri_intelext *cfip = cfi->cmdset_priv;
@@ -916,7 +916,7 @@
flstate_t oldstate, newstate;
start = xip_currtime();
- usec = *chip_op_time * 8;
+ usec = chip_op_time * 8;
if (usec == 0)
usec = 500000;
done = 0;
@@ -1000,7 +1000,7 @@
map_write(map, CMD(0x70), adr);
chip->state = oldstate;
start = xip_currtime();
- } else if (usec >= 1000000/HZ) {
+ } else if (chip_op_time >= 1000000/HZ) {
/*
* Try to save on CPU power when waiting delay
* is at least a system timer tick period.
@@ -1026,8 +1026,8 @@
#define XIP_INVAL_CACHED_RANGE(map, from, size) \
INVALIDATE_CACHED_RANGE(map, from, size)
-#define INVAL_CACHE_AND_WAIT(map, chip, cmd_adr, inval_adr, inval_len,
p_usec) \
- xip_wait_for_operation(map, chip, cmd_adr, p_usec)
+#define INVAL_CACHE_AND_WAIT(map, chip, cmd_adr, inval_adr, inval_len,
usec) \
+ xip_wait_for_operation(map, chip, cmd_adr, usec)
#else
@@ -1039,26 +1039,24 @@
static int inval_cache_and_wait_for_operation(
struct map_info *map, struct flchip *chip,
unsigned long cmd_adr, unsigned long inval_adr, int
inval_len,
- int *chip_op_time )
+ int chip_op_time )
{
struct cfi_private *cfi = map->fldrv_priv;
map_word status, status_OK = CMD(0x80);
- int z, chip_state = chip->state;
- unsigned long timeo;
-
+ int chip_state = chip->state;
+ unsigned long timeo, timer_resolution = 1000000/HZ;
+
spin_unlock(chip->mutex);
if (inval_len)
INVALIDATE_CACHED_RANGE(map, inval_adr, inval_len);
- if (*chip_op_time)
- cfi_udelay(*chip_op_time);
+
+ if ( chip_op_time / 2 > timer_resolution)
+ cfi_udelay(chip_op_time / 2 );
+
spin_lock(chip->mutex);
+
+ timeo = jiffies + HZ/2 + chip_op_time * 8 / timer_resolution;
- timeo = *chip_op_time * 8 * HZ / 1000000;
- if (timeo < HZ/2)
- timeo = HZ/2;
- timeo += jiffies;
-
- z = 0;
for (;;) {
if (chip->state != chip_state) {
/* Someone's suspended the operation: sleep */
@@ -1069,7 +1067,7 @@
spin_unlock(chip->mutex);
schedule();
remove_wait_queue(&chip->wq, &wait);
- timeo = jiffies + (HZ / 2); /* FIXME */
+ timeo = jiffies + HZ/2 + chip_op_time * 8 /
timer_resolution;
spin_lock(chip->mutex);
continue;
}
@@ -1086,18 +1084,14 @@
}
/* Latency issues. Drop the lock, wait a while and retry
*/
- z++;
spin_unlock(chip->mutex);
- cfi_udelay(1);
+ if (chip_op_time / 2 < timer_resolution)
+ cfi_udelay(1);
+ else
+ cfi_udelay(timer_resolution);
spin_lock(chip->mutex);
}
- if (!z) {
- if (!--(*chip_op_time))
- *chip_op_time = 1;
- } else if (z > 1)
- ++(*chip_op_time);
-
/* Done and happy. */
chip->state = FL_STATUS;
return 0;
@@ -1106,8 +1100,7 @@
#endif
#define WAIT_TIMEOUT(map, chip, adr, udelay) \
- ({ int __udelay = (udelay); \
- INVAL_CACHE_AND_WAIT(map, chip, adr, 0, 0, &__udelay); })
+ INVAL_CACHE_AND_WAIT(map, chip, adr, 0, 0, udelay)
static int do_point_onechip (struct map_info *map, struct flchip *chip,
loff_t adr, size_t len)
@@ -1331,7 +1324,7 @@
ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
adr, map_bankwidth(map),
- &chip->word_write_time);
+ chip->word_write_time);
if (ret) {
xip_enable(map, chip, adr);
printk(KERN_ERR "%s: word write error (status
timeout)\n", map->name);
@@ -1568,7 +1561,7 @@
ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
adr, len,
- &chip->buffer_write_time);
+ chip->buffer_write_time);
if (ret) {
map_write(map, CMD(0x70), cmd_adr);
chip->state = FL_STATUS;
@@ -1703,7 +1696,7 @@
ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
adr, len,
- &chip->erase_time);
+ chip->erase_time);
if (ret) {
map_write(map, CMD(0x70), adr);
chip->state = FL_STATUS;
More information about the linux-mtd
mailing list