fix MTD CFI/LPDDR flash driver huge latency bug
PATCH 1/1 fix MTD CFI/LPDDR flash driver huge latency bug
This patch fix a huge latency problem in the MTD CFI and LPDDR flash drivers.
The use of a memcpy() during a spinlock operation will cause very long thread context switch delays if the flash chip bandwidth is low and the data to be copied large, because a spinlock will disable preemption.
For example: A flash with 6,5 MB/s bandwidth will cause under ubifs, which request sometimes 128 KB (the flash erase size), a preemption delay of 20 milliseconds. High priority threads will not be served during this time, regardless whether this threads access the flash or not. This behavior breaks real time.
The patch change all the use of spin_lock operations for xxxx->mutex into mutex operations, which is exact what the name says and means.
There is no performance regression since the mutex is normally not acquired.
The patch is against kernel 2.6.33. Please merge it.
--- drivers/mtd/chips/cfi_cmdset_0001.c | 131 +++++++++++++++++----------------- drivers/mtd/chips/cfi_cmdset_0002.c | 127 +++++++++++++++++---------------- drivers/mtd/chips/cfi_cmdset_0020.c | 136 ++++++++++++++++++------------------ drivers/mtd/chips/gen_probe.c | 3 drivers/mtd/lpddr/lpddr_cmds.c | 79 ++++++++++---------- include/linux/mtd/flashchip.h | 4 - 6 files changed, 240 insertions(+), 240 deletions(-)
drivers/mtd/chips/cfi_cmdset_0001.c renamed from /drivers/mtd/chips/cfi_cmdset_0001.c +65 -66
@@ -727,8 +727,7 @@ static int cfi_intelext_partition_fixup(/* those should be reset too since they create memory references. */ init_waitqueue_head (&chip->wq); -spin_lock_init (&chip->_spinlock); - chip->mutex = &chip->_spinlock; +mutex_init (&chip->mutex); chip++; } }
@@ -774,9 +773,9 @@ static int chip_ready (struct map_info *if (chip->priv &&map_word_andequal (map, status, status_PWS, status_PWS))break ; -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);cfi_udelay (1 ); -spin_lock (chip->mutex); +mutex_lock (chip->mutex);/* Someone else might have been playing with it. */ return -EAGAIN ; }
@@ -823,9 +822,9 @@ static int chip_ready (struct map_info *return -EIO ; } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);cfi_udelay (1 ); -spin_lock (chip->mutex); +mutex_lock (chip->mutex);/* Nobody will touch it while it's in state FL_ERASE_SUSPENDING. So we can just loop here. */ }
@@ -852,10 +851,10 @@ static int chip_ready (struct map_info * sleep:set_current_state (TASK_UNINTERRUPTIBLE );add_wait_queue (&chip->wq, &wait); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);schedule ();remove_wait_queue (&chip->wq, &wait); -spin_lock (chip->mutex); +mutex_lock (chip->mutex);return -EAGAIN ; } }
@@ -901,20 +900,20 @@ static int get_chip(struct map_info *map* it'll happily send us to sleep. In any case, when * get_chip returns success we're clear to go ahead. */ - ret =spin_trylock (contender->mutex); + ret =mutex_trylock (contender->mutex);spin_unlock (&shared->lock);if (!ret) goto retry; -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); ret =chip_ready (map, contender, contender->start, mode); -spin_lock (chip->mutex); +mutex_lock (chip->mutex);if (ret == -EAGAIN ) { -spin_unlock (contender->mutex); +mutex_unlock (contender->mutex); goto retry; }if (ret) { -spin_unlock (contender->mutex); +mutex_unlock (contender->mutex);return ret; }spin_lock (&shared->lock);
@@ -923,10 +922,10 @@ static int get_chip(struct map_info *map* in FL_SYNCING state. Put contender and retry. */ if (chip->state ==FL_SYNCING ) {put_chip (map, contender, contender->start); -spin_unlock (contender->mutex); +mutex_unlock (contender->mutex); goto retry; } -spin_unlock (contender->mutex); +mutex_unlock (contender->mutex); }/* Check if we already have suspended erase
@@ -936,10 +935,10 @@ static int get_chip(struct map_info *mapspin_unlock (&shared->lock);set_current_state (TASK_UNINTERRUPTIBLE );add_wait_queue (&chip->wq, &wait); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);schedule ();remove_wait_queue (&chip->wq, &wait); -spin_lock (chip->mutex); +mutex_lock (chip->mutex); goto retry; }
@@ -969,12 +968,12 @@ static void put_chip(struct map_info *maif (shared->writing && shared->writing != chip) {/* give back ownership to who we loaned it from */ struct flchip *loaner = shared->writing; -spin_lock (loaner->mutex); +mutex_lock (loaner->mutex);spin_unlock (&shared->lock); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);put_chip (map, loaner, loaner->start); -spin_lock (chip->mutex); -spin_unlock (loaner->mutex); +mutex_lock (chip->mutex); +mutex_unlock (loaner->mutex);wake_up (&chip->wq);return ; }
@@ -1144,7 +1143,7 @@ static int __xipram xip_wait_for_operati (void )map_read (map, adr);xip_iprefetch ();local_irq_enable (); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);xip_iprefetch ();cond_resched ();
@@ -1154,15 +1153,15 @@ static int __xipram xip_wait_for_operati* a suspended erase state. If so let's wait * until it's done. */ -spin_lock (chip->mutex); +mutex_lock (chip->mutex);while (chip->state != newstate) {DECLARE_WAITQUEUE (wait, current);set_current_state (TASK_UNINTERRUPTIBLE );add_wait_queue (&chip->wq, &wait); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);schedule ();remove_wait_queue (&chip->wq, &wait); -spin_lock (chip->mutex); +mutex_lock (chip->mutex); }/* Disallow XIP again */ local_irq_disable ();
@@ -1218,10 +1217,10 @@ static int inval_cache_and_wait_for_operint chip_state = chip->state;unsigned int timeo, sleep_time, reset_timeo; -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);if (inval_len)INVALIDATE_CACHED_RANGE (map, inval_adr, inval_len); -spin_lock (chip->mutex); +mutex_lock (chip->mutex); timeo = chip_op_time_max;if (!timeo)
@@ -1241,7 +1240,7 @@ static int inval_cache_and_wait_for_oper }/* OK Still waiting. Drop the lock, wait a while and retry. */ -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);if (sleep_time >=1000000 /HZ ) {/* * Half of the normal delay still remaining
@@ -1256,17 +1255,17 @@ static int inval_cache_and_wait_for_opercond_resched (); timeo--; } -spin_lock (chip->mutex); +mutex_lock (chip->mutex);while (chip->state != chip_state) {/* Someone's suspended the operation: sleep */ DECLARE_WAITQUEUE (wait, current);set_current_state (TASK_UNINTERRUPTIBLE );add_wait_queue (&chip->wq, &wait); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);schedule ();remove_wait_queue (&chip->wq, &wait); -spin_lock (chip->mutex); +mutex_lock (chip->mutex); }if (chip->erase_suspended && chip_state ==FL_ERASING ) {/* Erase suspend occured while sleep: reset timeout */
@@ -1302,7 +1301,7 @@ static int do_point_onechip (struct map_/* Ensure cmd read/writes are aligned. */ cmd_addr = adr & ~(map_bankwidth (map)-1 ); -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, cmd_addr,FL_POINT );
@@ -1313,7 +1312,7 @@ static int do_point_onechip (struct map_ chip->state =FL_POINT ; chip->ref_point_counter++; } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1398,7 +1397,7 @@ static void cfi_intelext_unpoint(structelse thislen = len; -spin_lock (chip->mutex); +mutex_lock (chip->mutex);if (chip->state ==FL_POINT ) { chip->ref_point_counter--;if (chip->ref_point_counter ==0 )
@@ -1407,7 +1406,7 @@ static void cfi_intelext_unpoint(structprintk (KERN_ERR "%s: Warning: unpoint called on non pointed region\n" , map->name);/* Should this give an error? */ put_chip (map, chip, chip->start); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); len -= thislen; ofs =0 ;
@@ -1426,10 +1425,10 @@ static inline int do_read_onechip(struct/* Ensure cmd read/writes are aligned. */ cmd_addr = adr & ~(map_bankwidth (map)-1 ); -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, cmd_addr,FL_READY );if (ret) { -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1443,7 +1442,7 @@ static inline int do_read_onechip(structput_chip (map, chip, cmd_addr); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return 0 ; }
@@ -1506,10 +1505,10 @@ static int __xipram do_write_oneword(strreturn -EINVAL ; } -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, adr, mode);if (ret) { -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1555,7 +1554,7 @@ static int __xipram do_write_oneword(strxip_enable (map, chip, adr); out:put_chip (map, chip, adr); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1664,10 +1663,10 @@ static int __xipram do_write_buffer(stru/* Let's determine this according to the interleave only once */ write_cmd = (cfi->cfiq->P_ID !=0x0200 ) ?CMD (0xe8 ) :CMD (0xe9 ); -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, cmd_adr,FL_WRITING );if (ret) { -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1798,7 +1797,7 @@ static int __xipram do_write_buffer(struxip_enable (map, chip, cmd_adr); out:put_chip (map, chip, cmd_adr); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1877,10 +1876,10 @@ static int __xipram do_erase_oneblock(st adr += chip->start; retry: -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, adr,FL_ERASING );if (ret) { -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1936,7 +1935,7 @@ static int __xipram do_erase_oneblock(st }else if (chipstatus &0x20 && retries--) {printk (KERN_DEBUG "block erase failed at 0x%08lx: status 0x%lx. Retrying...\n" , adr, chipstatus);put_chip (map, chip, adr); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); goto retry; }else {printk (KERN_ERR "%s: block erase failed at 0x%08lx (status 0x%lx)\n" , map->name, adr, chipstatus);
@@ -1948,7 +1947,7 @@ static int __xipram do_erase_oneblock(stxip_enable (map, chip, adr); out:put_chip (map, chip, adr); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -1981,7 +1980,7 @@ static void cfi_intelext_sync (struct mtfor (i=0 ; !ret && i<cfi->numchips; i++) { chip = &cfi->chips[i]; -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, chip->start,FL_SYNCING );if (!ret) {
@@ -1992,7 +1991,7 @@ static void cfi_intelext_sync (struct mt* with the chip now anyway. */ } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); }/* Unlock the chips again */
@@ -2000,14 +1999,14 @@ static void cfi_intelext_sync (struct mtfor (i--; i >=0 ; i--) { chip = &cfi->chips[i]; -spin_lock (chip->mutex); +mutex_lock (chip->mutex);if (chip->state ==FL_SYNCING ) { chip->state = chip->oldstate; chip->oldstate =FL_READY ;wake_up (&chip->wq); } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); } }
@@ -2053,10 +2052,10 @@ static int __xipram do_xxlock_oneblock(s adr += chip->start; -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, adr,FL_LOCKING );if (ret) { -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -2090,7 +2089,7 @@ static int __xipram do_xxlock_oneblock(sxip_enable (map, chip, adr); out:put_chip (map, chip, adr); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -2155,10 +2154,10 @@ do_otp_read(struct map_info *map, structstruct cfi_private *cfi = map->fldrv_priv;int ret; -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, chip->start,FL_JEDEC_QUERY );if (ret) { -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return ret; }
@@ -2177,7 +2176,7 @@ do_otp_read(struct map_info *map, structINVALIDATE_CACHED_RANGE (map, chip->start + offset, size);put_chip (map, chip, chip->start); -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex);return 0 ; }
@@ -2452,7 +2451,7 @@ static int cfi_intelext_suspend(struct mfor (i=0 ; !ret && i<cfi->numchips; i++) { chip = &cfi->chips[i]; -spin_lock (chip->mutex); +mutex_lock (chip->mutex);switch (chip->state) {case FL_READY :
@@ -2484,7 +2483,7 @@ static int cfi_intelext_suspend(struct mcase FL_PM_SUSPENDED :break ; } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); }/* Unlock the chips again */
@@ -2493,7 +2492,7 @@ static int cfi_intelext_suspend(struct mfor (i--; i >=0 ; i--) { chip = &cfi->chips[i]; -spin_lock (chip->mutex); +mutex_lock (chip->mutex);if (chip->state ==FL_PM_SUSPENDED ) {/* No need to force it into a known state here,
@@ -2503,7 +2502,7 @@ static int cfi_intelext_suspend(struct m chip->oldstate =FL_READY ;wake_up (&chip->wq); } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); } }
@@ -2544,7 +2543,7 @@ static void cfi_intelext_resume(struct m chip = &cfi->chips[i]; -spin_lock (chip->mutex); +mutex_lock (chip->mutex);/* Go to known state. Chip may have been power cycled */ if (chip->state ==FL_PM_SUSPENDED ) {
@@ -2553,7 +2552,7 @@ static void cfi_intelext_resume(struct mwake_up (&chip->wq); } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); } if ((mtd->flags & MTD_POWERUP_LOCK)
@@ -2573,14 +2572,14 @@ static int cfi_intelext_reset(struct mtd/* force the completion of any ongoing operation and switch to array mode so any bootloader in flash is accessible for soft reboot. */ -spin_lock (chip->mutex); +mutex_lock (chip->mutex); ret =get_chip (map, chip, chip->start,FL_SHUTDOWN );if (!ret) {map_write (map,CMD (0xff ), chip->start); chip->state =FL_SHUTDOWN ;put_chip (map, chip, chip->start); } -spin_unlock (chip->mutex); +mutex_unlock (chip->mutex); }return 0 ;
drivers/mtd/chips/cfi_cmdset_0002.c renamed from /drivers/mtd/chips/cfi_cmdset_0002.c +65 -62
439 lines not on this page · raw message
drivers/mtd/chips/cfi_cmdset_0020.c renamed from /drivers/mtd/chips/cfi_cmdset_0020.c +68 -68
473 lines not on this page · raw message
drivers/mtd/chips/gen_probe.c renamed from /drivers/mtd/chips/gen_probe.c +1 -2
@@ -155,8 +155,7 @@ static struct cfi_private *genprobe_iden pchip->start = (i << cfi.chipshift); pchip->state =FL_READY ;init_waitqueue_head (&pchip->wq); -spin_lock_init (&pchip->_spinlock); - pchip->mutex = &pchip->_spinlock; +mutex_init (pchip->mutex); } }
drivers/mtd/lpddr/lpddr_cmds.c renamed from /drivers/mtd/lpddr/lpddr_cmds.c +39 -40
241 lines not on this page · raw message
include/linux/mtd/flashchip.h renamed from /include/linux/mtd/flashchip.h +2 -2
@@ -15,6 +15,7 @@* has asm/spinlock.h, or 2.4, which has linux/spinlock.h */ #include <linux/sched.h> +#include <linux/mutex.h> typedef enum { FL_READY,
@@ -74,8 +75,7 @@ struct flchip { unsigned int erase_suspended:1;unsigned long in_progress_block_addr; -spinlock_t *mutex; -spinlock_t _spinlock;/* We do it like this because sometimes they'll be shared. */ +struct mutex mutex[1 ];wait_queue_head_t wq;/* Wait on here when we're waiting for the chip to be ready */ int word_write_time;
Quoted 13 lines · On Sat, 06 Mar 2010 17:48:57 +0100 Stefani Seibold <[email protected]> wrote:
This patch fix a huge latency problem in the MTD CFI and LPDDR flash drivers.
The use of a
memcpy()during a spinlock operation will cause very long thread context switch delays if the flash chip bandwidth is low and the data to be copied large, because a spinlock will disable preemption.For example: A flash with 6,5 MB/s bandwidth will cause under ubifs, which request sometimes 128 KB (the flash erase size), a preemption delay of 20 milliseconds. High priority threads will not be served during this time, regardless whether this threads access the flash or not. This behavior breaks real time.
Quoted 5 lines
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.There is no performance regression since the mutex is normally not acquired.
hm, big scary patch. Are you sure this mutex is never taken from atomic or irq contexts? Is it ully tested with all relevant debug options and lockdep enabled?
Quoted 10 lines · Andrew Morton
Quoted 7 lines · On Sat, 06 Mar 2010 17:48:57 +0100 Stefani Seibold <[email protected]> wrote:
This patch fix a huge latency problem in the MTD CFI and LPDDR flash drivers.
The use of a
memcpy()during a spinlock operation will cause very long thread context switch delays if the flash chip bandwidth is low and the data to be copied large, because a spinlock will disable preemption.
Quoted 5 lines
Quoted 5 lines
For example: A flash with 6,5 MB/s bandwidth will cause under ubifs, which request sometimes 128 KB (the flash erase size), a preemption delay of 20 milliseconds. High priority threads will not be served during this time, regardless whether this threads access the flash or not. This behavior breaks real time.
I agree that's a problem, and it's not just real time that's affected.
I've just realised I have a video player with ~1.5 MB/s bandwidth 64kb/block flash attached, and this might be the reason JFFS2 activity makes video play less smooth on it. 44ms is even worse.
Quoted 2 lines
Quoted 2 lines
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.
It would be even better if it also split the critical sections into smaller ones with cond_resched() between, so that non-preemptible kernels benefit too.
Quoted 6 lines
Quoted 2 lines
There is no performance regression since the mutex is normally not acquired.
hm, big scary patch. Are you sure this mutex is never taken from atomic or irq contexts? Is it ully tested with all relevant debug options and lockdep enabled?
Including from mtdoops?
-- Jamie
Am Freitag, den 12.03.2010, 23:38 +0000 schrieb Jamie Lokier:
Quoted 6 lines
Quoted 3 lines · Andrew Morton
On Sat, 06 Mar 2010 17:48:57 +0100
Stefani Seibold <[email protected]> wrote:
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.It would be even better if it also split the critical sections into smaller ones with
cond_resched()between, so that non-preemptible kernels benefit too.
The problem is the memcpy operation which is very slow. A cond_resched wouldn't help, since the cpu bus is blocked during the transfer of the word.
Quoted 10 lines · Stefani Seibold
Am Freitag, den 12.03.2010, 23:38 +0000 schrieb Jamie Lokier:
Quoted 9 lines
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Sat, 06 Mar 2010 17:48:57 +0100 Stefani Seibold <[email protected]> wrote:
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.
Quoted 7 lines
Quoted 3 lines
It would be even better if it also split the critical sections into smaller ones with
cond_resched()between, so that non-preemptible kernels benefit too.The problem is the memcpy operation which is very slow. A
cond_reschedwouldn't help, since the cpu bus is blocked during the transfer of the word.
I mean split the memcpy into multiple smaller memcpys, so that the total time in each memcpy is limited to something reasonable.
The check in cond_resched() is fast, especially once cached. memcpy speed depends a lot on the attached flash and how everything's configured, varying from 2.5MB/s up to hundreds of MB/s. So how about doing cond_resched() every 256 bytes?
-- Jamie
Am Montag, den 15.03.2010, 03:03 +0000 schrieb Jamie Lokier:
Quoted 22 lines
Quoted 10 lines · Stefani Seibold
Am Freitag, den 12.03.2010, 23:38 +0000 schrieb Jamie Lokier:
Quoted 9 lines
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Sat, 06 Mar 2010 17:48:57 +0100 Stefani Seibold <[email protected]> wrote:
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.Quoted 7 lines
Quoted 3 lines
It would be even better if it also split the critical sections into smaller ones with
cond_resched()between, so that non-preemptible kernels benefit too.The problem is the memcpy operation which is very slow. A
cond_reschedwouldn't help, since the cpu bus is blocked during the transfer of the word.I mean split the memcpy into multiple smaller memcpys, so that the total time in each memcpy is limited to something reasonable.
Quoted 6 lines
The check in
cond_resched()is fast, especially once cached. memcpy speed depends a lot on the attached flash and how everything's configured, varying from 2.5MB/s up to hundreds of MB/s. So how about doingcond_resched()every 256 bytes?-- Jamie
I thoght about this aporoach and i don't like this idea. Why not using a preemptible kernel?
Stefani
Quoted 28 lines · Stefani Seibold
Am Montag, den 15.03.2010, 03:03 +0000 schrieb Jamie Lokier:
Quoted 27 lines
Quoted 10 lines · Stefani Seibold
Am Freitag, den 12.03.2010, 23:38 +0000 schrieb Jamie Lokier:
Quoted 9 lines
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Sat, 06 Mar 2010 17:48:57 +0100 Stefani Seibold <[email protected]> wrote:
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.Quoted 7 lines
Quoted 3 lines
It would be even better if it also split the critical sections into smaller ones with
cond_resched()between, so that non-preemptible kernels benefit too.The problem is the memcpy operation which is very slow. A
cond_reschedwouldn't help, since the cpu bus is blocked during the transfer of the word.I mean split the memcpy into multiple smaller memcpys, so that the total time in each memcpy is limited to something reasonable.
The check in
cond_resched()is fast, especially once cached. memcpy speed depends a lot on the attached flash and how everything's configured, varying from 2.5MB/s up to hundreds of MB/s. So how about doingcond_resched()every 256 bytes?
Quoted 4 lines
Quoted 1 line
-- Jamie
I thoght about this aporoach and i don't like this idea. Why not using a preemptible kernel?
Because it introduces too many risks to enable CONFIG_PREEMPT in a stable rolled out device which isn't using it already. Especially on devices where it's not well tested by other people, and with drivers that nobody ever used with CONFIG_PREEMPT before.
And because CONFIG_PREEMPT isn't always better. (Why do you think it's a config option?)
As a bug fix for observed high scheduling latency when a flash I/O is occurring, splitting the memcpys is a good choice. I will be trying it on my kernels, even if it doesn't get mainlined. Thanks for the idea ;-)
-- Jamie
Quoted 22 lines · Jamie Lokier
Quoted 12 lines
Quoted 12 lines
Quoted 3 lines
The problem is the memcpy operation which is very slow. A
cond_reschedwouldn't help, since the cpu bus is blocked during the transfer of the word.I mean split the memcpy into multiple smaller memcpys, so that the total time in each memcpy is limited to something reasonable.
The check in
cond_resched()is fast, especially once cached. memcpy speed depends a lot on the attached flash and how everything's configured, varying from 2.5MB/s up to hundreds of MB/s. So how about doingcond_resched()every 256 bytes?Quoted 4 lines
Quoted 1 line
-- Jamie
I thoght about this aporoach and i don't like this idea. Why not using a preemptible kernel?
Because it introduces too many risks to enable
CONFIG_PREEMPTin a stable rolled out device which isn't using it already. Especially on devices where it's not well tested by other people, and with drivers that nobody ever used withCONFIG_PREEMPTbefore.
Quoted 6 lines
And because
CONFIG_PREEMPTisn't always better. (Why do you think it's a config option?)As a bug fix for observed high scheduling latency when a flash I/O is occurring, splitting the memcpys is a good choice. I will be trying it on my kernels, even if it doesn't get mainlined. Thanks for the idea ;-)
Rather than pulling a number our of our posterior like "every 256 bytes" which might _really_ screw up performance of some architectures' memcpy routines, I suspect we might want the platform to provide an optimised "sleepable_memcpy" function which does it at whatever interval is appropriate for the memcpy routine in use. Or magically makes it preemptable. Or uses a DMA engine. Or whatever.
I wonder where else we could use such a function...
Signature · 2 lines
David Woodhouse Open Source Technology Centre [email protected] Intel Corporation
Quoted 8 lines · David Woodhouse
Rather than pulling a number our of our posterior like "every 256 bytes" which might _really_ screw up performance of some architectures' memcpy routines, I suspect we might want the platform to provide an optimised "
sleepable_memcpy" function which does it at whatever interval is appropriate for the memcpy routine in use. Or magically makes it preemptable. Or uses a DMA engine. Or whatever.I wonder where else we could use such a function...
The posterior number isn't great, although I don't see how it would really harm memcpy performance to check current->need_resched even quite often.
In this instance, the speed depends on the flash which can be as much as 100x slower than RAM - that's the particular situation where it might be most useful to split the copies.
Other uses of sleepable_memcpy you may be thinking of could be operating on RAM only, so the number should be 100x larger for them.
In other words, "whatever interval is appropriate for memcpy" does not exist, and could not be hard-coded into sleepable_memcpy. It's whatever interval is appropriate for the particular memory being copied, so it would have to be a parameter.
-- Jamie
Am Freitag, den 12.03.2010, 14:23 -0800 schrieb Andrew Morton:
Quoted 19 lines
Quoted 16 lines · On Sat, 06 Mar 2010 17:48:57 +0100 Stefani Seibold <[email protected]> wrote:
This patch fix a huge latency problem in the MTD CFI and LPDDR flash drivers.
The use of a
memcpy()during a spinlock operation will cause very long thread context switch delays if the flash chip bandwidth is low and the data to be copied large, because a spinlock will disable preemption.For example: A flash with 6,5 MB/s bandwidth will cause under ubifs, which request sometimes 128 KB (the flash erase size), a preemption delay of 20 milliseconds. High priority threads will not be served during this time, regardless whether this threads access the flash or not. This behavior breaks real time.
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.
Quoted 8 lines
Quoted 2 lines
There is no performance regression since the mutex is normally not acquired.
hm, big scary patch. Are you sure this mutex is never taken from atomic or irq contexts? Is it ully tested with all relevant debug options and lockdep enabled?
I have analyzed this drivers and IMHO i don't think there will be used from irq or atomic contexts. There is no request interrupt and there are a lot msleep and add_wait_queues/schedule calls during holding the mutex, which are not very useful in a irq or atomic context. But i don't know the whole mtd stack.
I tested the patch with the following kernel debug options:
CONFIG_DEBUG_KERNEL=yCONFIG_DETECT_SOFTLOCKUP=yCONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE=0CONFIG_SCHED_DEBUG=yCONFIG_SCHEDSTATS=yCONFIG_TIMER_STATS=yCONFIG_DEBUG_MUTEXES=yCONFIG_DEBUG_SPINLOCK_SLEEP=y
Quoted 37 lines · On Sat, 13 Mar 2010 13:31:30 +0100 Stefani Seibold <[email protected]> wrote:
Am Freitag, den 12.03.2010, 14:23 -0800 schrieb Andrew Morton:
Quoted 19 lines
Quoted 16 lines · On Sat, 06 Mar 2010 17:48:57 +0100 Stefani Seibold <[email protected]> wrote:
This patch fix a huge latency problem in the MTD CFI and LPDDR flash drivers.
The use of a
memcpy()during a spinlock operation will cause very long thread context switch delays if the flash chip bandwidth is low and the data to be copied large, because a spinlock will disable preemption.For example: A flash with 6,5 MB/s bandwidth will cause under ubifs, which request sometimes 128 KB (the flash erase size), a preemption delay of 20 milliseconds. High priority threads will not be served during this time, regardless whether this threads access the flash or not. This behavior breaks real time.
The patch change all the use of
spin_lockoperations forxxxx->mutexinto mutex operations, which is exact what the name says and means.Quoted 8 lines
Quoted 2 lines
There is no performance regression since the mutex is normally not acquired.
hm, big scary patch. Are you sure this mutex is never taken from atomic or irq contexts? Is it ully tested with all relevant debug options and lockdep enabled?
I have analyzed this drivers and IMHO i don't think there will be used from irq or atomic contexts. There is no request interrupt and there are a lot msleep and add_wait_queues/schedule calls during holding the mutex, which are not very useful in a irq or atomic context. But i don't know the whole mtd stack.
I tested the patch with the following kernel debug options:
Quoted 9 lines
CONFIG_DEBUG_KERNEL=yCONFIG_DETECT_SOFTLOCKUP=yCONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE=0CONFIG_SCHED_DEBUG=yCONFIG_SCHEDSTATS=yCONFIG_TIMER_STATS=yCONFIG_DEBUG_MUTEXES=yCONFIG_DEBUG_SPINLOCK_SLEEP=y
Neato. As was mentioned, one thing to check is the mtdoops path. oopses can happen with locks held, from IRQ context, etc.
If we're trying to take that mutex in oops context then I guess that's fixable by just not taking it and hoping for the best. Or, better, mutex_trylock() and conditional mutex_unlock() to try to be nice to possible concurrent activity on other CPUs.
Am Samstag, den 13.03.2010, 06:25 -0500 schrieb Andrew Morton:
Quoted 36 lines · On Sat, 13 Mar 2010 13:31:30 +0100 Stefani Seibold <[email protected]> wrote:
Quoted 25 lines
Am Freitag, den 12.03.2010, 14:23 -0800 schrieb Andrew Morton:
Quoted 4 lines
On Sat, 06 Mar 2010 17:48:57 +0100
Stefani Seibold <[email protected]> wrote:The patch change all the use of
spin_lockoperations forxxxx->mutexQuoted 10 lines
Quoted 4 lines
into mutex operations, which is exact what the name says and means.
There is no performance regression since the mutex is normally not acquired.
hm, big scary patch. Are you sure this mutex is never taken from atomic or irq contexts? Is it ully tested with all relevant debug options and lockdep enabled?
I have analyzed this drivers and IMHO i don't think there will be used from irq or atomic contexts. There is no request interrupt and there are a lot msleep and add_wait_queues/schedule calls during holding the mutex, which are not very useful in a irq or atomic context. But i don't know the whole mtd stack.
I tested the patch with the following kernel debug options:
Quoted 9 lines
CONFIG_DEBUG_KERNEL=yCONFIG_DETECT_SOFTLOCKUP=yCONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE=0CONFIG_SCHED_DEBUG=yCONFIG_SCHEDSTATS=yCONFIG_TIMER_STATS=yCONFIG_DEBUG_MUTEXES=yCONFIG_DEBUG_SPINLOCK_SLEEP=y
Quoted 3 lines
Neato. As was mentioned, one thing to check is the mtdoops path. oopses can happen with locks held, from IRQ context, etc.
Okay, i didn't checked that case. But the old code has also a dead lock, if the oops occurred during the spinlock(xxx->mutex) was held. With the new mutex solution the change is bigger to run into that deadlock due the possible preemption.
But i did a "grep" at the whole mtd code and there is no panic_write function assigned to mtd_info struct for the CFI flash chips. So this problem will currently never occure.
Quoted 5 lines
If we're trying to take that mutex in oops context then I guess that's fixable by just not taking it and hoping for the best. Or, better,
mutex_trylock()and conditionalmutex_unlock()to try to be nice to possible concurrent activity on other CPUs.
Concurrent access are dangerous and in most cases are not possible, that's why the spinlock(xxxx->mutex) was for.
I also did some concurrency checks like:
cat /dev/zero >/flash/aa & cat /dev/zero >/flash/bb
without and side effects.