remove complex initialization, add possible race conditions

Removed the complex initilization procedure by removing the
is_initialized atomic type. However, this introduces a slight chance
that we might activate the plugin with invalid parameters.

1 files changed, 31 insertions, 114 deletions

diff --git a/litmus/sched_edf_hsb.c b/litmus/sched_edf_hsb.c
index 4c4643226675..dbf912e889f3 100644
--- a/litmus/sched_edf_hsb.c
+++ b/litmus/sched_edf_hsb.c
@@ -7,6 +7,7 @@
 #include <linux/ctype.h>
 #include <linux/time.h>
 #include <linux/string.h>
+#include <linux/sched.h>
 
 #include <litmus/litmus.h>
 #include <litmus/rt_domain.h>
@@ -14,9 +15,9 @@
 #include <litmus/litmus_proc.h>
 #include <litmus/edf_common.h>
 
-/* synchronization point to start only when all HRT servers are set up */
-static atomic_t initialized = ATOMIC_INIT(0);
-#define is_initialized (atomic_read(&initialized))
+/* forward declaration so the below macro works */
+static struct sched_plugin edf_hsb_plugin __cacheline_aligned_in_smp;
+#define is_active_plugin (litmus == &edf_hsb_plugin)
 
 struct hrt_server {
         int             cpu;
@@ -37,7 +38,8 @@ DEFINE_PER_CPU_SHARED_ALIGNED(struct hrt_server, hrt_servers);
 /*
  * Set the WCET and period for an HRT server on a specific CPU.
  * Times are given in milliseconds.
- * This should only be done in the initialization phase!
+ * There is a small race condition in checking we're not the active plugin
+ * before setting parameters, but we are not worried about that so much.
  */
 int set_hrt_params(unsigned long long cpu, unsigned long long wcet,
                 unsigned long long period)
@@ -64,17 +66,14 @@ int set_hrt_params(unsigned long long cpu, unsigned long long wcet,
                 goto out;
         }
 
-        hrt_server = &per_cpu(hrt_servers, cpu);
-
-        write_lock_irq(&hrt_server->param_lock);
-
-        /* must do this check after getting the write lock for correctness */
-        if (is_initialized){
-                printk(KERN_WARNING "Attempt to set HRT params after init.\n");
+        if (is_active_plugin) {
+                printk(KERN_WARNING "Can't set params on active plugin.\n");
                 rv = -EPERM;
-                goto out_unlock;
+                goto out;
         }
 
+        hrt_server = &per_cpu(hrt_servers, cpu);
+        write_lock_irq(&hrt_server->param_lock);
         hrt_server->wcet = wcet * NSEC_PER_MSEC;
         hrt_server->period = period * NSEC_PER_MSEC;
 
@@ -82,59 +81,33 @@ int set_hrt_params(unsigned long long cpu, unsigned long long wcet,
          * TODO probably need a function to reset hrtimers and stuff here
          */
 
-out_unlock:
         write_unlock_irq(&hrt_server->param_lock);
 out:
         return rv;
 }
 
 /*
- * do all hrt_servers have a non-zero WCET and period? if they do, set
- * the atomic initialized variable to 1.
+ * Do all hrt_servers have a non-zero WCET and period? Return 0 if so.
  */
 static int check_hrt_servers_initialized(void)
 {
         struct hrt_server *hrt_server;
         int cpu, all_init = 1;
 
-        /*
-         * we first acquire hrt server writer locks for each cpu to ensure that
-         * no one changes the value of the parameters from under us while we
-         * are checking that they are valid
-         *
-         * we don't support cpu hotplug (and I hope we never have to)
-         */
-
         for_each_online_cpu(cpu) {
 
                 hrt_server = &per_cpu(hrt_servers, cpu);
-
-                /*
-                 * acquire all the reader/writer locks as writers.
-                 * this usually triggers a "recursive lock" warning in syslog.
-                 */
-                write_lock_irq(&hrt_server->param_lock);
+                read_lock_irq(&hrt_server->param_lock);
 
                 /* if any parameter is zero, all_init will become zero */
                 all_init = all_init && hrt_server->wcet &&
                         hrt_server->period;
-        }
-
-        /*
-         * now we hold all the writer locks, so we can make a decision about
-         * these HRT servers without worry about them getting re-set
-         * underneath us.
-         */
-        if (all_init)
-                atomic_set(&initialized, 1);
 
-        for_each_online_cpu(cpu) {
-                hrt_server = &per_cpu(hrt_servers, cpu);
-                /* unlock all the reader/writer locks */
-                write_unlock_irq(&hrt_server->param_lock);
+                read_unlock_irq(&hrt_server->param_lock);
         }
 
-        return all_init;
+        /* inverted because zero is success */
+        return !all_init;
 }
 
 void reset_hrt_servers(void)
@@ -157,22 +130,30 @@ void reset_hrt_servers(void)
 
 static long edf_hsb_deactivate_plugin(void)
 {
-        return 0;
-        atomic_set(&initialized, 0);
         reset_hrt_servers();
+        return 0;
 }
 
+/*
+ * There is a small race condition in checking that all the server parameters
+ * are valid, then having one of them change before setting the active plugin,
+ * but oh well.
+ */
 static long edf_hsb_activate_plugin(void)
 {
-        return 0;
+        long rv = 0;
+        
+        if (check_hrt_servers_initialized())
+                rv = -EINVAL;
+
+        return rv;
 }
 
 /************************************************************
  * Proc stuff
 ************************************************************/
 
-static struct proc_dir_entry *edf_hsb_proc_dir = NULL, *hrt_server_proc = NULL,
-                             *initialized_proc = NULL;
+static struct proc_dir_entry *edf_hsb_proc_dir = NULL, *hrt_server_proc = NULL;
 
 static int hrt_proc_read(char* page, char **start, off_t off, int count,
                 int *eof, void *data)
@@ -274,53 +255,8 @@ loop_end:
         return count;
 }
 
-static int initialized_proc_read(char* page, char **start, off_t off, int count,
-                int *eof, void *data)
-{
-        int len = 0;
-        len += snprintf(page, PAGE_SIZE, "%d\n", is_initialized);
-        *eof = 1;
-        return len;
-}
-
-static int initialized_proc_write(struct file *file, const char __user *input,
-                unsigned long count, void *data)
-{
-#define INITIALIZED_PROC_BUF 3
-        char buffer[INITIALIZED_PROC_BUF];
-        int in_val = -1, num_read;
-
-        /* don't allow changes after initialized */
-        if (is_initialized)
-                return -EPERM;
-
-        if (count >= INITIALIZED_PROC_BUF){
-                printk(KERN_WARNING "proc buffer possibly too small in %s.\n",
-                                __func__);
-                return -ENOSPC;
-        }
-
-        if (copy_from_user(buffer, input, (INITIALIZED_PROC_BUF - 1)))
-                return -EFAULT;
-
-        buffer[INITIALIZED_PROC_BUF-1] = '\0';
-
-        num_read = sscanf(buffer, "%d", &in_val);
-
-        if (!num_read || (in_val!= 0 && in_val != 1)) {
-                printk(KERN_WARNING "Invalid HSB initialization in proc.\n");
-                return -EIO;
-        } else if (in_val == 1) {
-                check_hrt_servers_initialized();
-                if (!is_initialized)
-                        printk(KERN_WARNING "Attempted to set initialized = 1 "
-                                        "without all HRT servers setup.\n");
-        }
-
-        return count;
-}
-
-/*      Plugin object   */
+/* actual plugin object */
+/* static struct sched_plugin edf_hsb_plugin = { */
 static struct sched_plugin edf_hsb_plugin __cacheline_aligned_in_smp = {
         .plugin_name            = "EDF-HSB",
 
@@ -349,15 +285,9 @@ static struct sched_plugin edf_hsb_plugin __cacheline_aligned_in_smp = {
 };
 
 #define HRT_SERVER_PROC_NAME "hrt_server"
-#define INITIALIZED_PROC_NAME "initialized"
 
 static void exit_proc(void)
 {
-        if (initialized_proc) {
-                remove_proc_entry(INITIALIZED_PROC_NAME, edf_hsb_proc_dir);
-                initialized_proc = NULL;
-        }
-
         if (hrt_server_proc) {
                 remove_proc_entry(HRT_SERVER_PROC_NAME, edf_hsb_proc_dir);
                 hrt_server_proc = NULL;
@@ -399,19 +329,6 @@ static int __init init_edf_hsb(void)
         hrt_server_proc->read_proc = hrt_proc_read;
         hrt_server_proc->write_proc = hrt_proc_write;
 
-        initialized_proc = create_proc_entry(INITIALIZED_PROC_NAME,
-                        0644, edf_hsb_proc_dir);
-        if (!initialized_proc) {
-                printk(KERN_ERR "Could not create proc entry: %s.\n",
-                                INITIALIZED_PROC_NAME);
-                rv = -ENOENT;
-                goto out_clean_proc;
-        }
-
-        initialized_proc->read_proc = initialized_proc_read;
-        initialized_proc->write_proc = initialized_proc_write;
-
-        atomic_set(&initialized, 0);
         reset_hrt_servers();
 
         goto out;