Changeset 8432ae1 in mainline

Timestamp:

2019-08-03T08:33:31Z (5 years ago)

Author:

Matthieu Riolo <matthieu.riolo@…>

Children:

4b1c6a4b

Parents:

095d03c

git-author:

Michal Koutny <xm.koutny+hos@…> (2015-04-25 00:29:51)

git-committer:

Matthieu Riolo <matthieu.riolo@…> (2019-08-03 08:33:31)

Message:

sysman: Correct reference counting of jobs

Location:

uspace/srv/sysman

Files:

• job.c (modified) (15 diffs)
• job.h (modified) (1 diff)
• main.c (modified) (1 diff)
• sysman.c (modified) (6 diffs)

uspace/srv/sysman/job.c

@@ -48 +48 @@
                 return ENOMEM;
         }
-
         job_add_ref(blocked_job);
+
         blocked_job->blocking_jobs += 1;
 
         return EOK;
+}
+
+/** Remove blocking_job from blocked job structure
+ *
+ * @note Caller must remove blocked_job from collection of blocked_jobs
+ */
+static void job_unblock(job_t *blocked_job, job_t *blocking_job)
+{
+        if (blocking_job->retval == JOB_FAILED) {
+                blocked_job->blocking_job_failed = true;
+        }
+        blocked_job->blocking_jobs -= 1;
+
+        job_del_ref(&blocked_job);
 }
 
@@ -62 +76 @@
         link_initialize(&job->job_queue);
 
-        /* Start with one reference for the creator */
-        atomic_set(&job->refcnt, 1);
+        atomic_set(&job->refcnt, 0);
 
         job->target_state = target_state;
@@ -100 +113 @@
         job_t *job = data;
 
+        /*
+         * We have one reference from caller for our disposal,   *
+         * if needed, pass it to observer.
+         */
         if (job_eval_retval(job)) {
                 job_finish(job);
+                job_del_ref(&job);
         } else {
                 // TODO place for timeout
-                // TODO add reference to job?
                 sysman_object_observer(u, &job_check, job);
         }
@@ -110 +127 @@
 
 
-static void job_unblock(job_t *blocked_job, job_t *blocking_job)
-{
-        if (blocking_job->retval == JOB_FAILED) {
-                blocked_job->blocking_job_failed = true;
-        }
-        blocked_job->blocking_jobs -= 1;
-}
+
 
 static void job_destroy(job_t **job_ptr)
@@ -126 +137 @@
 
         assert(!link_used(&job->job_queue));
+
+        dyn_array_foreach(job->blocked_jobs, job_ptr_t, job_it) {
+                job_del_ref(&(*job_it));
+        }
         dyn_array_destroy(&job->blocked_jobs);
-        // TODO I should decrease referece of blocked jobs
 
         free(job);
@@ -164 +178 @@
                 (*job_it)->state = JOB_QUEUED;
                 list_append(&(*job_it)->job_queue, &job_queue);
-                // TODO explain this reference
-                job_add_ref(*job_it);
+                /* We pass reference from the closure to the queue */
         }
 
@@ -203 +216 @@
 
         if (result) {
-                // TODO update refcount
+                /* Remove job from queue and pass refernce to caller */
                 list_remove(&result->job_queue);
                 result->state = JOB_DEQUEUED;
@@ -225 +238 @@
          * Traverse dependency graph in BFS fashion and create jobs for every
          * necessary unit.
+         * Closure keeps reference to each job. We've to add reference to the
+         * main job, other newly created jobs are pased to the closure.
          */
         fifo_push(jobs_fifo, main_job);
         job_t *job;
+        job_add_ref(main_job);
         while ((job = fifo_pop(jobs_fifo)) != NULL) {
-                /*
-                 * Do not increase reference count of job, as we're passing it
-                 * to the closure.
-                 */
+                /* No refcount increase, pass it to the closure */
                 dyn_array_append(job_closure, job_ptr_t, job);
 
@@ -256 +269 @@
         /*
          * Newly created jobs are already passed to the closure, thus not
-         * deleting them here.
+         * deleting reference to them here.
          */
         return rc;
@@ -266 +279 @@
         if (job != NULL) {
                 job_init(job, u, target_state);
+
+                /* Start with one reference for the creator */
+                job_add_ref(job);
         }
 
@@ -271 +287 @@
 }
 
+/** Add one reference to job
+ *
+ * Usage:
+ *   - adding observer which references the job,
+ *   - raising and event that references the job,
+ *   - anytime any other new reference is made.
+ */
 void job_add_ref(job_t *job)
 {
@@ -276 +299 @@
 }
 
+/** Remove one reference from job, last remover destroys the job
+ *
+ * Usage:
+ *   - inside observer callback that references the job,
+ *   - inside event handler that references the job,
+ *   - anytime you dispose a reference to the job.
+ */
 void job_del_ref(job_t **job_ptr)
 {
         job_t *job = *job_ptr;
-        if (job == NULL) {
-                return;
-        }
-
+
+        assert(job != NULL);
         assert(atomic_get(&job->refcnt) > 0);
         if (atomic_predec(&job->refcnt) == 0) {
@@ -295 +323 @@
 
         unit_t *u = job->unit;
-        sysman_log(LVL_DEBUG, "%s, %s -> %i",
-            __func__, unit_name(u), job->target_state);
+        sysman_log(LVL_DEBUG, "%s(%p), %s -> %i",
+            __func__, job, unit_name(u), job->target_state);
 
         /* Propagate failure */
@@ -316 +344 @@
         }
 
+        /*
+         * job_check deletes reference, we want job to remain to caller, thus
+         * add one dummy ref
+         */
+        job_add_ref(job);
         job_check(job->unit, job);
         return;
@@ -333 +366 @@
         assert(job->retval != JOB_UNDEFINED_);
 
-        sysman_log(LVL_DEBUG2, "%s(%s) -> %i",
-            __func__, unit_name(job->unit), job->retval);
+        sysman_log(LVL_DEBUG2, "%s(%p) %s -> %i",
+            __func__, job, unit_name(job->unit), job->retval);
 
         job->state = JOB_FINISHED;
 
-        /* Job finished */
+        /* First remove references, then clear the array */
         dyn_array_foreach(job->blocked_jobs, job_ptr_t, job_it) {
                 job_unblock(*job_it, job);
         }
-        // TODO remove reference from blocked jobs
-
-        // TODO should reference be added (for the created event)
+        dyn_array_clear(&job->blocked_jobs);
+
+        /* Add reference for the event */
+        job_add_ref(job);
         sysman_raise_event(&sysman_event_job_changed, job);
 }

uspace/srv/sysman/job.h

@@ -85 +85 @@
 extern void job_del_ref(job_t **);
 
-
 extern void job_run(job_t *);
 extern void job_finish(job_t *);

uspace/srv/sysman/main.c

@@ -149 +149 @@
 
         /* Queue first job for processing */
+        job_add_ref(first_job);
         sysman_object_observer(first_job, &first_job_handler, NULL);
+        job_add_ref(first_job);
         sysman_raise_event(&sysman_event_job_process, first_job);
+        
+        /*
+         * Releasing our own reference (could be merged with previous add_ref,
+         * this is more explicit though.
+         */
+        job_del_ref(&first_job);
 
         /* Start sysman server */

uspace/srv/sysman/sysman.c

@@ -150 +150 @@
 
                 /* Process event */
+                sysman_log(LVL_DEBUG2, "process(%p, %p)", event->handler, event->data);
                 event->handler(event->data);
                 free(event);
@@ -157 +158 @@
 void sysman_raise_event(event_handler_t handler, void *data)
 {
+        sysman_log(LVL_DEBUG2, "%s(%p, %p)", __func__, handler, data);
         event_t *event = malloc(sizeof(event_t));
         if (event == NULL) {
@@ -237 +239 @@
         }
 
+        /*
+         * If jobs are queued, reference is passed from closure to the queue,
+         * otherwise, we still have the reference.
+         */
         rc = job_queue_add_jobs(&job_closure);
         if (rc != EOK) {
-                // TODO job_queue_add_jobs should log message
                 goto fail;
         }
+        /* We don't need job anymore */
+        job_del_ref(&job);
 
         // TODO explain why calling asynchronously
@@ -250 +257 @@
         job->retval = JOB_FAILED;
         job_finish(job);
-        // TODO clarify refcount to the main job
+        job_del_ref(&job);
+
         dyn_array_foreach(job_closure, job_ptr_t, closure_job) {
                 job_del_ref(&(*closure_job));
@@ -263 +271 @@
         while ((job = job_queue_pop_runnable())) {
                 job_run(job);
+                job_del_ref(&job);
         }
 }
@@ -269 +278 @@
 {
         notify_observers(object);
-}
+        /* Unreference the event data */
+        job_t *job = object;
+        job_del_ref(&job);
+}