Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#12344 - Sansa Clip+: PANIC occurred when dircache is enabled

Attached to Project: Rockbox
Opened by Akio Idehara (idak) - Saturday, 22 October 2011, 02:09 GMT+2
Last edited by MichaelGiacomelli (saratoga) - Sunday, 04 November 2012, 18:38 GMT+2
Task Type Bugs
Category Operating System/Drivers
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Sansa Clip+: *PANIC* occurred when dircache is enabled. This occurred the simulator and the real target in r30819.
--
(1) Go "Settings" ->"General Settings" ->"System"->"Disk"->"Directory Cache"
(2) Select "No"
(3) Then Select "Yes"
--
The simulator outputs the following.
--
audio_reset_buffer_noalloc(): EOM (3960 > 3912)
--
This task depends upon

Closed by  MichaelGiacomelli (saratoga)
Sunday, 04 November 2012, 18:38 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  Looks like this is fixed and just left open. If i'm wrong, please reopen.
Comment by Fred Bauer (freddyb) - Saturday, 22 October 2011, 11:31 GMT+2
I messed up r30818 and reverted it. Do you still have the problem with current SVN?
Comment by Akio Idehara (idak) - Saturday, 22 October 2011, 13:44 GMT+2
This bug still occurs in current SVN (r30824).
I bisecting this bug and I found this bug was introduced in r 30393.
Comment by Fred Bauer (freddyb) - Monday, 07 November 2011, 23:46 GMT+2
The problem is that the audio file buffer gets a small allocation after creation and scan of dircache. After dircache is created from scratch the playback file buffer is allocated from the first hole rather than the largest. This hole created by shrinking the dircache allocation from scanning mode to operation may not have the minimum size required for playback to operate. Even if the memory is enough to prevent the crash it is still not efficient to have a small file buffer when there may be several times as much contiguous memory available.

The patch below does several things to improve the interaction between dircache / buflib / playback. It adds some code to buflib.c which tries to find the best match for the requested size instead of the first available. (i.e. it won't return a megabyte for core_alloc_max() when 6 megs contiguous are available in a higher block.) It resets the audio file buffer when playback is stopped to claim the largest hunk of memory for the file buffer. It shows size of val in the buflib debug menu. Finally, it updates some additional pointers during the buflib move callback for dircache. Unfortunately, you still have to stop playback after the initial dircache build is done to have fully compacted memory.
   buflib_bestmatch.5.patch (4.9 KiB) 
 apps/playback.c            |    3 ++
 apps/debug_menu.c          |    4 ++-
 firmware/buflib.c          |   59 ++++++++++++++++++++++++++++-----------------
 firmware/common/dircache.c |   15 +++++++++++
 4 files changed, 59 insertions(+), 22 deletions(-)

Comment by Akio Idehara (idak) - Tuesday, 08 November 2011, 15:31 GMT+2
Applying this patch, Panic does not occur.
But I have one question about this patch.
Is "Please reboot to enable" message intentional?
(r30393 comment is "Allow dircache to be enabled without reboot.")
Comment by Fred Bauer (freddyb) - Tuesday, 08 November 2011, 16:38 GMT+2
No, it's not desired but the live build can fail and require a reboot. If you reboot with dircache enabled how much memory does it use? (Main menu > system > debug > View dircache info) Would you try this patch and see what error message it gives when you turn on dircache?
   buflib_bestmatch.5.debug.patch (5.3 KiB) 
 apps/playback.c            |    3 ++
 apps/debug_menu.c          |    4 ++-
 firmware/buflib.c          |   59 ++++++++++++++++++++++++++++-----------------
 firmware/common/dircache.c |    8 ++++++
 4 files changed, 52 insertions(+), 22 deletions(-)

Comment by Fred Bauer (freddyb) - Wednesday, 09 November 2011, 07:09 GMT+2
Reworked a bit. The dircache memory allocation now attempts successively smaller allocations until it succeeds and calls a audio_hard_stop() when done to prompt memory compaction for the audio file buffer. You should now be able to enable dircache without rebooting even with the database in ram and still have a large contiguous hunk of memory for the audio file buffer.
   buflib_bestmatch.6.patch (5.3 KiB) 
 apps/playback.c            |    3 ++
 firmware/buflib.c          |   59 ++++++++++++++++++++++++++++-----------------
 firmware/common/dircache.c |   30 +++++++++++++++++++++-
 3 files changed, 69 insertions(+), 23 deletions(-)

Comment by Akio Idehara (idak) - Wednesday, 09 November 2011, 15:48 GMT+2
(1)
Applying 5.debug.patch, and
Splash message is "dircache memory allocation failed"

(2)
Applying 6.patch and Splash message is nothing.
# But playback is stopped.

(3)
Dircache Info is the following.
---
Cache initialized: Yes
Cache size: 1070835 B
Last size: 1070835 B
Limit: 6291456 B
Reserve: 3/65536 B
Scanning took: 0 s
Entry count: 12514
---
Comment by Fred Bauer (freddyb) - Wednesday, 09 November 2011, 16:39 GMT+2
I think stopping playback after shrinking the dircache buffer to the operational size is as good as I can get it. Without the audio_hard_stop() we could be wasting lots of RAM. Playback is interrupted when taking memory from playback anyway and changing the dircache setting should be a rare event. Does this seem acceptable?
Comment by Akio Idehara (idak) - Wednesday, 09 November 2011, 17:41 GMT+2
I'm not a developer, so I cannot say stopping playback is acceptable or not.
But in my opinion, resuming playback after stopping is more acceptable.

Please see  FS#12279  and r30900
http://www.rockbox.org/tracker/task/12279
http://svn.rockbox.org/viewvc.cgi?view=rev;revision=30900
Comment by Fred Bauer (freddyb) - Friday, 11 November 2011, 21:34 GMT+2
Here's an update to try to restore playback using the same code from the playback.c buflib move callback.
   buflib_bestmatch.8.patch (9.1 KiB) 
 apps/playback.c            |   52 ++++++++++++++++++++++++++++-----------
 apps/playback.h            |    2 -
 firmware/buflib.c          |   59 ++++++++++++++++++++++++++++-----------------
 firmware/common/dircache.c |   32 ++++++++++++++++++++++--
 uisimulator/common/io.c    |    1 
 5 files changed, 106 insertions(+), 40 deletions(-)

Comment by Fred Bauer (freddyb) - Friday, 11 November 2011, 23:11 GMT+2
Oops. Posted old version before.
   buflib_bestmatch.9.patch (9.7 KiB) 
 apps/playback.c            |   53 ++++++++++++++++++++++++++++------------
 apps/playback.h            |    1 
 firmware/export/audio.h    |    1 
 firmware/buflib.c          |   59 ++++++++++++++++++++++++++++-----------------
 firmware/common/dircache.c |   34 +++++++++++++++++++++++--
 uisimulator/common/io.c    |    1 
 6 files changed, 109 insertions(+), 40 deletions(-)

Comment by Akio Idehara (idak) - Saturday, 12 November 2011, 00:45 GMT+2
I think this is perfect!
Thank you very much for all your hard work.
Comment by Fred Bauer (freddyb) - Saturday, 12 November 2011, 01:35 GMT+2
You're welcome!
Comment by Bertrik Sikken (bertrik) - Saturday, 12 November 2011, 16:25 GMT+2
I guess you don't need/intended the printf in io.c
Comment by Thomas Martitz (kugel.) - Saturday, 12 November 2011, 16:33 GMT+2
I would rather not see this committed. I can image a few better solutions.
Comment by Fred Bauer (freddyb) - Saturday, 12 November 2011, 17:41 GMT+2
Bertrik, no.
Comment by Fred Bauer (freddyb) - Saturday, 12 November 2011, 20:38 GMT+2
Here are some of the examples of issues that I was working on beyond the initial report by Akio:

On the iPod Video simulator: creating dircache after boot leaves a hole of ~6 megabytes for ~8000 files. Disabling the dircache after that leaves a hole of about 7 megs.

On the Sansa Fuzev1 target: First off, dircache has to be modified to work at all because audio_buffer_available gives a number about 256k too big to allocate. After that, dircache after boot leaves a hole of ~3.7 megs for ~7500 files. Disabling dircache after that leaves a hole of ~4.1 megs. That leaves about 930k for audio buffer which is just barely enough for the pcm buffer (i.e. almost nothing for file buffer). Inserting or removing cards causes a complete rebuild of the cache which fails.

I really think we need to force recompaction after freeing big hunks of memory as dircache does.
Comment by Thomas Martitz (kugel.) - Sunday, 13 November 2011, 18:05 GMT+2
Right, the 256K need to be considered in audio_buffer_available().

I'm preparing a patch that makes core_available() smarter, to count free space before the allocation end (and return the best contiguous size, if there's any unmovable alloc). I don't want to just compact in that function, I would rather keep that compaction can only happen at alloc-time (although that's an implementation detail).

Comment by Fred Bauer (freddyb) - Sunday, 13 November 2011, 19:37 GMT+2
I know we're going to have to live with some inefficiency with a dynamic memory allocation system and we're going to waste ~100k now and then but I think a multiple megabyte hole is worth breaking some rules to patch up.
Comment by Thomas Martitz (kugel.) - Monday, 14 November 2011, 10:55 GMT+2
Okay, after thinking about this a bit more I think several steps should be done.

1. Fix audio_buffer_available() to consider the 256K reserve buffer (trivial).
2. Make buflib_compact_and_shrink() smarter in that it considers free space before or after the alloc to be shrinked before shrinking, so that if something allocs 3MB and 2.5MB is free before the audio buffer only 0.5MB will be taken from it (implemented that already).
3. Make buflib_available() smarter to return the best contiguous free space, and not only the one at the end (I'm now considering doing a compaction here, since that'll simplify finding best, as it makes holes as contiguous as possible). Shrinkable allocs not going to be considered here, as it's not currently possible to peek what they are able to give. So this would return the maximum size that's guaranteed to be allocatable (again, without considering shrinking).
4. Make playback.c reclaim unused space on rebuffering or other times where it needn't to stop playback. I've done that a while ago locally but it's not as trivial as I thought. Will pick up the patch, hopefully.

What will still be problematic is how to make it possible to alloc the maximum buffer available (including parts from the audio buffer). Buflib can't peek into the audio buffer to see what can be shrinked, and playback can't look into buflib to see what's free right before its buffer. I tend to think an approach of trial and error (e.g try to allocate a lot, and retry with less until it succeeds) could work, however I would like to avoid it since means to stop playback more often.
Comment by Fred Bauer (freddyb) - Monday, 14 November 2011, 15:16 GMT+2
Why is all this any better than the patch I submitted? The net result is still going to be leaving memory unused after a buflib free/shrink. If you're really going to do all the work to make things 'right' it would be better spent on breaking up the filebuffer into equal blocks that can be moved around cheaply.
Comment by Thomas Martitz (kugel.) - Monday, 14 November 2011, 15:42 GMT+2
It'd suck to require all buflib users to be aware of the audio buffer. dircache isn't the only problem, I want to enable dynamic toggling of tagcache ramcache as well. Parsing skins perhaps too. I.e. all buffers that are not known exactly beforehand. And it introduces an unnecessary playback start/stop cycle.

Point 4) makes sure no memory is left unused.
Comment by Fred Bauer (freddyb) - Monday, 14 November 2011, 16:08 GMT+2
My patch does not create or expand the dependence on the knowing the size of the audio buffer; it actually makes it trivial to eliminate it. The playback engine already goes through a start and stop cycle for every core_alloc() call when the memory is compact if I understand it correctly.
Comment by Thomas Martitz (kugel.) - Monday, 14 November 2011, 16:11 GMT+2
I was referring to the audio_realloc() thing. With my proposal (4) the audio buffer would itself reclaim unused space without any other module needing to take explicit action.
Comment by Fred Bauer (freddyb) - Monday, 14 November 2011, 19:39 GMT+2
When you fix audio_buffer_available() don't forget to allow for alignment padding.
Comment by Thomas Martitz (kugel.) - Tuesday, 15 November 2011, 18:02 GMT+2
This implements 1 to 3 of my proposal. This should fix the core issues of this task.

For 4 I'm still exploring the best solution (so that reclaiming doesnt need stopping playback). Implementing that is not critical yet so I'm doing it separately.
   0001-Buflib-Make-shrinking-and-buflib_available-s... (10.9 KiB) 
 b/apps/menus/main_menu.c  |    4 +-
 b/apps/mpeg.c             |   29 ++++++++++++++-----
 b/apps/playback.c         |   19 ++++++++++--
 b/firmware/buflib.c       |   68 +++++++++++++++++++++++++++++++++++++++++-----
 b/firmware/export/audio.h |    4 ++
 5 files changed, 104 insertions(+), 20 deletions(-)

Comment by Fred Bauer (freddyb) - Thursday, 17 November 2011, 13:33 GMT+2
Isn't #2 a highly specialized case? Wouldn't it just be easier to make the audio buffer mobile? It seems to get completely emptied and reloaded on every allocation, anyway.
Comment by Thomas Martitz (kugel.) - Thursday, 17 November 2011, 13:39 GMT+2
As you can see, 2) is 10 LOC. Making the audio buffer movable is not that easy, it's not easy at all.

The entire mechanism to shrink a buffer is a special case.

Did you gave that patch a test?
Comment by Fred Bauer (freddyb) - Thursday, 17 November 2011, 15:10 GMT+2
Yes, the patch ran ok for everything I tried.
Comment by Thomas Martitz (kugel.) - Thursday, 17 November 2011, 18:56 GMT+2
I'm going closing this. Let's talk about how audiobuf can use unused memory in another task.

What about your dircache move callback fixes? will you commit them?
Comment by Fred Bauer (freddyb) - Thursday, 17 November 2011, 19:30 GMT+2
Ok. I just want to be clear, you have no objection to the callback fixes, right?
Comment by Thomas Martitz (kugel.) - Thursday, 17 November 2011, 21:01 GMT+2
No, they should be alright. Perhaps use the macros I added for the pointer arithmetic (or follow the union-with-char style of dircache), but they're fine in general. Thanks for catching these.

Why should I object?
Comment by Fred Bauer (freddyb) - Friday, 18 November 2011, 23:34 GMT+2
Here's the patch for just the dircache buflib move callback. I simplified the input address check to just zero or not on the suspicion that the pointers involved either either point to NULL or inside the buflib allocation. I can't test this right now because SVN simulator is crashing for me. I'll work on this some more tomorrow.
   fixdircachecallback.patch (1.3 KiB) 
 firmware/common/dircache.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Comment by Thomas Martitz (kugel.) - Tuesday, 22 November 2011, 13:22 GMT+2
Looks good.
Comment by Igor Milhit (iGor) - Wednesday, 16 May 2012, 11:10 GMT+2
I'm encountering a similar problem with my SansaFuze+, when I'm starting :
*PANIC*
audio_reset_buffer_noalloc() : EOM (655 36 > 0)
and the screen is stucked there. I'm only able to shutdown, to start with the OF, that's all.
I'm not able to go to the menu. Somebody know what I could do ?
Thanks.

Loading...