[Do Not Review] Secondary core tearing down series #4718

keyonjie · 2021-09-06T14:53:37Z

unregister/free schedulers, IDCs, for DSP core tearing down gracefully.

gkbldcig · 2021-09-08T01:27:00Z

Can one of the admins verify this patch?

lgirdwood · 2021-10-19T13:06:13Z

src/schedule/ll_schedule.c

This change looks like a rename and only part of it is needed ?

lgirdwood · 2021-10-19T13:06:22Z

src/include/sof/schedule/schedule.h

How do we do this per core ? What's the flow ?

lgirdwood · 2021-10-19T13:06:55Z

src/schedule/ll_schedule.c

What about EDF scheduler ?

lgirdwood · 2021-10-19T13:14:51Z

src/drivers/intel/cavs/idc.c

This should probably be renamed to reflect the use cases i.e. we have

power down the core (with memories off i.e. S4)

power down the core (with memories on i.e. S3)
The flows will also be very similar for both 1 & 2 and diverge at some point, so this could be

idc_core_power_down();

which in turn would call a generic

core_power_down(int pm_flags);

ranj063 · 2021-10-19T15:33:29Z

src/include/sof/schedule/schedule.h

@keyonjie we already call schedule_free() in cpu_power_down_core(). Is this really ncessary on tpo of that?

Hi @ranj063 , yes, we already call schedule_free(), the problem is that neither the generic schedule_data struct nor the scheduler specific struct (e.g. edf_schedule_data and ll_schedule_data) is freed. Here the PR is for fixing the memory freeing. This will make sense in case of only the secondary cores are in loops of power off <--> on, e.g. dynamic pipelines run on a secondary core.

@keyonjie do you have an issue to show the behaviour? I have run secondary core power on/off (all 4 cores) loops for 10000 cycles without any issues. I added a debugfs entry in the kernel and wrote a script to test it. Have yyou done something similar and noticed issues?

@keyonjie do you have an issue to show the behaviour? I have run secondary core power on/off (all 4 cores) loops for 10000 cycles without any issues. I added a debugfs entry in the kernel and wrote a script to test it. Have yyou done something similar and noticed issues?

Thanks, @ranj063 I didn't run this kind of test, can you share your patch to test it? Your comment below about the "free_heap(SOF_MEM_ZONE_SYS)" explains why we are not seeing system memory leak.

I would still suggest this change as it is a graceful and symmetric way to tearing down each modules, it can make sure each allocated buffer is freed and writeback/invalidated, no matter it is located in system or some other zone in future, we should not rely on the powerful free_heap() and dcache_writeback_invalidate_all().

ranj063 · 2021-10-19T15:34:09Z

src/drivers/intel/cavs/idc.c

same here. idc_free() is also called from cpu_power_down_core()

same. the original idc_free() is lack of freeing of struct idc and its payload, they are allocated in generic src/idc/idc.c so add the freeing to there also, and the platform specific platform_idc_free() only do the register clearing and interrupt unregistering stuff.

ranj063 · 2021-10-19T15:35:00Z

@keyonjie I think the main problem is with the shutting down of the primary core. The secondary core power down sequence is actually quite rock solid.

ranj063 · 2021-10-20T16:35:26Z

@keyonjie I'm still unsure about the need for this change. If this series is related to your changes in #4747 , you need to mention it. AFAICT, looking at free_heap(SOF_MEM_ZONE_SYS); in cpu_power_down_core(), frees the entire sys zone for the secondary cores. But if your change is needed because you are getting rid of the mem zones, then this change needs to be folded into the other PR where you do that and in a way that you don't break bisect. For ex: freeing the sys zone today will lead to a pnic. So you need to remove the panic first, free the zone and the add the changes in to remove the zone. It is very hard to review these patches otherwise

To power down a secondary core safely, we need to unregister and free the schedulers that run on the corresponding core cleanly, which will help to create clean one on the next powering up to the core. Move the scheduler data allocation to SYSTEM_RUNTIME zone, and free it at core powering down. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Add scheuler free support to align to multi-core cAVS platform implementation. We don't need explicit freeing in tb_pipeline_free() anymore. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

To power down a secondary core gracefully, we need to free the IDC related memory used on the corresponding core cleanly, which will help to create clean one on the next powering up to the core. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

To power down a secondary core gracefully, we need to free the notifier related memory used on the corresponding core cleanly, which will help to create clean one on the next powering up to the core. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

abonislawski · 2021-10-22T09:00:45Z

I can agree too that this is not needed and probably wrong.

Currently we are not freeing such things because they are not designed to free:

System Zone. Fixed size heap where alloc always succeeds and is never
freed. Used by any init code that will never give up the memory.

Thats why you have changed them to SYS_RUNTIME, but are they? We will not alloc idc again in runtime.

So we will abuse mem zone design only to get something like free(1) free(2) and anyway free(all)

keyonjie · 2021-10-22T09:12:07Z

@abonislawski @ranj063 thanks for review, looks this doesn't fix any secondary core power down/on issues although no side effect observed either. Let me put it away at the moment.

keyonjie · 2021-11-22T09:28:07Z

closing this as it doesn't look needed.

keyonjie requested review from RanderWang, akloniex, bardliao, dbaluta, lbetlej, lgirdwood, libinyang, lyakh, mmaka1, mrajwa, plbossart and ranj063 as code owners September 6, 2021 14:53

keyonjie marked this pull request as draft September 6, 2021 14:53

keyonjie force-pushed the multi_core branch from 878d3f4 to 696648f Compare September 8, 2021 05:08

keyonjie force-pushed the multi_core branch 2 times, most recently from 1f49a53 to 73fc2b1 Compare October 18, 2021 09:03

keyonjie force-pushed the multi_core branch 2 times, most recently from 7c7669a to 99c153e Compare October 19, 2021 09:59

This was referenced Oct 19, 2021

edf_schedule: don't assert if there is no extra task to run #4874

Merged

lps: restore secondary cores on wake up #4839

Merged

keyonjie force-pushed the multi_core branch 2 times, most recently from 4105535 to 5cea70f Compare October 19, 2021 12:08

lgirdwood reviewed Oct 19, 2021

View reviewed changes

keyonjie changed the title ~~[DO NOT REVIEW][For Test Only] Multi-core test only~~ Secondary core tearing down series Oct 19, 2021

keyonjie marked this pull request as ready for review October 19, 2021 14:08

ranj063 reviewed Oct 19, 2021

View reviewed changes

keyonjie force-pushed the multi_core branch from 5cea70f to bf5c110 Compare October 20, 2021 05:06

keyonjie force-pushed the multi_core branch 2 times, most recently from d484277 to 5745c64 Compare October 21, 2021 08:47

keyonjie added 4 commits October 21, 2021 22:20

testbench: scheduler: add scheduler free support

046992f

Add scheuler free support to align to multi-core cAVS platform implementation. We don't need explicit freeing in tb_pipeline_free() anymore. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

idc: add idc free support

5fc2b97

To power down a secondary core gracefully, we need to free the IDC related memory used on the corresponding core cleanly, which will help to create clean one on the next powering up to the core. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie force-pushed the multi_core branch from 5745c64 to 485653c Compare October 21, 2021 14:21

keyonjie marked this pull request as draft October 22, 2021 09:12

keyonjie changed the title ~~Secondary core tearing down series~~ [Do Not Review] Secondary core tearing down series Oct 22, 2021

keyonjie closed this Nov 22, 2021

[Do Not Review] Secondary core tearing down series #4718

[Do Not Review] Secondary core tearing down series #4718

Uh oh!

Conversation

keyonjie commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkbldcig commented Sep 8, 2021

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!

keyonjie Oct 20, 2021 • 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!

ranj063 commented Oct 19, 2021

Uh oh!

ranj063 commented Oct 20, 2021

Uh oh!

abonislawski commented Oct 22, 2021

Uh oh!

keyonjie commented Oct 22, 2021

Uh oh!

keyonjie commented Nov 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

keyonjie commented Sep 6, 2021 •

edited

Loading

keyonjie Oct 20, 2021 •

edited

Loading