• Status Closed
  • Percent Complete
  • Task Type Bugs
  • Category Operating System/Drivers
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by idak - 2011-10-22
Last edited by saratoga - 2012-11-04

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

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)

Closed by  saratoga
2012-11-04 17:38
Reason for closing:  Fixed
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

Looks like this is fixed and just left open. If i'm wrong, please reopen.

I messed up r30818 and reverted it. Do you still have the problem with current SVN?

idak commented on 2011-10-22 11:44

This bug still occurs in current SVN (r30824).
I bisecting this bug and I found this bug was introduced in r 30393.

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.

idak commented on 2011-11-08 14:31

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.”)

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?

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.

idak commented on 2011-11-09 14:48

Applying 5.debug.patch, and
Splash message is “dircache memory allocation failed”

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

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

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?

idak commented on 2011-11-09 16:41

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;revision=30900

Here’s an update to try to restore playback using the same code from the playback.c buflib move callback.

Oops. Posted old version before.

idak commented on 2011-11-11 23:45

I think this is perfect!
Thank you very much for all your hard work.

You’re welcome!

I guess you don’t need/intended the printf in io.c

I would rather not see this committed. I can image a few better solutions.

Bertrik, no.

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.

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).

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.

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.

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.

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.

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.

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.

When you fix audio_buffer_available() don’t forget to allow for alignment padding.

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.

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.

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?

Yes, the patch ran ok for everything I tried.

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?

Ok. I just want to be clear, you have no objection to the callback fixes, right?

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?

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.

Looks good.

iGor commented on 2012-05-16 09:10

I’m encountering a similar problem with my SansaFuze+, when I’m starting :
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 ?


Available keyboard shortcuts


Task Details

Task Editing