Feather-Trace: dynamic memory allocation and clean exit2011.1

This patch changes Feather-Trace to allocate memory for the minor
devices dynamically, which addresses a long-standing FIXME. It also
provides clean module exit and error conditions for Feather-Trace.

4 files changed, 148 insertions, 55 deletions

diff --git a/include/litmus/ftdev.h b/include/litmus/ftdev.h
index efb2a5c9a9b0..348387e9adf9 100644
--- a/include/litmus/ftdev.h
+++ b/include/litmus/ftdev.h
@@ -6,8 +6,6 @@
 #include <linux/mutex.h>
 #include <linux/cdev.h>
 
-#define MAX_FTDEV_MINORS NR_CPUS
-
 #define FTDEV_ENABLE_CMD        0
 #define FTDEV_DISABLE_CMD       1
 
@@ -32,11 +30,11 @@ struct ftdev_minor {
 };
 
 struct ftdev {
+        dev_t                   major;
         struct cdev             cdev;
         struct class*           class;
         const char*             name;
-        /* FIXME: don't waste memory, allocate dynamically */
-        struct ftdev_minor      minor[MAX_FTDEV_MINORS];
+        struct ftdev_minor*     minor;
         unsigned int            minor_cnt;
         ftdev_alloc_t           alloc;
         ftdev_free_t            free;
@@ -46,7 +44,9 @@ struct ftdev {
 struct ft_buffer* alloc_ft_buffer(unsigned int count, size_t size);
 void free_ft_buffer(struct ft_buffer* buf);
 
-void ftdev_init(struct ftdev* ftdev, struct module* owner, const char* name);
+int ftdev_init( struct ftdev* ftdev, struct module* owner,
+                const int minor_cnt, const char* name);
+void ftdev_exit(struct ftdev* ftdev);
 int register_ftdev(struct ftdev* ftdev);
 
 #endif
diff --git a/litmus/ftdev.c b/litmus/ftdev.c
index d68d05c6d7dc..4a4b2e3e56c2 100644
--- a/litmus/ftdev.c
+++ b/litmus/ftdev.c
@@ -310,80 +310,131 @@ struct file_operations ftdev_fops = {
         .read    = ftdev_read,
 };
 
-
-void ftdev_init(struct ftdev* ftdev, struct module* owner, const char* name)
+int ftdev_init( struct ftdev* ftdev, struct module* owner,
+                const int minor_cnt, const char* name)
 {
-        int i, error;
+        int i, err;
+
+        BUG_ON(minor_cnt < 1);
+
         cdev_init(&ftdev->cdev, &ftdev_fops);
         ftdev->name = name;
+        ftdev->minor_cnt = minor_cnt;
         ftdev->cdev.owner = owner;
         ftdev->cdev.ops = &ftdev_fops;
-        ftdev->minor_cnt  = 0;
-        for (i = 0; i < MAX_FTDEV_MINORS; i++) {
+        ftdev->alloc    = NULL;
+        ftdev->free     = NULL;
+        ftdev->can_open = NULL;
+
+        ftdev->minor = kcalloc(ftdev->minor_cnt, sizeof(*ftdev->minor),
+                        GFP_KERNEL);
+        if (!ftdev->minor) {
+                printk(KERN_WARNING "ftdev(%s): Could not allocate memory\n",
+                        ftdev->name);
+                err = -ENOMEM;
+                goto err_out;
+        }
+
+        for (i = 0; i < ftdev->minor_cnt; i++) {
                 mutex_init(&ftdev->minor[i].lock);
                 ftdev->minor[i].readers = 0;
                 ftdev->minor[i].buf     = NULL;
                 ftdev->minor[i].events  = NULL;
         }
-        ftdev->alloc    = NULL;
-        ftdev->free     = NULL;
-        ftdev->can_open = NULL;
 
         ftdev->class = class_create(owner, ftdev->name);
         if (IS_ERR(ftdev->class)) {
-                error = PTR_ERR(ftdev->class);
+                err = PTR_ERR(ftdev->class);
                 printk(KERN_WARNING "ftdev(%s): "
-                        "Could not create device class.\n",
-                        name);
+                        "Could not create device class.\n", ftdev->name);
+                goto err_dealloc;
         }
+
+        return 0;
+
+err_dealloc:
+        kfree(ftdev->minor);
+err_out:
+        return err;
+}
+
+/*
+ * Destroy minor devices up to, but not including, up_to.
+ */
+static void ftdev_device_destroy(struct ftdev* ftdev, unsigned int up_to)
+{
+        dev_t minor_cntr;
+
+        if (up_to < 1)
+                up_to = (ftdev->minor_cnt < 1) ? 0 : ftdev->minor_cnt;
+
+        for (minor_cntr = 0; minor_cntr < up_to; ++minor_cntr)
+                device_destroy(ftdev->class, MKDEV(ftdev->major, minor_cntr));
+}
+
+void ftdev_exit(struct ftdev* ftdev)
+{
+        printk("ftdev(%s): Exiting\n", ftdev->name);
+        ftdev_device_destroy(ftdev, -1);
+        cdev_del(&ftdev->cdev);
+        unregister_chrdev_region(MKDEV(ftdev->major, 0), ftdev->minor_cnt);
+        class_destroy(ftdev->class);
+        kfree(ftdev->minor);
 }
 
 int register_ftdev(struct ftdev* ftdev)
 {
         struct device **device;
-        dev_t   trace_dev;
-        int error = 0, major, i;
+        dev_t trace_dev_tmp, minor_cntr;
+        int err;
 
-        error = alloc_chrdev_region(&trace_dev, 0, ftdev->minor_cnt,
+        err = alloc_chrdev_region(&trace_dev_tmp, 0, ftdev->minor_cnt,
                         ftdev->name);
-        major = MAJOR(trace_dev);
-        if (error)
-        {
+        if (err) {
                 printk(KERN_WARNING "ftdev(%s): "
-                       "Could not register major/minor number %d/%u\n",
-                       ftdev->name, major, ftdev->minor_cnt);
-                goto out;
+                       "Could not allocate char. device region (%d minors)\n",
+                       ftdev->name, ftdev->minor_cnt);
+                goto err_out;
         }
-        error = cdev_add(&ftdev->cdev, trace_dev, ftdev->minor_cnt);
-        if (error) {
+
+        ftdev->major = MAJOR(trace_dev_tmp);
+
+        err = cdev_add(&ftdev->cdev, trace_dev_tmp, ftdev->minor_cnt);
+        if (err) {
                 printk(KERN_WARNING "ftdev(%s): "
-                       "Could not add cdev for major/minor = %d/%u.\n",
-                       ftdev->name, major, ftdev->minor_cnt);
-                goto out;
+                       "Could not add cdev for major %u with %u minor(s).\n",
+                       ftdev->name, ftdev->major, ftdev->minor_cnt);
+                goto err_unregister;
         }
 
-        /*
-         * create all the minor devices
-         */
-        for (i = 0; i < ftdev->minor_cnt; ++i)
+        /* create the minor device(s) */
+        for (minor_cntr = 0; minor_cntr < ftdev->minor_cnt; ++minor_cntr)
         {
-                trace_dev = MKDEV(major, i);
-                device = &(ftdev->minor[i].device);
+                trace_dev_tmp = MKDEV(ftdev->major, minor_cntr);
+                device = &ftdev->minor[minor_cntr].device;
 
-                *device = device_create(ftdev->class, NULL, trace_dev, NULL,
-                                "%s%d", ftdev->name, i);
+                *device = device_create(ftdev->class, NULL, trace_dev_tmp, NULL,
+                                "litmus/%s%d", ftdev->name, minor_cntr);
                 if (IS_ERR(*device)) {
-                        error = PTR_ERR(*device);
+                        err = PTR_ERR(*device);
                         printk(KERN_WARNING "ftdev(%s): "
                                 "Could not create device major/minor number "
-                                "%d/%d\n", ftdev->name, major, i);
+                                "%u/%u\n", ftdev->name, ftdev->major,
+                                minor_cntr);
                         printk(KERN_WARNING "ftdev(%s): "
-                                "Will not continue creating devices. Tracing "
-                                "may be in an inconsistent state.\n",
+                                "will attempt deletion of allocated devices.\n",
                                 ftdev->name);
-                        goto out;
+                        goto err_minors;
                 }
         }
-out:
-        return error;
+
+        return 0;
+
+err_minors:
+        ftdev_device_destroy(ftdev, minor_cntr);
+        cdev_del(&ftdev->cdev);
+err_unregister:
+        unregister_chrdev_region(MKDEV(ftdev->major, 0), ftdev->minor_cnt);
+err_out:
+        return err;
 }
diff --git a/litmus/sched_task_trace.c b/litmus/sched_task_trace.c
index 7ef7b2889b42..a15b25d21a89 100644
--- a/litmus/sched_task_trace.c
+++ b/litmus/sched_task_trace.c
@@ -38,12 +38,17 @@ static int st_dev_can_open(struct ftdev *dev, unsigned int cpu)
 static int __init init_sched_task_trace(void)
 {
         struct local_buffer* buf;
-        int i, ok = 0;
+        int i, ok = 0, err;
         printk("Allocated %u sched_trace_xxx() events per CPU "
                "(buffer size: %d bytes)\n",
                NO_EVENTS, (int) sizeof(struct local_buffer));
-        ftdev_init(&st_dev, THIS_MODULE, "sched_trace");
-        for (i = 0; i < NR_CPUS; i++) {
+
+        err = ftdev_init(&st_dev, THIS_MODULE,
+                        num_online_cpus(), "sched_trace");
+        if (err)
+                goto err_out;
+
+        for (i = 0; i < st_dev.minor_cnt; i++) {
                 buf = &per_cpu(st_event_buffer, i);
                 ok += init_ft_buffer(&buf->ftbuf, NO_EVENTS,
                                      sizeof(struct st_event_record),
@@ -51,16 +56,32 @@ static int __init init_sched_task_trace(void)
                                      buf->record);
                 st_dev.minor[i].buf = &buf->ftbuf;
         }
-        if (ok == NR_CPUS) {
-                st_dev.minor_cnt = NR_CPUS;
+        if (ok == st_dev.minor_cnt) {
                 st_dev.can_open = st_dev_can_open;
-                return register_ftdev(&st_dev);
+                err = register_ftdev(&st_dev);
+                if (err)
+                        goto err_dealloc;
         } else {
-                return -EINVAL;
+                err = -EINVAL;
+                goto err_dealloc;
         }
+
+        return 0;
+
+err_dealloc:
+        ftdev_exit(&st_dev);
+err_out:
+        printk(KERN_WARNING "Could not register sched_trace module\n");
+        return err;
+}
+
+static void __exit exit_sched_task_trace(void)
+{
+        ftdev_exit(&st_dev);
 }
 
 module_init(init_sched_task_trace);
+module_exit(exit_sched_task_trace);
 
 
 static inline struct st_event_record* get_record(u8 type, struct task_struct* t)
diff --git a/litmus/trace.c b/litmus/trace.c
index da650dfe7f4d..e7ea1c2ab3e4 100644
--- a/litmus/trace.c
+++ b/litmus/trace.c
@@ -90,12 +90,33 @@ static void free_timestamp_buffer(struct ftdev* ftdev, unsigned int idx)
 
 static int __init init_ft_overhead_trace(void)
 {
+        int err;
+
         printk("Initializing Feather-Trace overhead tracing device.\n");
-        ftdev_init(&overhead_dev, THIS_MODULE, "ft_trace");
-        overhead_dev.minor_cnt = 1; /* only one buffer */
+        err = ftdev_init(&overhead_dev, THIS_MODULE, 1, "ft_trace");
+        if (err)
+                goto err_out;
+
         overhead_dev.alloc = alloc_timestamp_buffer;
         overhead_dev.free  = free_timestamp_buffer;
-        return register_ftdev(&overhead_dev);
+
+        err = register_ftdev(&overhead_dev);
+        if (err)
+                goto err_dealloc;
+
+        return 0;
+
+err_dealloc:
+        ftdev_exit(&overhead_dev);
+err_out:
+        printk(KERN_WARNING "Could not register ft_trace module.\n");
+        return err;
+}
+
+static void __exit exit_ft_overhead_trace(void)
+{
+        ftdev_exit(&overhead_dev);
 }
 
 module_init(init_ft_overhead_trace);
+module_exit(exit_ft_overhead_trace);