Active message deadlock and kmalloc canary
authorBarret Rhoden <brho@cs.berkeley.edu>
Sun, 25 Oct 2009 07:45:08 +0000 (00:45 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Sun, 25 Oct 2009 07:45:08 +0000 (00:45 -0700)
Removed the sync version of active messages (they always work now), and
removed any complaints about active message deadlock.

Kmalloc also has a canary, which will detect most kfrees of
non-kmalloc'd or misaligned buffers.

For quick and dirty  printk-style debugging, use the macro I_AM_HERE.

kern/include/kmalloc.h
kern/include/stdio.h
kern/include/trap.h
kern/src/kmalloc.c
kern/src/monitor.c
kern/src/page_alloc.c
kern/src/process.c

index c09e70c..93c841b 100644 (file)
@@ -24,15 +24,19 @@ void* (DALLOC(size) kmalloc)(size_t size, int flags);
 void* (DALLOC(size) krealloc)(void* buf, size_t size, int flags);
 void  (DFREE(addr) kfree)(void *addr);
 
+/* Flags */
 #define KMALLOC_TAG_CACHE 1
 #define KMALLOC_TAG_PAGES 2
 
+#define KMALLOC_CANARY 0xdeadbabe
+
 struct kmalloc_tag {
        int flags;
        union {
                struct kmem_cache *my_cache;
                size_t num_pages;
        };
+       uint32_t canary;
 };
 
 #endif //ROS_KERN_KMALLOC_H
index 8212951..b6d3a87 100644 (file)
@@ -14,6 +14,8 @@
 #endif
 
 #define printk(args...) cprintf(args)
+#define I_AM_HERE printk("Core %d is in %s() at %s:%d\n", core_id(), \
+                         __FUNCTION__, __FILE__, __LINE__);
 
 // lib/stdio.c
 void   cputchar(int c);
index 903c37f..35b8868 100644 (file)
@@ -69,14 +69,4 @@ typedef struct active_message NTPTV(a0t) NTPTV(a1t) NTPTV(a2t) active_message_t;
 uint32_t send_active_message(uint32_t dst, amr_t pc,
                              TV(a0t) arg0, TV(a1t) arg1, TV(a2t) arg2);
 
-/* Spins til the active message is sent.  Could block in the future. */
-static inline void
-send_active_msg_sync(uint32_t dst, amr_t pc,
-                     TV(a0t) arg0, TV(a1t) arg1, TV(a2t) arg2)
-{
-       while (send_active_message(dst, pc, arg0, arg1, arg2))
-               cpu_relax();
-       return;
-}
-
 #endif /* ROS_KERN_TRAP_H */
index 9ff4592..d53ba33 100644 (file)
@@ -74,6 +74,8 @@ void *boot_calloc(uint32_t n, size_t sz, uint32_t align)
 struct kmem_cache *kmalloc_caches[NUM_KMALLOC_CACHES];
 void kmalloc_init(void)
 {
+       // i want to know if we ever make the tag bigger (should be below 16 bytes)
+       static_assert(sizeof(struct kmalloc_tag) <= KMALLOC_ALIGNMENT);
        // build caches of common sizes
        size_t ksize = KMALLOC_SMALLEST;
        for (int i = 0; i < NUM_KMALLOC_CACHES; i++) {
@@ -105,6 +107,7 @@ void *kmalloc(size_t size, int flags)
                struct kmalloc_tag *tag = buf;
                tag->flags = KMALLOC_TAG_PAGES;
                tag->num_pages = num_pgs;
+               tag->canary = KMALLOC_CANARY;
                return buf + KMALLOC_OFFSET;
        }
        // else, alloc from the appropriate cache
@@ -115,6 +118,7 @@ void *kmalloc(size_t size, int flags)
        struct kmalloc_tag *tag = buf;
        tag->flags = KMALLOC_TAG_CACHE;
        tag->my_cache = kmalloc_caches[cache_id];
+       tag->canary = KMALLOC_CANARY;
        return buf + KMALLOC_OFFSET;
 }
 
@@ -130,6 +134,7 @@ void kfree(void *addr)
 {
        assert(addr);
        struct kmalloc_tag *tag = (struct kmalloc_tag*)(addr - KMALLOC_OFFSET);
+       assert(tag->canary == KMALLOC_CANARY);
        if (tag->flags & KMALLOC_TAG_CACHE)
                kmem_cache_free(tag->my_cache, addr - KMALLOC_OFFSET);
        else if (tag->flags & KMALLOC_TAG_PAGES) {
index 1c9163d..a0f61a3 100644 (file)
@@ -277,6 +277,7 @@ int mon_procinfo(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                printk("\tresources: show resources wanted/granted for all procs\n");
                printk("\tpid NUM: show a lot of info for proc NUM\n");
                printk("\tunlock NUM: unlock the lock for proc NUM (OMG!!!)\n");
+               printk("\tkill NUM: destroy proc NUM\n");
                return 1;
        }
        if (!strcmp(argv[1], "idle_cores")) {
@@ -299,6 +300,14 @@ int mon_procinfo(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                struct proc *p;
                envid2env(strtol(argv[2], 0, 0), &p, 0);
                spin_unlock_irqsave(&p->proc_lock);
+       } else if (!strcmp(argv[1], "kill")) {
+               if (argc != 3) {
+                       printk("Give me a pid number.\n");
+                       return 1;
+               }
+               struct proc *p;
+               envid2env(strtol(argv[2], 0, 0), &p, 0);
+               proc_destroy(p);
        } else {
                printk("Bad option\n");
                return 1;
index 6dcba14..e9076c2 100644 (file)
@@ -71,7 +71,7 @@ error_t page_alloc(page_t** page)
 }
 
 /**
- * @brief Allocated 2^order contiguous physical pages.  Will incrememnt the
+ * @brief Allocated 2^order contiguous physical pages.  Will increment the
  * reference count for the pages.
  *
  * @param[in] order order of the allocation
index 4df51d8..3a83fb5 100644 (file)
@@ -156,23 +156,22 @@ void proc_run(struct proc *p)
                        if (p->num_vcores) {
                                // TODO: handle silly state (HSS)
                                // set virtual core 0 to run the main context
+                               // TODO: this should only happen on transition (VC0)
 #ifdef __IVY__
-                               send_active_msg_sync(p->vcoremap[0], __startcore, p,
-                                                    &p->env_tf, (void *SNT)0);
+                               send_active_message(p->vcoremap[0], __startcore, p,
+                                                   &p->env_tf, (void *SNT)0);
 #else
-                               send_active_msg_sync(p->vcoremap[0], (void *)__startcore,
-                                                    (void *)p, (void *)&p->env_tf, 0);
+                               send_active_message(p->vcoremap[0], (void *)__startcore,
+                                                   (void *)p, (void *)&p->env_tf, 0);
 #endif
-                               /* handle the others.  note the sync message will spin until
-                                * there is a free active message slot, which could lock up the
-                                * system.  think about this. (TODO)(AMDL) */
+                               /* handle the others. */
                                for (int i = 1; i < p->num_vcores; i++)
 #ifdef __IVY__
-                                       send_active_msg_sync(p->vcoremap[i], __startcore,
-                                                            p, (trapframe_t *CT(1))NULL, (void *SNT)i);
+                                       send_active_message(p->vcoremap[i], __startcore,
+                                                           p, (trapframe_t *CT(1))NULL, (void *SNT)i);
 #else
-                                       send_active_msg_sync(p->vcoremap[i], (void *)__startcore,
-                                                            (void *)p, (void *)0, (void *)i);
+                                       send_active_message(p->vcoremap[i], (void *)__startcore,
+                                                           (void *)p, (void *)0, (void *)i);
 #endif
                        }
                        /* There a subtle (attempted) race avoidance here.  proc_startcore
@@ -340,8 +339,8 @@ void proc_destroy(struct proc *p)
                                current = NULL;
                        }
                        #endif
-                       send_active_msg_sync(p->vcoremap[0], __death, (void *SNT)0,
-                                            (void *SNT)0, (void *SNT)0);
+                       send_active_message(p->vcoremap[0], __death, (void *SNT)0,
+                                           (void *SNT)0, (void *SNT)0);
                        #if 0
                        /* right now, RUNNING_S only runs on a mgmt core (0), not cores
                         * managed by the idlecoremap.  so don't do this yet. */
@@ -536,17 +535,17 @@ error_t proc_give_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num)
                                printd("setting vcore %d to pcore %d\n", free_vcoreid, corelist[i]);
                                p->vcoremap[free_vcoreid] = corelist[i];
                                p->num_vcores++;
-                               // TODO: careful of active message deadlock (AMDL)
                                assert(corelist[i] != core_id()); // sanity
                                /* if we want to allow yielding of vcore0 and restarting it at
                                 * its yield point *while still RUNNING_M*, uncomment this */
+                               // TODO: we don't (VC0)
                                /*
                                if (i == 0)
-                                       send_active_msg_sync(p->vcoremap[0], __startcore,
-                                                            (uint32_t)p, (uint32_t)&p->env_tf, 0);
+                                       send_active_message(p->vcoremap[0], __startcore,
+                                                           (uint32_t)p, (uint32_t)&p->env_tf, 0);
                                else */
-                               send_active_msg_sync(corelist[i], __startcore, p,
-                                                    (void*)0, (void*)free_vcoreid);
+                               send_active_message(corelist[i], __startcore, p,
+                                                   (void*)0, (void*)free_vcoreid);
                        }
                        break;
                default:
@@ -575,7 +574,7 @@ error_t proc_set_allcores(struct proc *SAFE p, uint32_t corelist[], size_t *num,
  * contain how many items are in corelist.  This isn't implemented yet, but
  * might be necessary later.  Or not, and we'll never do it.
  *
- * TODO: think about taking vcore0.  probably are issues...
+ * TODO: think about taking vcore0.  probably are issues... (or not (VC0))
  *
  * WARNING: You must hold the proc_lock before calling this!*/
 error_t proc_take_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num,
@@ -598,8 +597,7 @@ error_t proc_take_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num,
                vcoreid = get_vcoreid(p, corelist[i]);
                assert(p->vcoremap[vcoreid] == corelist[i]);
                if (message)
-                       // TODO: careful of active message deadlock (AMDL)
-                       send_active_msg_sync(corelist[i], message, 0, 0, 0);
+                       send_active_message(corelist[i], message, 0, 0, 0);
                // give the pcore back to the idlecoremap
                idlecoremap[num_idlecores++] = corelist[i];
                p->vcoremap[vcoreid] = -1;
@@ -633,9 +631,8 @@ error_t proc_take_allcores(struct proc *SAFE p, amr_t message)
                // find next active vcore
                active_vcoreid = get_busy_vcoreid(p, active_vcoreid);
                if (message)
-                       // TODO: careful of active message deadlock (AMDL)
-                       send_active_msg_sync(p->vcoremap[active_vcoreid], message,
-                                            (void *SNT)0, (void *SNT)0, (void *SNT)0);
+                       send_active_message(p->vcoremap[active_vcoreid], message,
+                                           (void *SNT)0, (void *SNT)0, (void *SNT)0);
                // give the pcore back to the idlecoremap
                idlecoremap[num_idlecores++] = p->vcoremap[active_vcoreid];
                p->vcoremap[active_vcoreid] = -1;