Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
keyonjie wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6928679
scheduler: add scheduler free support
keyonjie Oct 12, 2021
046992f
testbench: scheduler: add scheduler free support
keyonjie Oct 19, 2021
5fc2b97
idc: add idc free support
keyonjie Oct 19, 2021
485653c
notifier: add notifier free support
keyonjie Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/drivers/intel/cavs/idc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ int platform_idc_init(void)
/**
* \brief Frees IDC data and unregisters interrupt.
*/
void idc_free(void)
void platform_idc_free(void)
Copy link
Member

@lgirdwood lgirdwood Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. power down the core (with memories off i.e. S4)
  2. 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);

Copy link
Collaborator

@ranj063 ranj063 Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@keyonjie keyonjie Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
struct idc *idc = *idc_get();
int core = cpu_get_id();
Expand All @@ -265,6 +265,4 @@ void idc_free(void)
if (idctfc & IPC_IDCTFC_BUSY)
idc_write(IPC_IDCTFC(i), core, idctfc);
}

schedule_task_free(&idc->idc_task);
}
19 changes: 18 additions & 1 deletion src/idc/idc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ int idc_init(void)
tr_info(&idc_tr, "idc_init()");

/* initialize idc data */
*idc = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM, sizeof(**idc));
*idc = rzalloc(SOF_MEM_ZONE_SYS_RUNTIME, 0, SOF_MEM_CAPS_RAM, sizeof(**idc));
(*idc)->payload = cache_to_uncache((struct idc_payload *)static_payload);

/* process task */
Expand All @@ -336,3 +336,20 @@ int idc_init(void)
return 0;
#endif
}

/**
* \brief Frees IDC data for current core.
*/
void idc_free(void)
{
struct idc **idc = idc_get();

tr_info(&idc_tr, "idc_free()");

#ifndef __ZEPHYR__
platform_idc_free();
schedule_task_free(&(*idc)->idc_task);
#endif

rfree(*idc);
}
2 changes: 1 addition & 1 deletion src/include/sof/drivers/idc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ static inline struct idc_payload *idc_payload_get(struct idc *idc,

void idc_enable_interrupts(int target_core, int source_core);

void idc_free(void);
void platform_idc_free(void);

int platform_idc_init(void);

Expand Down
8 changes: 7 additions & 1 deletion src/include/sof/schedule/schedule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ static inline int schedule_task_free(struct task *task)
return -ENODEV;
}

/** See scheduler_ops::scheduler_free */
/** See scheduler_ops::scheduler_free, free all schedulers belong to the core */
static inline void schedule_free(void)
{
struct schedulers *schedulers = *arch_schedulers_get();
Expand Down Expand Up @@ -311,6 +311,12 @@ int schedule_task_init(struct task *task,
*/
void scheduler_init(int type, const struct scheduler_ops *ops, void *data);

/**
* freeing generic resource of a scheduler
* @param data Scheduler's private data.
*/
void scheduler_free(void *data);
Copy link
Member

@lgirdwood lgirdwood Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@ranj063 ranj063 Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@keyonjie keyonjie Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@ranj063 ranj063 Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@keyonjie keyonjie Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** @}*/

#endif /* __SOF_SCHEDULE_SCHEDULE_H__ */
5 changes: 4 additions & 1 deletion src/lib/notifier.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ void init_system_notify(struct sof *sof)
{
struct notify **notify = arch_notify_get();
int i;
*notify = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM,
*notify = rzalloc(SOF_MEM_ZONE_SYS_RUNTIME, 0, SOF_MEM_CAPS_RAM,
sizeof(**notify));

spinlock_init(&(*notify)->lock);
Expand All @@ -203,4 +203,7 @@ void init_system_notify(struct sof *sof)

void free_system_notify(void)
{
struct notify **notify = arch_notify_get();

rfree(*notify);
}
4 changes: 4 additions & 0 deletions src/platform/intel/cavs/include/cavs/drivers/idc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,16 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode);

int idc_init(void);

void idc_free(void);

#else

static inline int idc_send_msg(struct idc_msg *msg, uint32_t mode) { return 0; }

static inline int idc_init(void) { return 0; }

static inline int idc_free(void) { return 0; }

#endif

#endif /* __CAVS_DRIVERS_IDC_H__ */
Expand Down
27 changes: 27 additions & 0 deletions src/platform/library/schedule/schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ static void scheduler_register(struct schedule_data *scheduler)
list_item_append(&scheduler->list, &(*sch)->list);
}

static void scheduler_unregister(struct schedule_data *scheduler)
{
struct schedulers **sch = arch_schedulers_get();

list_item_del(&scheduler->list);

if (list_is_empty(&(*sch)->list))
free(*sch);
}

void scheduler_init(int type, const struct scheduler_ops *ops, void *data)
{
struct schedule_data *sch;
Expand All @@ -65,3 +75,20 @@ void scheduler_init(int type, const struct scheduler_ops *ops, void *data)

scheduler_register(sch);
}

void scheduler_free(void *data)
{
struct schedulers **schedulers = arch_schedulers_get();
struct list_item *slist;
struct schedule_data *sch;

list_for_item(slist, &(*schedulers)->list) {
sch = container_of(slist, struct schedule_data, list);
if (sch->data == data) {
/* found the scheduler, free it */
scheduler_unregister(sch);
free(sch);
break;
}
}
}
8 changes: 7 additions & 1 deletion src/schedule/edf_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ int scheduler_init_edf(void)

tr_info(&edf_tr, "edf_scheduler_init()");

edf_sch = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM,
edf_sch = rzalloc(SOF_MEM_ZONE_SYS_RUNTIME, 0, SOF_MEM_CAPS_RAM,
sizeof(*edf_sch));
list_init(&edf_sch->list);
edf_sch->clock = PLATFORM_DEFAULT_CLOCK;
Expand Down Expand Up @@ -295,6 +295,12 @@ static void scheduler_free_edf(void *data)
/* free main task context */
task_main_free();

/* free the generic scheduler resource */
scheduler_free(edf_sch);

/* free edf_schedule_data */
rfree(edf_sch);

irq_local_enable(flags);
}

Expand Down
14 changes: 10 additions & 4 deletions src/schedule/ll_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,13 +573,19 @@ static int reschedule_ll_task(void *data, struct task *task, uint64_t start)

static void scheduler_free_ll(void *data)
{
struct ll_schedule_data *sch = data;
struct ll_schedule_data *ll_sch = data;
Copy link
Member

@lgirdwood lgirdwood Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about EDF scheduler ?

uint32_t flags;

irq_local_disable(flags);

notifier_unregister(sch, NULL,
NOTIFIER_CLK_CHANGE_ID(sch->domain->clk));
notifier_unregister(ll_sch, NULL,
NOTIFIER_CLK_CHANGE_ID(ll_sch->domain->clk));

/* free the generic scheduler resource */
scheduler_free(ll_sch);
Copy link
Member

@lgirdwood lgirdwood Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/* free ll_schedule_data */
rfree(ll_sch);

irq_local_enable(flags);
}
Expand Down Expand Up @@ -626,7 +632,7 @@ int scheduler_init_ll(struct ll_schedule_domain *domain)
struct ll_schedule_data *sch;

/* initialize scheduler private data */
sch = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM, sizeof(*sch));
sch = rzalloc(SOF_MEM_ZONE_SYS_RUNTIME, 0, SOF_MEM_CAPS_RAM, sizeof(*sch));
list_init(&sch->tasks);
atomic_init(&sch->num_tasks, 0);
sch->domain = domain;
Expand Down
30 changes: 28 additions & 2 deletions src/schedule/schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,23 +49,49 @@ static void scheduler_register(struct schedule_data *scheduler)

if (!*sch) {
/* init schedulers list */
*sch = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM,
*sch = rzalloc(SOF_MEM_ZONE_SYS_RUNTIME, 0, SOF_MEM_CAPS_RAM,
sizeof(**sch));
list_init(&(*sch)->list);
}

list_item_append(&scheduler->list, &(*sch)->list);
}

static void scheduler_unregister(struct schedule_data *scheduler)
{
struct schedulers **sch = arch_schedulers_get();

list_item_del(&scheduler->list);
if (list_is_empty(&(*sch)->list))
rfree(*sch);
}

void scheduler_init(int type, const struct scheduler_ops *ops, void *data)
{
struct schedule_data *sch;

sch = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM, sizeof(*sch));
sch = rzalloc(SOF_MEM_ZONE_SYS_RUNTIME, 0, SOF_MEM_CAPS_RAM, sizeof(*sch));
list_init(&sch->list);
sch->type = type;
sch->ops = ops;
sch->data = data;

scheduler_register(sch);
}

void scheduler_free(void *data)
{
struct schedulers **schedulers = arch_schedulers_get();
struct list_item *slist;
struct schedule_data *sch;

list_for_item(slist, &(*schedulers)->list) {
sch = container_of(slist, struct schedule_data, list);
if (sch->data == data) {
/* found the scheduler, free it */
scheduler_unregister(sch);
rfree(sch);
break;
}
}
}
9 changes: 0 additions & 9 deletions tools/testbench/common_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,6 @@ struct ipc_data {

void tb_pipeline_free(struct sof *sof)
{
struct schedule_data *sch;
struct schedulers **schedulers;
struct list_item *slist, *_slist;
struct notify **notify = arch_notify_get();
struct ipc_data *iipc;

Expand All @@ -73,12 +70,6 @@ void tb_pipeline_free(struct sof *sof)

/* free all scheduler data */
schedule_free();
schedulers = arch_schedulers_get();
list_for_item_safe(slist, _slist, &(*schedulers)->list) {
sch = container_of(slist, struct schedule_data, list);
free(sch);
}
free(*arch_schedulers_get());

/* free IPC data */
iipc = sof->ipc->private;
Expand Down