Rockbox

Tasklist

FS#8568 - backrolling spinlock changes (svn 16105)

Attached to Project: Rockbox
Opened by Andree Buschmann (Buschel) - Saturday, 09 February 2008, 16:02 GMT
Last edited by Michael Sevakis (MikeS) - Friday, 22 February 2008, 17:30 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch undos the spinlock changes which came in with svn 16105. With this patch the strange HDD activity on iPod 5G's with 60/80GB HDDs is solved. Furthermore this patch should therefor solve the low battery runtime for these players.
This task depends upon

Closed by  Michael Sevakis (MikeS)
Friday, 22 February 2008, 17:30 GMT
Reason for closing:  Fixed
Additional comments about closing:  Temporary measure to deal with the specific targets is committed.
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 09 February 2008, 18:03 GMT
When you say "strange HDD activity on iPod 5G", are you referring to  FS#8397 ? This bug was reported to appear before r16105 was committed.
Comment by Andree Buschmann (Buschel) - Saturday, 09 February 2008, 18:11 GMT
No, I am talking of very long loading time for buffering on iPod Video 60/80GB (not the 30GB!). This patch was also tested by a volunteer in IRC and solves this issue.
Comment by Sascha Wolf (Horscht) - Saturday, 09 February 2008, 20:55 GMT
Let me clarify. From rev 16105 onwards, the SVN had an issue on 60/80 Ipod videos, which caused the buffering of music to take a lot longer than before, which degraded teh battery life by 2+ hours. I stopwatched several SVNs buffering 21 files with a total size of 125MB or something. I measured the time from HD spin up to HD spindown, and also noted the times HD activity seemed to be done (immense disk reading).
svn pre 16105 came out 20 seconds intensive reading, total spin time of HD was 1:20 minutes.
svn post 16105 came out 1:07 minutes intesive reading, total spin time of HD was 2:30 mintes
i tested up to svn rev 16243 with the same issues as post 16105. Latest SVN (16254) with above patch returned the intense reading to 20 secs, and total spin time to 1:20
Comment by Ryan Sawhill (ryran) - Saturday, 09 February 2008, 21:03 GMT
I hadn't taken the time to do controlled testing but I had definitely noticed this. Good job Sascha.
Comment by Michael Sevakis (MikeS) - Sunday, 10 February 2008, 02:21 GMT
I'd suggest looking for a deeper reason for the problem and wouldn't consider this patch to really solve the problem but to be a reversion to hiding something important. How about finding _why_ it happens?

1) Why no issue on any other model? I did fix a stupid mistake of mine which did affect many models and sensibly so. I haven't perceived any negative effect on the many players I have whether flash based or HD based. Perhaps we simply have some other "oops" factor at play.

2) This should have no effect at all since a mutex provides no more a barrier to critical section entry and thread execution than the "spinlock". The difference may be a change in thread scheduling which could cause misbehavior due to race conditions.

3) Scheduler turnaround overheads are in the 15-25 microsecond range for even the worst-case of a queue_send to a thread on a timeout wait (PP502x at 80MHz). Hardly significant relative to HD speeds and the rather low 2000 or so read calls to fill the file buffer should at worst add up to a few millseconds. That's _if_ blocking and wakeup is involved.

4) Based on above, does 5g have any particularly time-consuming subsystem that placing a thread at the end of the run list may delay execution of the buffering thread? Changing to insert-next didn't show an effect but that was before fixing the (rather embarrassing) priority_yield mistake.

5) Does MPEGPlayer significantly vary in its buffering behavior with or without this patch? It should be equally if not more adversely affected. Observe the audio buffer fullness indication in "Show FPS".

Perhaps I'll post the change mentioned in 4) here so it may be tested with the other fix in place.
Comment by Andree Buschmann (Buschel) - Sunday, 10 February 2008, 04:45 GMT
MikeS:

I do not want this patch as final solution. My aim is to kind of repair the svn for the 5G 60/80GB users. You will understand that it is quite hard to do battery runtime tests (which were and are needed for the device disables patch) when the svn degraded that much.

So, why not backrolling the changes in svn to have svn working for all and open an flyspray entry to re-add it and work on the 5G issue? There are several testers available who can (and I think will) support any testing.
Comment by Michael Sevakis (MikeS) - Sunday, 10 February 2008, 07:05 GMT
Sure. I just really need to know what's going on here with one particular device. I'd even like to see a short video of the buffering screen under varying conditions with and without something else updating like the database or tagcache in the background. What's happening with only playback accessing the drive will have no codepath difference since the lock will be granted immediately with no scheduler involvement at all.

With this patch the thread using the disk will be more aggressively run if it was blocked and explicitly woken so it should actually have an advantage over spinlock.
Comment by Andree Buschmann (Buschel) - Sunday, 10 February 2008, 07:48 GMT
MikeS: Only the 5G iPod with a large 60GB or 80GB disk has an issue (on my 5G 30GB there is no problem at all). For the 5G iPods there were added changes in ata.c and an additional #define (#define MAX_PHYS_SECTOR_SIZE 1024) in config-ipodvideo.h to get the large disks working in rockbox. Either if it's timing or an general issue -> I assume the root cause is somewhere within ata.c in combination with the larger 5G disks.
Comment by Sascha Wolf (Horscht) - Sunday, 10 February 2008, 10:07 GMT
Michaels patch (applied on stock svn, new checkout) has slightly better values than unpatched SVN, but still quite a fair amount behind Andree's revert.
I now get 50 seconds of "buffering" and 2:05 minutes HD spin time. I have been asked several times, why my HD doesn't shut off right after buffering, so I think i should calrify this here.

in the first 20/50/70 seconds (depending on which patch applied) i can hear continous read activity on the HD (the usual "ticking" noise). After that, i can only hear the HD spining. Every 4 or 5 seconds read activity can be heard on the HD for a brief second. That's why the HD shuts down after 1:20/2:05/2:30 respectively

I hope these values are helpfull at all. All values were measured with no background activity (they were measured several times on each patch/build as well, all within +/- 2 seconds)
Comment by Sascha Wolf (Horscht) - Sunday, 10 February 2008, 11:15 GMT
Ok, I retested several patches and here are the results (all based upon SVN 16263)

version: hd activity time, hd spintime

stock: 1:05min 2:10min
stock + ata_priority_realtime: 0:55min 2:05min
stock + ata_priority_realtime + thread_wakeup_insert_first: 0:45min 2:00min
stock + spinlock_rollback_v02 : 0:20min 1:30min
Comment by Sascha Wolf (Horscht) - Wednesday, 13 February 2008, 18:23 GMT
Sorry to bother, but do you happen to have a version of that patch that works with recent SVN? I do not have any coding skills apart from basic html, so I am not able to synch it myself. As the disk thrashing issue is still persistent, I would like to apply this patch to enhance my battery runtime.

thanks in advance

Horscht
Comment by Andree Buschmann (Buschel) - Thursday, 14 February 2008, 19:16 GMT
Hi, I've tried to sync against svn 16313. Not sure whether it still works as there were many changes since the spinlock change.
Comment by Andree Buschmann (Buschel) - Thursday, 14 February 2008, 19:17 GMT
Always hitting the wrong button :/
Comment by Michael Sevakis (MikeS) - Tuesday, 19 February 2008, 06:55 GMT
I think I'll just whip up a kernel object that does the yielding spinlock thing instead of rolling back for all devices and changing the one in current form and using that if MAX_PHYS_SECTOR_SIZE is defined.

Other work is in progress to deal properly with priorities in general and I could post a patch of that (perhaps under a different FS). It won't have the inheritance protocols yet (maybe) and will be a tiny bit out of tune but the results are quite good for priority in general anyway and I'm curious how it so far affects this problem. The said temp object can be easily and ultimately removed - no fuss, no muss.
Comment by Michael Sevakis (MikeS) - Tuesday, 19 February 2008, 07:00 GMT
BTW: I'm curious why spinlocks are rolled back outside the ata driver if the issue is the disk only.
Comment by Michael Sevakis (MikeS) - Tuesday, 19 February 2008, 09:00 GMT
This rolls back "spinlock" just in the ata driver for the affected targets only.
Comment by Sascha Wolf (Horscht) - Thursday, 21 February 2008, 07:16 GMT
Patch works "fine", but I suppose this wont go into SVN as a fix to the issue?
Comment by Michael Sevakis (MikeS) - Thursday, 21 February 2008, 11:06 GMT
I will if it's "fine" since it's a temp fix limited to the specific targets and not a global revert and so it's easily removed with a simple delete of a few lines. I hope you'll be willing to test more permanent threading developments now in the works anyway when they're ready.
Comment by Sascha Wolf (Horscht) - Thursday, 21 February 2008, 17:18 GMT
I will be glad to help testing stuff. Please do consider, though, that I have close to no coding knowledge (by close i mean "very close"). I do know how to apply patches and compile using configure and make. Anything beyond that is out of my range (i.e. reading or writing code).

Loading...