Don't leak memory on long chains of events

Submitted by Rebecca N. Palmer on July 21, 2018, 6:19 p.m.

Details

Message ID 0645c9a0-9a74-4c65-af17-6c316501b2f9@zoho.com
State New
Headers show
Series "Don't leak memory on long chains of events" ( rev: 1 ) in Beignet

Not browsing as part of any series.

Commit Message

Rebecca N. Palmer July 21, 2018, 6:19 p.m.
Delete event->depend_events when it is no longer needed, to allow
the event objects it refers to to be freed.

This avoids out-of-memory hangs in large dependency trees
(e.g. long iterative calculations):
https://launchpad.net/bugs/1354086

Signed-off-by: Rebecca N. Palmer <rebecca_palmer@zoho.com>
---
*Possibly* also https://bugs.freedesktop.org/show_bug.cgi?id=102509

pocl does something similar:
https://sources.debian.org/src/pocl/1.1-5/lib/CL/devices/common.c/?hl=722#L714
Neo instead has the pointers go the other way (from an event to
the events waiting for it), but that would be harder and riskier
to convert existing code to.

Patch hide | download patch | download mbox

--- a/src/cl_event.c
+++ b/src/cl_event.c
@@ -183,6 +183,25 @@  cl_event_new(cl_context ctx, cl_command_
   return e;
 }
 
+/* This exists to prevent long chains of events from filling up memory (https://bugs.launchpad.net/ubuntu/+source/beignet/+bug/1354086).  Call only after the dependencies are complete, or failed and marked as such in this event's status, or when this event is being destroyed */
+LOCAL void
+cl_event_delete_depslist(cl_event event)
+{
+  CL_OBJECT_LOCK(event);
+  cl_event *old_depend_events = event->depend_events;
+  int depend_count = event->depend_event_num;
+  event->depend_event_num = 0;
+  event->depend_events = NULL;
+  CL_OBJECT_UNLOCK(event);
+  if (old_depend_events) {
+    assert(depend_count);
+    for (int i = 0; i < depend_count; i++) {
+      cl_event_delete(old_depend_events[i]);
+    }
+    cl_free(old_depend_events);
+  }
+}
+
 LOCAL void
 cl_event_delete(cl_event event)
 {
@@ -199,13 +218,7 @@  cl_event_delete(cl_event event)
 
   assert(list_node_out_of_list(&event->enqueue_node));
 
-  if (event->depend_events) {
-    assert(event->depend_event_num);
-    for (i = 0; i < event->depend_event_num; i++) {
-      cl_event_delete(event->depend_events[i]);
-    }
-    cl_free(event->depend_events);
-  }
+  cl_event_delete_depslist(event);
 
   /* Free all the callbacks. Last ref, no need to lock. */
   while (!list_empty(&event->callbacks)) {
@@ -565,8 +578,12 @@  cl_event_exec(cl_event event, cl_int exe
   assert(depend_status <= CL_COMPLETE || ignore_depends || exec_to_status == CL_QUEUED);
   if (depend_status < CL_COMPLETE) { // Error happend, cancel exec.
     ret = cl_event_set_status(event, depend_status);
+    cl_event_delete_depslist(event);
     return depend_status;
   }
+  if (depend_status == CL_COMPLETE) { // Avoid memory leak
+    cl_event_delete_depslist(event);
+  }
 
   if (cur_status <= exec_to_status) {
     return ret;
--- a/src/cl_event.h
+++ b/src/cl_event.h
@@ -44,7 +44,7 @@  typedef struct _cl_event {
   cl_command_type event_type; /* Event type. */
   cl_bool is_barrier;         /* Is this event a barrier */
   cl_int status;              /* The execution status */
-  cl_event *depend_events;    /* The events must complete before this. */
+  cl_event *depend_events;    /* The events must complete before this. May disappear after they have completed - see cl_event_delete_depslist*/
   cl_uint depend_event_num;   /* The depend events number. */
   list_head callbacks;        /* The events The event callback functions */
   list_node enqueue_node;     /* The node in the enqueue list. */