Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Operating System/Drivers
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.4
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by fg - 2010-04-01
Last edited by fg - 2010-04-03

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

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.

Closed by  fg
2010-04-03 22:02
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

r25459

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 ?

Admin
fg commented on 2010-04-02 08:37

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.

Admin
fg commented on 2010-04-02 17:02

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.

Admin
fg commented on 2010-04-03 13:45

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.

Admin
fg commented on 2010-04-03 13:49

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

Admin
fg commented on 2010-04-03 14:13

This one should also handle multiple drives properly.

Admin
fg commented on 2010-04-03 14:23

Fix stupid warning.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing