sched: Introduce alternative task API.

In the current implementation struct task is public so users of this
structure can mess with internal scheduler details as they please. This
has led to many bugs and questionable code in the past. This commit
is the first step to overcome this design mistake. At the end of this
patch series struct task can be made private to sched.c.

This commit introduces the following new public functions:

	* task_register,
	* task_context,
	* sched_shutdown.

It also adds the new public task_info structure which carries the
information passed to the scheduler when a new task is registered. This
structure will stay public while struct task will become private once
all users have been converted.

task_register() is supposed to eventually replace register_task(). The
main difference of the two is that the new function returns a _pointer_
to a task structure which is allocated dynamically. Users are not
supposed to look at the fields of this pointer directly.

task_context() is a temporary helper which can be removed again at the
end of the series. Its sole purpose is to return the context pointer
which was passed at task register time as part of struct task_info.

The final new function, sched_shutdown(), deallocates the task
structures allocated during task_register() to cleanly shut down the
scheduler after all tasks have terminated.

All users need to be converted to the new API. This patch only converts
the stdout task though. The other tasks will be converted in subsequent
patches. The scheduler can tell if a task was registered using the
new API by means of the new ->owned_by_sched bit of struct task.
This boolean variable can also be removed after all tasks have been
converted.

Some users will need to query the exit status of a terminated
task. Hence we keep all tasks on the task list  after ->post_select()
returned negative but call neither ->pre_select() nor ->post_select()
any more for such tasks. This leads to the concept of zombie tasks.

diff --git a/client.c b/client.c
index b39a8b0199f0fbe66f95c12a34c5c6b05b5d23d7..338f05c8b45efe6f80705cbfc1d18fb231424549 100644 (file)
--- a/client.c
+++ b/client.c
@@ -539,8 +539,7 @@ static int supervisor_post_select(struct sched *s, struct task *t)
        if (ct->task.error < 0)
                return ct->task.error;
        if (!svt->stdout_task_started && ct->status == CL_EXECUTING) {
        if (ct->task.error < 0)
                return ct->task.error;
        if (!svt->stdout_task_started && ct->status == CL_EXECUTING) {

-               stdout_set_defaults(&sot);
-               register_task(s, &sot.task);
+               stdout_task_register(&sot, s);

                svt->stdout_task_started = true;
                return 1;
        }
                svt->stdout_task_started = true;
                return 1;
        }


@@ -619,6 +618,7 @@ int main(int argc, char *argv[])
                default: ret = -E_SERVER_CMD_FAILURE;
                }
        }
                default: ret = -E_SERVER_CMD_FAILURE;
                }
        }

+       sched_shutdown(&sched);

 out:
        if (ret < 0)
                PARA_ERROR_LOG("%s\n", para_strerror(-ret));
 out:
        if (ret < 0)
                PARA_ERROR_LOG("%s\n", para_strerror(-ret));

diff --git a/filter.c b/filter.c
index b3dc022e37f5ceaaca680f63a25173e46ba398d4..69b12d7a19a880df99e3637c25d9bb9aaee45dbc 100644 (file)
--- a/filter.c
+++ b/filter.c
@@ -141,13 +141,13 @@ int main(int argc, char *argv[])
        }
        sot->btrn = btr_new_node(&(struct btr_node_description)
                EMBRACE(.name = "stdout", .parent = parent));
        }
        sot->btrn = btr_new_node(&(struct btr_node_description)
                EMBRACE(.name = "stdout", .parent = parent));

-       stdout_set_defaults(sot);
-       register_task(&s, &sot->task);
+       stdout_task_register(sot, &s);

 
        s.default_timeout.tv_sec = 1;
        s.default_timeout.tv_usec = 0;
        btr_log_tree(sit->btrn, LL_INFO);
        ret = schedule(&s);
 
        s.default_timeout.tv_sec = 1;
        s.default_timeout.tv_usec = 0;
        btr_log_tree(sit->btrn, LL_INFO);
        ret = schedule(&s);

+       sched_shutdown(&s);

 out_cleanup:
        for (i--; i >= 0; i--) {
                struct filter_node *fn = fns[i];
 out_cleanup:
        for (i--; i >= 0; i--) {
                struct filter_node *fn = fns[i];

diff --git a/recv.c b/recv.c
index babd1e306989b18f2702ec14ef0e26e5706e99e5..4d8916f9bc4f0061732ec0394690d40470d9d177 100644 (file)
--- a/recv.c
+++ b/recv.c
@@ -89,11 +89,9 @@ int main(int argc, char *argv[])
                goto out;
        r_opened = 1;
 
                goto out;
        r_opened = 1;
 

-       memset(&sot, 0, sizeof(struct stdout_task));

        sot.btrn = btr_new_node(&(struct btr_node_description)
                EMBRACE(.parent = rn.btrn, .name = "stdout"));
        sot.btrn = btr_new_node(&(struct btr_node_description)
                EMBRACE(.parent = rn.btrn, .name = "stdout"));

-       stdout_set_defaults(&sot);
-       register_task(&s, &sot.task);
+       stdout_task_register(&sot, &s);

 
        rn.task.pre_select = r->pre_select;
        rn.task.post_select = r->post_select;
 
        rn.task.pre_select = r->pre_select;
        rn.task.post_select = r->post_select;


@@ -103,6 +101,7 @@ int main(int argc, char *argv[])
        s.default_timeout.tv_sec = 1;
        s.default_timeout.tv_usec = 0;
        ret = schedule(&s);
        s.default_timeout.tv_sec = 1;
        s.default_timeout.tv_usec = 0;
        ret = schedule(&s);

+       sched_shutdown(&s);

 out:
        if (r_opened)
                r->close(&rn);
 out:
        if (r_opened)
                r->close(&rn);

diff --git a/sched.c b/sched.c
index 85f4238b1233de58adcf07e8ac3265a1cbab9f0d..101cc4c0dad3706e841881b2a6d2989825fc58b0 100644 (file)
--- a/sched.c
+++ b/sched.c
@@ -41,6 +41,14 @@ static void sched_preselect(struct sched *s)
        }
 }
 
        }
 }
 

+static void unlink_and_free_task(struct task *t)
+{
+       PARA_INFO_LOG("freeing task %s\n", t->status);
+       list_del(&t->node);
+       if (t->owned_by_sched)
+               free(t);
+}
+

 //#define SCHED_DEBUG 1
 static inline void call_post_select(struct sched *s, struct task *t)
 {
 //#define SCHED_DEBUG 1
 static inline void call_post_select(struct sched *s, struct task *t)
 {


@@ -61,16 +69,23 @@ static inline void call_post_select(struct sched *s, struct task *t)
 #endif
 }
 
 #endif
 }
 

-static void sched_post_select(struct sched *s)
+static unsigned sched_post_select(struct sched *s)

 {
        struct task *t, *tmp;
 {
        struct task *t, *tmp;

+       unsigned num_running_tasks = 0;

 
        list_for_each_entry_safe(t, tmp, &s->task_list, node) {
 
        list_for_each_entry_safe(t, tmp, &s->task_list, node) {

+               if (t->error < 0)
+                       continue;

                call_post_select(s, t);
                t->notification = 0;
                call_post_select(s, t);
                t->notification = 0;

-               if (t->error < 0)
-                       list_del(&t->node);
+               if (t->error < 0) {
+                       if (!t->owned_by_sched)
+                               list_del(&t->node);
+               } else
+                       num_running_tasks++;

        }
        }

+       return num_running_tasks;

 }
 
 /**
 }
 
 /**


@@ -91,6 +106,7 @@ static void sched_post_select(struct sched *s)
 int schedule(struct sched *s)
 {
        int ret;
 int schedule(struct sched *s)
 {
        int ret;

+       unsigned num_running_tasks;

 
        if (!s->select_function)
                s->select_function = para_select;
 
        if (!s->select_function)
                s->select_function = para_select;


@@ -116,14 +132,34 @@ again:
                FD_ZERO(&s->wfds);
        }
        clock_get_realtime(now);
                FD_ZERO(&s->wfds);
        }
        clock_get_realtime(now);

-       sched_post_select(s);
-       if (list_empty(&s->task_list))
+       num_running_tasks = sched_post_select(s);
+       if (num_running_tasks == 0)

                return 0;
        goto again;
 }
 
 /**
                return 0;
        goto again;
 }
 
 /**

- * Add a task to the scheduler.
+ * Deallocate all resources of all tasks of a scheduler instance.
+ *
+ * \param s The scheduler instance.
+ *
+ * This should only be called after \ref schedule() has returned.
+ */
+void sched_shutdown(struct sched *s)
+{
+       struct task *t, *tmp;
+
+       list_for_each_entry_safe(t, tmp, &s->task_list, node) {
+               if (t->error >= 0)
+                       /* The task list should contain only terminated tasks. */
+                       PARA_WARNING_LOG("shutting down running task %s\n",
+                               t->status);
+               unlink_and_free_task(t);
+       }
+}
+
+/**
+ * Add a task to the scheduler. Deprecated.

  *
  * \param t The task to add.
  * \param s The scheduler instance to add the task to.
  *
  * \param t The task to add.
  * \param s The scheduler instance to add the task to.


@@ -135,9 +171,54 @@ void register_task(struct sched *s, struct task *t)
        PARA_INFO_LOG("registering %s (%p)\n", t->status, t);
        assert(t->post_select);
        t->notification = 0;
        PARA_INFO_LOG("registering %s (%p)\n", t->status, t);
        assert(t->post_select);
        t->notification = 0;

+       t->owned_by_sched = false;
+       if (!s->task_list.next)
+               INIT_LIST_HEAD(&s->task_list);
+       list_add_tail(&t->node, &s->task_list);
+}
+
+/**
+ * Add a task to the scheduler task list.
+ *
+ * \param info Task information supplied by the caller.
+ * \param s The scheduler instance.
+ *
+ * \return A pointer to a newly allocated task structure. It will be
+ * freed by sched_shutdown().
+ */
+struct task *task_register(struct task_info *info, struct sched *s)
+{
+       struct task *t = para_malloc(sizeof(*t));
+
+       assert(info->post_select);
+

        if (!s->task_list.next)
                INIT_LIST_HEAD(&s->task_list);
        if (!s->task_list.next)
                INIT_LIST_HEAD(&s->task_list);

+
+       snprintf(t->status, sizeof(t->status) - 1, "%s", info->name);
+       t->status[sizeof(t->status) - 1] = '\0';
+       t->notification = 0;
+       t->error = 0;
+       t->pre_select = info->pre_select;
+       t->post_select = info->post_select;
+       t->context = info->context;
+       t->owned_by_sched = true;

        list_add_tail(&t->node, &s->task_list);
        list_add_tail(&t->node, &s->task_list);

+       return t;
+}
+
+/**
+ * Obtain the context pointer of a task.
+ *
+ * \param t Return this task's context pointer.
+ *
+ * \return A pointer to the memory location specified previously as \a
+ * task_info->context when the task was registered with \ref task_register().
+ */
+void *task_context(struct task *t)
+{
+       assert(t->owned_by_sched);
+       return t->context;

 }
 
 /**
 }
 
 /**


@@ -157,7 +238,9 @@ char *get_task_list(struct sched *s)
 
        list_for_each_entry_safe(t, tmp, &s->task_list, node) {
                char *tmp_msg;
 
        list_for_each_entry_safe(t, tmp, &s->task_list, node) {
                char *tmp_msg;

-               tmp_msg = make_message("%s%p\t%s\n", msg? msg : "", t, t->status);
+               tmp_msg = make_message("%s%p\t%s\t%s\n", msg? msg : "", t,
+                       t->error < 0? "zombie" : "running",
+                       t->status);

                free(msg);
                msg = tmp_msg;
        }
                free(msg);
                msg = tmp_msg;
        }

diff --git a/sched.h b/sched.h
index 303877a4435d0b31fc779ac03d68e20326af17b2..40524845fdaa63b898684c2d7b1abeb650f5b9b1 100644 (file)
--- a/sched.h
+++ b/sched.h
@@ -36,35 +36,55 @@ struct sched {
 /**
  * Paraslash's task structure.
  *
 /**
  * Paraslash's task structure.
  *

- * Before registering a task to the scheduler, the task structure must be
- * filled in properly by the caller.
+ * This is considered an internal structure and will eventually be made private.

  *
  * \sa \ref sched.
  */
 struct task {
  *
  * \sa \ref sched.
  */
 struct task {

+       /** Copied from the task_info struct during task_register(). */
+       void (*pre_select)(struct sched *s, struct task *t);
+       /** Copied from the task_info struct during task_register(). */
+       int (*post_select)(struct sched *s, struct task *t);
+       /** Whether this task is active (>=0) or in error state (<0). */
+       int error;
+       /** Position of the task in the task list of the scheduler. */
+       struct list_head node;
+       /** The task name supplied when the task was registered(). */
+       char status[255];
+       /** If less than zero, the task was notified by another task. */
+       int notification;
+       /** Whether the task structure should be freed in sched_shutdown(). */
+       bool owned_by_sched;
+       /** Usually a pointer to the struct containing this task. */
+       void *context;
+};
+
+/** Information that must be supplied by callers of \ref task_register(). */
+struct task_info {
+       /** Used for log messages and by \ref get_task_list(). */
+       const char *name;

        /**
        /**

-        * The optional pre select hook of \a t.
+        * The optional pre select method.

         *
         * Its purpose is to add file descriptors to the fd sets of the
         * scheduler and to decrease the select timeout if necessary.
         */
        void (*pre_select)(struct sched *s, struct task *t);
        /**
         *
         * Its purpose is to add file descriptors to the fd sets of the
         * scheduler and to decrease the select timeout if necessary.
         */
        void (*pre_select)(struct sched *s, struct task *t);
        /**

-        * The mandatory post select hook of \a t.
+        * The mandatory post select method.

         *
         * Its purpose is to evaluate and act upon the results of the previous
         * select call. If this function returns a negative value, the
         * scheduler unregisters the task.
         */
        int (*post_select)(struct sched *s, struct task *t);
         *
         * Its purpose is to evaluate and act upon the results of the previous
         * select call. If this function returns a negative value, the
         * scheduler unregisters the task.
         */
        int (*post_select)(struct sched *s, struct task *t);

-       /** Whether this task is active (>=0) or in error state (<0). */
-       int error;
-       /** Position of the task in the task list of the scheduler. */
-       struct list_head node;
-       /** Descriptive text and current status of the task. */
-       char status[255];
-       /** If less than zero, the task was notified by another task. */
-       int notification;
+       /**
+        * This pointer is saved when the task is register(ed). It may be
+        * queried from ->pre_select() and ->post_select() via \ref
+        * task_context(). Usually this is a pointer to the struct owned by the
+        * caller which contains the task pointer as one member.
+        */
+       void *context;

 };
 
 /**
 };
 
 /**


@@ -75,8 +95,11 @@ struct task {
  */
 extern struct timeval *now;
 
  */
 extern struct timeval *now;
 

+struct task *task_register(struct task_info *info, struct sched *s);
+void *task_context(struct task *t);

 void register_task(struct sched *s, struct task *t);
 int schedule(struct sched *s);
 void register_task(struct sched *s, struct task *t);
 int schedule(struct sched *s);

+void sched_shutdown(struct sched *s);

 char *get_task_list(struct sched *s);
 void task_notify(struct task *t, int err);
 void task_notify_all(struct sched *s, int err);
 char *get_task_list(struct sched *s);
 void task_notify(struct task *t, int err);
 void task_notify_all(struct sched *s, int err);

diff --git a/stdout.c b/stdout.c
index 4554145f3d24883e828c849e856e4eca2f428cc6..0ff24e92b1b7065b8c5a2267c70817487363d251 100644 (file)
--- a/stdout.c
+++ b/stdout.c
@@ -27,7 +27,7 @@
  */
 static void stdout_pre_select(struct sched *s, struct task *t)
 {
  */
 static void stdout_pre_select(struct sched *s, struct task *t)
 {

-       struct stdout_task *sot = container_of(t, struct stdout_task, task);
+       struct stdout_task *sot = task_context(t);

        int ret;
 
        ret = btr_node_status(sot->btrn, 0, BTR_NT_LEAF);
        int ret;
 
        ret = btr_node_status(sot->btrn, 0, BTR_NT_LEAF);


@@ -48,7 +48,7 @@ static void stdout_pre_select(struct sched *s, struct task *t)
  */
 static int stdout_post_select(struct sched *s, struct task *t)
 {
  */
 static int stdout_post_select(struct sched *s, struct task *t)
 {

-       struct stdout_task *sot = container_of(t, struct stdout_task, task);
+       struct stdout_task *sot = task_context(t);

        struct btr_node *btrn = sot->btrn;
        int ret;
        char *buf;
        struct btr_node *btrn = sot->btrn;
        int ret;
        char *buf;


@@ -85,21 +85,24 @@ out:
        }
        return ret;
 }
        }
        return ret;
 }

+

 /**
 /**

- * Initialize a stdout task structure with default values.
+ * Register a stdout task structure.

  *
  *

- * \param sot The stdout task structure.
+ * \param sot The stdout task structure to register.
+ * \param s The task will be added to this scheduler's task list.

  *
  *

- * This fills in the pre/post select function pointers of the task structure
- * given by \a sot.
+ * This sets up \a sot and registers a task with \a sot as context pointer.

  */
  */

-void stdout_set_defaults(struct stdout_task *sot)
+void stdout_task_register(struct stdout_task *sot, struct sched *s)

 {
        int ret;
 {
        int ret;

-
-       sot->task.pre_select = stdout_pre_select;
-       sot->task.post_select = stdout_post_select;
-       sprintf(sot->task.status, "stdout");
+       struct task_info ti = {
+               .pre_select = stdout_pre_select,
+               .post_select = stdout_post_select,
+               .context = sot,
+               .name = "stdout",
+       };

 
        /* See stdin.c for details. */
        ret = fcntl(STDOUT_FILENO, F_GETFL);
 
        /* See stdin.c for details. */
        ret = fcntl(STDOUT_FILENO, F_GETFL);


@@ -109,4 +112,5 @@ void stdout_set_defaults(struct stdout_task *sot)
        }
        sot->fd_flags = ret;
        sot->must_set_nonblock_flag = (sot->fd_flags & O_NONBLOCK) == 0;
        }
        sot->fd_flags = ret;
        sot->must_set_nonblock_flag = (sot->fd_flags & O_NONBLOCK) == 0;

+       sot->task = task_register(&ti, s);

 }
 }

diff --git a/stdout.h b/stdout.h
index dbf8866b368f16d800ab90e799a71e07ff726954..531fc7c2d5564755c7b20804817d41545d4ba2df 100644 (file)
--- a/stdout.h
+++ b/stdout.h
@@ -13,7 +13,7 @@
  */
 struct stdout_task {
        /** The task structure used by the scheduler. */
  */
 struct stdout_task {
        /** The task structure used by the scheduler. */

-       struct task task;
+       struct task *task;

        /** Stdout is always a leaf node in the buffer tree. */
        struct btr_node *btrn;
        /** The descriptor flags of STDOUT at startup. */
        /** Stdout is always a leaf node in the buffer tree. */
        struct btr_node *btrn;
        /** The descriptor flags of STDOUT at startup. */


@@ -22,4 +22,4 @@ struct stdout_task {
        bool must_set_nonblock_flag;
 };
 
        bool must_set_nonblock_flag;
 };
 

-void stdout_set_defaults(struct stdout_task *sot);
+void stdout_task_register(struct stdout_task *sot, struct sched *s);