Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11167 - Add IO priorities, and lower the priority of the dircache thread

Attached to Project: Rockbox
Opened by Frank Gevaerts (fg) - Thursday, 01 April 2010, 23:35 GMT+2
Last edited by Frank Gevaerts (fg) - Sunday, 04 April 2010, 00:02 GMT+2
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch adds a storage_is_busy_elsewhere() function that returns true if another thread has recently (HZ/10 right now) accessed the disk. This is then used in the dircache thread to sleep a bit in such cases, so user-visible disk accesses like buffering or loading the wps get absolute priority. This improves boot times to the root menu from 11.4 seconds to 1.5 seconds, with playback then immediately starting.

The current code is still mostly a proof of concept, with things only implemented for ATA (ir probably won't even compile for others), and storage_* integration isn't done properly.
It should be perfectly safe though.
   dircache_backoff.diff (2.9 KiB) 
 firmware/export/storage.h  |    3 +++
 firmware/SOURCES           |    2 --
 firmware/storage.c         |   17 ++++++++++++++++-
 firmware/common/dircache.c |    5 +++++
 firmware/drivers/ata.c     |    2 ++
 5 files changed, 26 insertions(+), 3 deletions(-)

This task depends upon

Closed by  Frank Gevaerts (fg)
Sunday, 04 April 2010, 00:02 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  r25459
Comment by amaury pouly (pamaury) - Friday, 02 April 2010, 10:00 GMT+2
From the dircache point of view, the modification is trivial and makes sense so I agree with the idea. My only point is that it will slow down the update process in background and is more likely to trigger et know bug that Slasheri told me would fix but he never did (I promise one day I'll fix it :)).
Regarding the storage_* integration, I find it really akward but I'm not sure there is a proper way to do such a thing (except giving the threads an explicit I/O priority ?).

And finally, does such a modification makes sense on flash-based (ie non-hard drive) storage ?
Comment by Frank Gevaerts (fg) - Friday, 02 April 2010, 10:37 GMT+2
The storage_* integration is indeed awkward. It was designed to be implemented quickly so I could test, not to be an example of clean code (which is what I meant by "mostly a proof of concept")

I think it probably makes sense on flash as well. The numbers will of course be vastly different, but the dircache thread will still slow things down a bit by using bandwidth while things like buffering are going on. We'll see.
Comment by Frank Gevaerts (fg) - Friday, 02 April 2010, 19:02 GMT+2
An attempt at doing this in a cleanly integrated way, for all storage types. This one uses a wrapper function for storage_(read|write)_sectors(), which means that the IF_MD2() stuff is back for all users of those.
I've made storage_is_busy_elsewhere() (and the corresponding code elsewhere) dependent on HAVE_DIRCACHE. It might be better to give it its own CONFIG_* define

There's a RAM delta of around 32 bytes for targets without dircache, and from 200 to 300 bytes for targets with dircache.
   dircache_backoff_v2.diff (16.7 KiB) 
 firmware/export/storage.h       |   21 +---
 firmware/usbstack/usb_storage.c |    8 -
 firmware/SOURCES                |    2 
 firmware/storage.c              |  192 ++++++++++++++++++++++++----------------
 firmware/common/disk.c          |    2 
 firmware/common/dircache.c      |    8 +
 firmware/drivers/fat.c          |   18 +--
 7 files changed, 150 insertions(+), 101 deletions(-)

Comment by Frank Gevaerts (fg) - Saturday, 03 April 2010, 15:45 GMT+2
This patch reworks the entire thing to a more generic io priority concept. I think it's about OK for commit now

There are still a few possible questions:
- Are the times I picked good enough? (STORAGE_MINIMUM_IDLE_TIME and STORAGE_DELAY_UNIT in storage.c)
- Do we want this only for disk targets, or also for flash? I think it can't hurt flash, so I'd do it everywhere. (the current patch enables it for all targets that have dircache)
- Currently the waiting loop checks with the same priority for each iteration. The alternative would be to slowly raise the priority (thereby eliminating one variable). As long as we only use two priorities this won't make a difference. Which should we pick?

The changes in disk.c, fat.c, and usb_storage.c are only half related, and are due to the change from macros to wrapper functions.
   io_priority_v1.diff (22.2 KiB) 
 firmware/export/thread.h        |   10 +
 firmware/export/config.h        |    4 
 firmware/export/storage.h       |   20 +--
 firmware/usbstack/usb_storage.c |    8 -
 firmware/SOURCES                |    2 
 firmware/storage.c              |  218 +++++++++++++++++++++++++++-------------
 firmware/common/disk.c          |    2 
 firmware/common/dircache.c      |    8 +
 firmware/thread.c               |   19 +++
 firmware/drivers/fat.c          |   18 +--
 10 files changed, 211 insertions(+), 98 deletions(-)

Comment by Frank Gevaerts (fg) - Saturday, 03 April 2010, 15:49 GMT+2
Oh, one more thing that would be nice to have is to differentiate between drives on multi-drive systems. Currently I don't do this because the drivers don't give enough information (*_last_disk_activity() doesn't take a drive argument)

In theory this could slow down things on multidrive systems, but I doubt if it's ever going to be a problem in practice
Comment by Frank Gevaerts (fg) - Saturday, 03 April 2010, 16:13 GMT+2
This one should also handle multiple drives properly.
   io_priority_v2.diff (22.4 KiB) 
 firmware/export/thread.h        |   10 +
 firmware/export/config.h        |    4 
 firmware/export/storage.h       |   20 +--
 firmware/usbstack/usb_storage.c |    8 -
 firmware/SOURCES                |    2 
 firmware/storage.c              |  223 +++++++++++++++++++++++++++-------------
 firmware/common/disk.c          |    2 
 firmware/common/dircache.c      |    8 +
 firmware/thread.c               |   19 +++
 firmware/drivers/fat.c          |   18 +--
 10 files changed, 216 insertions(+), 98 deletions(-)

Comment by Frank Gevaerts (fg) - Saturday, 03 April 2010, 16:23 GMT+2
Fix stupid warning.
   io_priority_v3.diff (22.4 KiB) 
 firmware/export/thread.h        |   10 +
 firmware/export/config.h        |    4 
 firmware/export/storage.h       |   20 +--
 firmware/usbstack/usb_storage.c |    8 -
 firmware/SOURCES                |    2 
 firmware/storage.c              |  223 +++++++++++++++++++++++++++-------------
 firmware/common/disk.c          |    2 
 firmware/common/dircache.c      |    8 +
 firmware/thread.c               |   19 +++
 firmware/drivers/fat.c          |   18 +--
 10 files changed, 216 insertions(+), 98 deletions(-)

Loading...