Skip to content
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

CHAIN_DMA: fix memory leaks #8767

Merged
merged 3 commits into from
Jan 24, 2024
Merged

CHAIN_DMA: fix memory leaks #8767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2ffc0b
ipc4: helper: fix memory leak with CHAIN_DMA
plbossart Jan 22, 2024
00ca2d3
chain_dma: fix obvious typo in CONFIG_XRUN_NOTIFICATIONS_ENABLE
plbossart Jan 22, 2024
7c48509
chain_dma: fix tentative memory leak
plbossart Jan 22, 2024
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
14 changes: 9 additions & 5 deletions src/audio/chain_dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include <sof/ut.h>
#include <zephyr/pm/policy.h>
#include <rtos/init.h>
#if CONFIG_IPC4_XRUN_NOTIFICATIONS_ENABLE
#if CONFIG_XRUN_NOTIFICATIONS_ENABLE
#include <ipc4/notification.h>
#include <sof/ipc/msg.h>
#include <ipc/header.h>
Expand Down Expand Up @@ -52,7 +52,7 @@ struct chain_dma_data {
enum sof_ipc_stream_direction stream_direction;
/* container size in bytes */
uint8_t cs;
#if CONFIG_IPC4_XRUN_NOTIFICATIONS_ENABLE
#if CONFIG_XRUN_NOTIFICATIONS_ENABLE
bool xrun_notification_sent;
struct ipc_msg *msg_xrun;
#endif
Expand Down Expand Up @@ -140,7 +140,7 @@ static size_t chain_get_transferred_data_size(const uint32_t out_read_pos, const
return buff_size - in_read_pos + out_read_pos;
}

#if CONFIG_IPC4_XRUN_NOTIFICATIONS_ENABLE
#if CONFIG_XRUN_NOTIFICATIONS_ENABLE
static void handle_xrun(struct chain_dma_data *cd)
{
int ret;
Expand Down Expand Up @@ -187,7 +187,7 @@ static enum task_state chain_task_run(void *data)
case -EPIPE:
tr_warn(&chain_dma_tr, "chain_task_run(): dma_get_status() link xrun occurred,"
" ret = %u", ret);
#if CONFIG_IPC4_XRUN_NOTIFICATIONS_ENABLE
#if CONFIG_XRUN_NOTIFICATIONS_ENABLE
handle_xrun(cd);
#endif
break;
Expand Down Expand Up @@ -660,7 +660,7 @@ static struct comp_dev *chain_task_create(const struct comp_driver *drv,
if (ret)
goto error_cd;

#if CONFIG_IPC4_XRUN_NOTIFICATIONS_ENABLE
#if CONFIG_XRUN_NOTIFICATIONS_ENABLE
cd->msg_xrun = ipc_msg_init(header.dat,
sizeof(struct ipc4_resource_event_data_notification));
if (!cd->msg_xrun)
Expand All @@ -681,6 +681,10 @@ static void chain_task_free(struct comp_dev *dev)
{
struct chain_dma_data *cd = comp_get_drvdata(dev);

#if CONFIG_XRUN_NOTIFICATIONS_ENABLE
ipc_msg_free(cd->msg_xrun);
#endif

chain_release(dev);
rfree(cd);
rfree(dev);
Expand Down
12 changes: 12 additions & 0 deletions src/ipc/ipc4/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,9 @@ int ipc4_chain_dma_state(struct comp_dev *dev, struct ipc4_chain_dma *cdma)
{
const bool allocate = cdma->primary.r.allocate;
const bool enable = cdma->primary.r.enable;
struct ipc *ipc = ipc_get();
struct ipc_comp_dev *icd;
struct list_item *clist, *_tmp;
int ret;

if (!dev)
Expand All @@ -767,6 +770,15 @@ int ipc4_chain_dma_state(struct comp_dev *dev, struct ipc4_chain_dma *cdma)
ret = comp_trigger(dev, COMP_TRIGGER_PAUSE);
if (ret < 0)
return ret;

list_for_item_safe(clist, _tmp, &ipc->comp_list) {
icd = container_of(clist, struct ipc_comp_dev, list);
if (icd->cd != dev)
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this would make the resulting code more optimal, but this could be

	list_for_item_safe(clist, _tmp, &ipc->comp_list) {
		if (clist != &dev->list)
			continue;
		icd = ...
		...

Copy link
Member Author

Choose a reason for hiding this comment

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

we could, but most uses of list_for_item_safe start with the container_of stuff.
I don't mind doing the change if that was the preference, I prefer code that looks familiar and consistent with other usages.

git grep -A3 list_for_item_safe
--
src/audio/module_adapter/module/generic.c:      list_for_item_safe(mem_list, _mem_list, &mod->priv.memory.mem_list) {
src/audio/module_adapter/module/generic.c-              mem = container_of(mem_list, struct module_memory, mem_list);
src/audio/module_adapter/module/generic.c-              if (mem->ptr == ptr) {
src/audio/module_adapter/module/generic.c-                      rfree(mem->ptr);
--
src/audio/module_adapter/module/generic.c:      list_for_item_safe(mem_list, _mem_list, &mod->priv.memory.mem_list) {
src/audio/module_adapter/module/generic.c-              mem = container_of(mem_list, struct module_memory, mem_list);
src/audio/module_adapter/module/generic.c-              rfree(mem->ptr);
src/audio/module_adapter/module/generic.c-              list_item_del(&mem->mem_list);
--
src/audio/module_adapter/module_adapter.c:      list_for_item_safe(dp_queue_list_item, tmp, &mod->dp_queue_dp_to_ll_list) {
src/audio/module_adapter/module_adapter.c-              struct dp_queue *dp_queue =
src/audio/module_adapter/module_adapter.c-                      container_of(dp_queue_list_item, struct dp_queue, list);
src/audio/module_adapter/module_adapter.c-
--
src/audio/module_adapter/module_adapter.c:      list_for_item_safe(dp_queue_list_item, tmp, &mod->dp_queue_ll_to_dp_list) {
src/audio/module_adapter/module_adapter.c-              struct dp_queue *dp_queue =
src/audio/module_adapter/module_adapter.c-                      container_of(dp_queue_list_item, struct dp_queue, list);
src/audio/module_adapter/module_adapter.c-
--
src/audio/module_adapter/module_adapter.c:      list_for_item_safe(blist, _blist, &mod->sink_buffer_list) {
src/audio/module_adapter/module_adapter.c-              struct comp_buffer *buffer = container_of(blist, struct comp_buffer,
src/audio/module_adapter/module_adapter.c-                                                        sink_list);
src/audio/module_adapter/module_adapter.c-              uint32_t flags;
--
src/audio/module_adapter/module_adapter.c:              list_for_item_safe(dp_queue_list_item, tmp, &mod->dp_queue_dp_to_ll_list) {
src/audio/module_adapter/module_adapter.c-                      struct dp_queue *dp_queue =
src/audio/module_adapter/module_adapter.c-                                      container_of(dp_queue_list_item, struct dp_queue, list);
src/audio/module_adapter/module_adapter.c-
--
src/audio/module_adapter/module_adapter.c:              list_for_item_safe(dp_queue_list_item, tmp, &mod->dp_queue_ll_to_dp_list) {
src/audio/module_adapter/module_adapter.c-                      struct dp_queue *dp_queue =
src/audio/module_adapter/module_adapter.c-                                      container_of(dp_queue_list_item, struct dp_queue, list);
src/audio/module_adapter/module_adapter.c-
--
src/audio/module_adapter/module_adapter.c:      list_for_item_safe(blist, _blist, &mod->sink_buffer_list) {
src/audio/module_adapter/module_adapter.c-              struct comp_buffer *buffer = container_of(blist, struct comp_buffer,
src/audio/module_adapter/module_adapter.c-                                                        sink_list);
src/audio/module_adapter/module_adapter.c-              uint32_t flags;
--
src/ipc/ipc-helper.c:   list_for_item_safe(clist, tmp, &icd->cd->bsource_list) {
src/ipc/ipc-helper.c-           struct comp_buffer *buffer = container_of(clist, struct comp_buffer, sink_list);
src/ipc/ipc-helper.c-
src/ipc/ipc-helper.c-           buffer->sink = NULL;
--
src/ipc/ipc-helper.c:   list_for_item_safe(clist, tmp, &icd->cd->bsink_list) {
src/ipc/ipc-helper.c-           struct comp_buffer *buffer = container_of(clist, struct comp_buffer, source_list);
src/ipc/ipc-helper.c-
src/ipc/ipc-helper.c-           buffer->source = NULL;
--
src/ipc/ipc4/helper.c:          list_for_item_safe(list, _list, &icd->cd->bsink_list) {
src/ipc/ipc4/helper.c-                  struct comp_dev *sink;
src/ipc/ipc4/helper.c-
src/ipc/ipc4/helper.c-                  buffer = container_of(list, struct comp_buffer, source_list);
--
src/ipc/ipc4/helper.c:          list_for_item_safe(list, _list, &icd->cd->bsource_list) {
src/ipc/ipc4/helper.c-                  struct comp_dev *source;
src/ipc/ipc4/helper.c-
src/ipc/ipc4/helper.c-                  buffer = container_of(list, struct comp_buffer, sink_list);
--
src/lib/notifier.c:     list_for_item_safe(wlist, tlist, &notify->list[type]) {
src/lib/notifier.c-             handle = container_of(wlist, struct callback_handle, list);
src/lib/notifier.c-             if ((!receiver || handle->receiver == receiver) &&
src/lib/notifier.c-                 (!caller || handle->caller == caller)) {
--
src/lib/notifier.c:     list_for_item_safe(wlist, tlist, &notify->list[type]) {
src/lib/notifier.c-             handle = container_of(wlist, struct callback_handle, list);
src/lib/notifier.c-             if (!caller || !handle->caller || handle->caller == caller)
src/lib/notifier.c-                     handle->cb(handle->receiver, type, data);
--
src/library_manager/lib_notification.c: list_for_item_safe(list_elem, list_tmp, &lib_notif->list) {
src/library_manager/lib_notification.c-         msg_pool_elem = container_of(list_elem, struct ipc_lib_msg, list);
src/library_manager/lib_notification.c-         assert(msg_pool_elem->msg);
src/library_manager/lib_notification.c-
--
src/platform/library/schedule/ll_schedule.c:    list_for_item_safe(tlist, tlist_, &sched_list) {
src/platform/library/schedule/ll_schedule.c-            task = container_of(tlist, struct task, list);
src/platform/library/schedule/ll_schedule.c-
src/platform/library/schedule/ll_schedule.c-            /* only run queued tasks */
--
src/schedule/zephyr_ll.c:       list_for_item_safe(list, tmp, &task_head) {
src/schedule/zephyr_ll.c-               list_item_del(list);
src/schedule/zephyr_ll.c-               list_item_append(list, &sch->tasks);
src/schedule/zephyr_ll.c-       }
--
test/cmocka/src/notifier_mocks.c:       list_for_item_safe(wlist, tlist, &notify->list[type]) {
test/cmocka/src/notifier_mocks.c-               handle = container_of(wlist, struct callback_handle, list);
test/cmocka/src/notifier_mocks.c-               if (!caller || !handle->caller || handle->caller == caller)
test/cmocka/src/notifier_mocks.c-                       handle->cb(handle->receiver, type, data);
--
test/cmocka/src/notifier_mocks.c:       list_for_item_safe(wlist, tlist, &notify->list[type]) {
test/cmocka/src/notifier_mocks.c-               handle = container_of(wlist, struct callback_handle, list);
test/cmocka/src/notifier_mocks.c-               if ((!receiver || handle->receiver == receiver) &&

Copy link
Member Author

Choose a reason for hiding this comment

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

I might add that speed isn't really a problem here, tearing down a chain DMA requires participation from the host and firmware, saving a couple of cycles to scan the component list isn't going to make any difference.

list_item_del(&icd->list);
rfree(icd);
break;
}
comp_free(dev);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would ipc_comp_free() work instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at it based on the input from @kv2019i, but that routine takes a comp_id parameter that I can't really understand, and it does a lot more things.

With this PR I opted for the bare minimum of releasing the memory that was allocated and cleaning up the linked list, but yeah it's not necessarily final. I have no idea if locking is necessary, if the multi-core stuff is required, etc.

That's really for firmware experts to comment on, all I did was root-cause an imbalance and a memory leak. I am more than happy if someone has a better solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good question @ujfalusi . This is using the component infra in a creative way. I took another read through the code and I think this still correct. The chain-dma is not setting up a full pipeline, so the actions in ipc_comp_free() are not really applicable. Maybe @lyakh can take a quick look at this as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say it should be a one-liner

-	comp_free(dev);
+	ipc_comp_free(dev);

but one would need to check carefully that that function cannot do any harm. @plbossart could you maybe give it a quick test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

update: the API seems to be asymmetric. There's comp_free() and there's ipc_comp_free() but I don't think anything like comp_alloc(), the closest I find is ipc4_add_comp_dev() and that's what chain DMA is using. So in the short run, I guess - whatever works, in the long run someone might want to clean that up a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Multiple issues:
a) int ipc_comp_free(struct ipc *ipc, uint32_t comp_id)
what is the comp_id?

b) if (!icd->cd->bsource_list.next || !icd->cd->bsink_list.next)
return -EINVAL;
we don't have any such things defined for chain-dma, do we? My guess is that we would hit this error case.

I can give it a try once you guys help me with a)...

Copy link
Member

Choose a reason for hiding this comment

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

Lets take this as a followup improvement, this is fixing a leak and unblocking tests.

}
return ret;
Expand Down
Loading