[RFC] allocator: unify alloc API by RanderWang · Pull Request #1155 · thesofproject/sof

RanderWang · 2019-03-20T08:46:54Z

Rmalloc and rzalloc would waste much memory for allocation of big
chunk of memory. For example, if allocation size of 600 is requested,
memory sized 1024 would be allocated.Now refine the algorithm: First
remove the rballoc function and user should call rmalloc and rzalloc to
allocate memory. Second, if the allocation size > HEAP_BUFFER_SIZE,
it would be allocated from buffer heap, or it would be done as usual. And
if it is not supported by system heap, rt heap, it would be allocated from
buffer heap.

The idea is derived from #1128.

This fixes #1128

jajanusz

You don't need [RFC] tag in commit messages, it's good enough to have it in PR title

jajanusz · 2019-03-20T09:58:50Z

Can this name be less generic ? To indicate it's related to HEAP, or even just for non-rballoc

@RanderWang This will be platform and heap config specific. Use the buffer heap block size here.

HEAP_BUFFER_BLOCK_SIZE

You should also remodel the heap configs and remove any block sizes above HEAP_BUFFER_BLOCK_SIZE e.g.

/* Heap section sizes for module pool */ #define HEAP_RT_COUNT8 0 #define HEAP_RT_COUNT16 48 #define HEAP_RT_COUNT32 48 #define HEAP_RT_COUNT64 32 #define HEAP_RT_COUNT128 32 #define HEAP_RT_COUNT256 32 #define HEAP_RT_COUNT512 4 << remove since > than buffer block size. #define HEAP_RT_COUNT1024 4 << remove

Removing the large block frees up 6k which can either be added to this heap as smaller blocks or automatically to the buffer heap. I would add a 48, 96, 192 block sizes with the extra 6k as this makes the memory more granular.

/* Heap section sizes for system runtime heap */ #define HEAP_SYS_RT_COUNT64 64 #define HEAP_SYS_RT_COUNT512 8 << remove #define HEAP_SYS_RT_COUNT1024 4 << remove

This saves 8k, which again can be reallocated where best needed.

@lgirdwood Now the algorithm is:

if (size >= 384) rballoc else _malloc

So HEAP_RT_COUNT384 should be added to make HEAP_RT to support allocation size < 384 && size > 256

@lgirdwood , I find that cache line size is 64byte in xtensa core, I would drop 48 and 96

lgirdwood · 2019-03-20T16:03:42Z

The intention is that rballoc will just be static.

Thanks, I will update it

lgirdwood · 2019-03-20T16:19:47Z

@RanderWang This will be platform and heap config specific. Use the buffer heap block size here.

HEAP_BUFFER_BLOCK_SIZE

You should also remodel the heap configs and remove any block sizes above HEAP_BUFFER_BLOCK_SIZE e.g.

/* Heap section sizes for module pool */ #define HEAP_RT_COUNT8 0 #define HEAP_RT_COUNT16 48 #define HEAP_RT_COUNT32 48 #define HEAP_RT_COUNT64 32 #define HEAP_RT_COUNT128 32 #define HEAP_RT_COUNT256 32 #define HEAP_RT_COUNT512 4 << remove since > than buffer block size. #define HEAP_RT_COUNT1024 4 << remove

Removing the large block frees up 6k which can either be added to this heap as smaller blocks or automatically to the buffer heap. I would add a 48, 96, 192 block sizes with the extra 6k as this makes the memory more granular.

/* Heap section sizes for system runtime heap */ #define HEAP_SYS_RT_COUNT64 64 #define HEAP_SYS_RT_COUNT512 8 << remove #define HEAP_SYS_RT_COUNT1024 4 << remove

This saves 8k, which again can be reallocated where best needed.

RanderWang · 2019-03-21T05:24:03Z

You don't need [RFC] tag in commit messages, it's good enough to have it in PR title

Thanks! I will update my PR

RanderWang · 2019-03-21T09:34:12Z

update PR.
(1) add block size of 192 and 384
(2) refine block size according to cache line size, delete small block of 16, 32
(3) change _balloc, _malloc and _zalloc to static function

RanderWang · 2019-03-21T09:59:01Z

Sorry, I will refine code for host build tomorrow.

lgirdwood

I know 48, 96 is not aligned on a single cache line, but we will waste a lot of memory on systems like Baytrail that have very limited DRAM.

lgirdwood · 2019-03-21T16:13:59Z

Why are you adding 384 when it's the buffer block size ?

Now the algorithm is:

if (size >= 384) ptr = rballoc else ptr = _malloc if (!ptr) ptr = rballoc

Without 384 block, for the allocation 256 < size < 384, it can't be supported by _malloc but wastes cpu time in _malloc. So I add this to fix it

There is opportunity now (since we have save memory by deleting 512 and 1024 blocks) to add more step sizes here. You could also add 320 block size. It does need a macro though for your largest block size before rballoc blocks for the logic statement above.

RanderWang · 2019-03-22T07:32:58Z

I know 48, 96 is not aligned on a single cache line, but we will waste a lot of memory on systems like Baytrail that have very limited DRAM.

Yes, I will refine the block size on BYT and BDW

RanderWang · 2019-03-22T09:00:10Z

update my PR
(1) refine the definition of alloc api
(2) refine the block size on BYT and HSW

Thanks!

lgirdwood · 2019-03-22T10:21:24Z

Best to just return _balloc(zone, caps, bytes) here as it saves the extra indentation below.

lgirdwood · 2019-03-22T10:25:03Z

There is opportunity now (since we have save memory by deleting 512 and 1024 blocks) to add more step sizes here. You could also add 320 block size. It does need a macro though for your largest block size before rballoc blocks for the logic statement above.

lgirdwood · 2019-03-22T10:25:45Z

Why are you making block 256 count smaller ?

@lgirdwood This is the summary of allocation on BYT with nocodec. So I decreased the size of 256. Do you prefer to change the block number of other size ?

841 heap: runtime status 815 heap: 0xff317080 size 17664 blocks 8 caps 0x61 817 used 8704 free 8960 825 block 1 base 0xff317380 size 32 828 count 48 free 48 825 block 2 base 0xff317980 size 64 828 count 32 free 32 825 block 3 base 0xff318180 size 128 828 count 16 free 3 825 block 4 base 0xff318980 size 192 828 count 16 free 5 825 block 5 base 0xff319580 size 256 828 count 24 free 8 825 block 6 base 0xff31ad80 size 320 828 count 2 free 1 825 block 7 base 0xff31b280 size 384 828 count 2 free 2

@RanderWang why do you have blocks defined for 384 ? when this is the buffer block size ? i.e. it's not needed as code will use buffer heap here.

@lgirdwood we have got agreement at this 4 days ago, right ?

lgirdwood 4 days ago Member Why are you adding 384 when it's the buffer block size ? @RanderWang RanderWang 4 days ago • Author Member Now the algorithm is: if (size >= 384) ptr = rballoc else ptr = _malloc if (!ptr) ptr = rballoc Without 384 block, for the allocation 256 < size < 384, it can't be supported by _malloc but wastes cpu time in _malloc. So I add this to fix it @lgirdwood lgirdwood 4 days ago Member There is opportunity now (since we have save memory by deleting 512 and 1024 blocks) to add more step sizes here. You could also add 320 block size. It does need a macro though for your largest block size before rballoc blocks for the logic statement above.

@RanderWang I mean you dont need to add any blocks of size 384 to the non buffer heap map since all blocks of 384 size will be in buffer heap.

@lgirdwood yes, but I talk about allocation size (.eg 360 )< 384 and > 320. For this kind of allocation, It will be not allocated from buffer heap but it is not supported by other non buffer heap without 384 included. block of sized 384 is for allocation size < 384 && > 320

The idea is there is a gap between 320 and 384

--------------- ----------------------- 320 384

ok, now I see to fill this gap, but it would be better to remove

#define HEAP_RT_LP_COUNT384 4

and catch all allocations above the largest block size (in this case 320) to us the buffer heap.

I checked there is no free block of 384 sized on apollolake and whiskylake, icelake, so I will remove 384 on Baytray and haswell only. Thanks!

@RanderWang have you pushed the updated branch, I'm still seeing block size 384 for some heaps (not all)

RanderWang · 2019-03-25T08:19:16Z

update my PR
(1) add 320 sized block
(2) refine rballoc

RanderWang · 2019-03-28T08:55:09Z

update my PR. Remove 384 sized block on baytrial and haswell

lgirdwood · 2019-03-28T15:20:17Z

@RanderWang have you pushed the updated branch, I'm still seeing block size 384 for some heaps (not all)

lgirdwood · 2019-04-19T19:57:44Z

@tlauda I was hoping to wait til after v1.3, but due to IPC issue we can merge when conflicts are resolved. @RanderWang

Rmalloc and rzalloc would waste much memory for allocation of big chunk of memory. Now refine the algorithm: First remove the rballoc function and user should call rmalloc and rzalloc to allocate memory. Second, if the allocation size > HEAP_BUFFER_SIZE, it would be allocated from buffer heap, or it would be done as usual. And if it is not supported by system heap, rt heap, it would be allocated from buffer heap. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

Change block size of 512 and 1024 to 192 and 320 to save memory. For APL and later chips, cache line size is 64byte in xtensa core. For BYT and HSW, cpu cache is not supported. This PR refines the block size to be aligned to cache line size on supported chips. Now sys_runtime and runtime heap don't support allcation size bigger than 320. Buffer heap would support it. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

RanderWang · 2019-04-22T08:04:52Z

@tlauda I rebased my PR and get one CI failed. I checked the log but didn't find any hint. Could do you help to check it ? thanks!

tlauda · 2019-04-22T08:44:18Z

@zrombel Could you take a look?

lgirdwood · 2019-05-06T13:30:21Z

@RanderWang this is pending for 1.4, but in the meantime it's conflicting

zrombel · 2019-05-06T15:37:02Z

This PR is not ready for merging. PR is causing DSP panic in random python tests on all platforms especially in tests:
TestPtpSimpleStreamOperations, TestPtpParallelExternal and DMIC tests.

RanderWang · 2019-05-09T01:42:17Z

ok, I will test and refine it after 1.3 released

dabekjakub · 2019-05-16T14:01:10Z

-		  TEST_BULK, "rballoc_dma"),
-	TEST_CASE(256, RZONE_BUFFER, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA, 2,
-		  TEST_BULK, "rballoc_dma"),
-	TEST_CASE(2048, RZONE_BUFFER, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA, 100,


We cannot just delete those - marked test case is crucial for finding if we free memory properly. So they need to be converted to new memory allocator functions.

tlauda · 2019-05-30T09:28:54Z

@RanderWang Is there any ETA for your changes?

RanderWang · 2019-06-04T08:23:24Z

@RanderWang Is there any ETA for your changes?

@tlauda This PR is for sof 1.4. Now, as you know, we are busy at production release for HDA.
I will work on it after sof 1.3 released. If you have any advice, please share it.
Thanks!

tlauda · 2019-06-05T19:20:14Z

 	uint32_t flags;
 	void *ptr = NULL;

+	if (bytes >= HEAP_BUFFER_BLOCK_SIZE)


Looks like RZONE_BUFFER isn't used anymore.
Shouldn't it be used to take priority in allocation?
Some buffers explicitly want to be allocated directly on buffer heap and not on other heaps and some want to be allocated on system heaps. I don't see current code working with other cores running, where every core has its own system and system runtime heaps.
Also I think the current code will panic if you try to allocate buffer with bytes < HEAP_BUFFER_BLOCK_SIZE and zone RZONE_BUFFER.

tlauda · 2019-06-05T19:25:02Z

 		break;
 	}

+	spin_unlock_irq(&memmap.lock, flags);


Here you are unlocking before another try to alloc.
It can possibly create race condition.

tlauda · 2019-06-05T19:38:25Z

-		  TEST_BULK, "rballoc_dma"),
-	TEST_CASE(256, RZONE_BUFFER, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA, 2,
-		  TEST_BULK, "rballoc_dma"),
-	TEST_CASE(2048, RZONE_BUFFER, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA, 100,


tlauda · 2019-06-05T19:39:42Z

 #define HEAP_BUFFER_BLOCK_SIZE	0x180
 #define HEAP_BUFFER_COUNT	(HEAP_BUFFER_SIZE / HEAP_BUFFER_BLOCK_SIZE)

+#define PLATFORM_BUFFER_BLOCK_SIZE (HEAP_BUFFER_BLOCK_SIZE - 64)


I don't understand this.

@tlauda , please ignore this for I may rework the PR since The memory structure is significant different with the one on which I made this PR based

lgirdwood · 2019-06-05T21:20:03Z

@RanderWang can you revisit when your done with IPC.

RanderWang · 2019-06-12T05:56:27Z

@RanderWang can you revisit when your done with IPC.

yes, I think I have to rework it for commit #1483 (comment). The memory structure is significant different with the one on which I made this PR based.
Maybe this PR is not needed.

I will talk with @mengdonglin about the schedule.

lgirdwood · 2019-06-12T12:38:50Z

@RanderWang still needed as it makes the map more granular and simplifies usage.

RanderWang · 2019-07-09T03:14:44Z

The PR: #1567 is linked to my PR.
since it removes the hp buffer heap, I need to rework my PR.

lgirdwood · 2019-07-25T14:42:17Z

@RanderWang yep, please make sure it gets good QA coverage too.

RanderWang · 2019-07-26T07:10:39Z

@RanderWang yep, please make sure it gets good QA coverage too.

yes, I get it.

lgirdwood · 2019-07-29T13:02:46Z

@mengdonglin this needs QA as its for v1.4

tlauda · 2019-10-04T12:41:51Z

@RanderWang Ping.

RanderWang · 2019-10-08T07:07:40Z

@RanderWang Ping.

hi @mengdonglin, I will talk with you about this task.

mengdonglin · 2019-10-12T07:53:58Z

@lrgirdwo @tlauda Rander is working on SoundWire atm, do you see if someone else can take over and improve this PR? Thanks!

RanderWang · 2019-10-15T02:04:33Z

There are many changes in memory management since my first commit for this PR, so it will take great effort to redesign and validate the PR. Now I am occupied by soundwire project which is a high priority task and this is a low priority task, so I just close it and reopen it when I have time to implement it.

RanderWang requested review from lbetlej, lgirdwood, libinyang, mmaka1, plbossart and xiulipan as code owners March 20, 2019 08:46

RanderWang changed the title ~~allocator: unify alloc API~~ [RFC] allocator: unify alloc API Mar 20, 2019

jajanusz reviewed Mar 20, 2019

View reviewed changes

lgirdwood requested changes Mar 20, 2019

View reviewed changes

RanderWang force-pushed the memory_unify branch from cd124a4 to c59e05f Compare March 21, 2019 09:26

RanderWang requested review from mrajwa and singalsu as code owners March 21, 2019 09:26

RanderWang force-pushed the memory_unify branch from c59e05f to 0fb68a1 Compare March 21, 2019 09:47

lgirdwood requested changes Mar 21, 2019

View reviewed changes

RanderWang force-pushed the memory_unify branch from 0fb68a1 to 6561b30 Compare March 22, 2019 08:36

RanderWang requested a review from ranj063 as a code owner March 22, 2019 08:36

lgirdwood requested changes Mar 22, 2019

View reviewed changes

RanderWang force-pushed the memory_unify branch from 6561b30 to a140070 Compare March 25, 2019 07:37

RanderWang force-pushed the memory_unify branch from a140070 to 1bd0f4b Compare March 28, 2019 08:52

lgirdwood requested changes Mar 28, 2019

View reviewed changes

RanderWang force-pushed the memory_unify branch 3 times, most recently from b74faa5 to 3bbd6e5 Compare March 29, 2019 06:01

RanderWang force-pushed the memory_unify branch 2 times, most recently from d594b17 to d3d0068 Compare April 22, 2019 07:10

tlauda requested a review from lgirdwood May 6, 2019 09:55

dabekjakub requested changes May 17, 2019

View reviewed changes

tlauda suggested changes Jun 5, 2019

View reviewed changes

lgirdwood assigned mengdonglin Jul 29, 2019

lgirdwood modified the milestones: v1.4, v1.5 Aug 30, 2019

RanderWang closed this Oct 15, 2019

Conversation

RanderWang commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jajanusz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Mar 21, 2019

Uh oh!

RanderWang commented Mar 21, 2019

Uh oh!

RanderWang commented Mar 21, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Mar 22, 2019

Uh oh!

RanderWang commented Mar 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Mar 25, 2019

Uh oh!

RanderWang commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Apr 19, 2019

RanderWang commented Mar 20, 2019 •

edited

Loading

RanderWang Mar 22, 2019 •

edited

Loading

RanderWang Mar 25, 2019 •

edited

Loading

RanderWang Mar 27, 2019 •

edited

Loading

RanderWang commented Jul 9, 2019 •

edited

Loading