smp_call wait / backend work
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 27 Apr 2009 01:06:55 +0000 (18:06 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 27 Apr 2009 01:06:55 +0000 (18:06 -0700)
Fixes the problem mentioned in the previous commit, where you would
deadlock if you tried to use the same wrapper you were currently waiting
on.  There is still the possibility of deadlocking by holding too many
waits and then not being able to find a free handler, (get stuck in the
while loop).

Also, the backend is looking rather redundant with the front end.  Might
be time to get rid of the front end, and have the only toggling happen
on the backend of running the handler.  It'd mean we can't reuse the
vector until everyone is complete, not just everyone knows what function
to run.  But we can't reliably wait without that.  The other way would
be to have the backend list be on the caller's stack, so there's no
chance of clobbering.

kern/atomic.c
kern/atomic.h
kern/init.c
kern/smp.c
kern/testing.c

index d96b190..4ac337c 100644 (file)
@@ -13,7 +13,7 @@ int commit_checklist_wait(checklist_t* list, checklist_mask_t* mask)
        // wait til the list is available.  could have some adaptive thing here
        // where it fails after X tries (like 500), gives up the lock, and returns
        // an error code
-       while (!(BITMASK_IS_CLEAR(list->mask.bits, list->mask.size)))
+       while (!checklist_is_clear(list))
                cpu_relax();
 
        // list is ours and clear, set it to the settings of our list
@@ -57,19 +57,38 @@ int commit_checklist_nowait(checklist_t* list, checklist_mask_t* mask)
 int waiton_checklist(checklist_t* list)
 {
        // can consider breakout out early, like above, and erroring out
-       while (!(BITMASK_IS_CLEAR(list->mask.bits, list->mask.size)))
+       while (!checklist_is_clear(list))
                cpu_relax();
        spin_unlock_irqsave(&list->lock);
        return 0;
 }
 
+// peaks in and sees if the list is locked with it's spinlock
+int checklist_is_locked(checklist_t* list)
+{
+       // remember the lock status is the lowest byte of the lock
+       return list->lock & 0xff;
+}
+
+// no synch guarantees - just looks at the list
+int checklist_is_clear(checklist_t* list)
+{
+       return BITMASK_IS_CLEAR(list->mask.bits, list->mask.size);
+}
+
+// no synch guarantees - just resets the list to empty
+void reset_checklist(checklist_t* list)
+{
+       CLR_BITMASK(list->mask.bits, list->mask.size);
+}
+
 // CPU mask specific - this is how cores report in
 void down_checklist(checklist_t* list)
 {
        CLR_BITMASK_BIT_ATOMIC(list->mask.bits, lapic_get_id());
 }
 
-// byte per cpu, as mentioned below
+/* Barriers */
 void init_barrier(barrier_t* barrier, uint32_t count)
 {
        barrier->lock = 0;
index 7207ee1..82f95bb 100644 (file)
@@ -46,6 +46,9 @@ typedef struct checklist {
 int commit_checklist_wait(checklist_t* list, checklist_mask_t* mask);
 int commit_checklist_nowait(checklist_t* list, checklist_mask_t* mask);
 int waiton_checklist(checklist_t* list);
+int checklist_is_locked(checklist_t* list);
+int checklist_is_clear(checklist_t* list);
+void reset_checklist(checklist_t* list);
 void down_checklist(checklist_t* list);
 // TODO - do we want to adjust the size?  (YES, don't want to check it all)
 // TODO - do we want to be able to call waiton without having called commit?
index e3d980e..4215d49 100644 (file)
@@ -61,8 +61,8 @@ void kernel_init(multiboot_info_t *mboot_info)
        // this returns when all other cores are done and ready to receive IPIs
        smp_boot();
 
-       /*
        test_smp_call_functions();
+       /*
        test_checklists();
        test_barrier();
        test_print_info();
index 05e2c23..b718406 100644 (file)
@@ -263,28 +263,44 @@ static int smp_call_function(uint8_t type, uint8_t dest, isr_t handler,
        // like taking too long to acquire the lock or clear the mask, at which
        // point, we try the next one.
        // When we are done, wrapper points to the one we finally got.
+       // Note we need to make sure the backend is unused too, even if we don't
+       // want to wait.  Don't want our backend responses clobbering another
+       // waiters results.  Even if we don't plan on waiting, we need to make sure
+       // the list is done, so that old results don't trick future viewers into
+       // thinking the list is clear.  The main idea is that if a backend is
+       // unlocked, then no one is waiting on it.  If it is clear, then all
+       // previous callees have *finished* the entire handler function.  We don't
+       // want a weird sequence of events to be able to bypass that.
        vector = lapic_get_id() % NUM_HANDLER_WRAPPERS;
        while(1) {
                wrapper = &handler_wrappers[vector];
                if (!commit_checklist_nowait(wrapper->front_cpu_list, &cpu_mask)) {
-                       break;
+                       // if the backend is free (not locked and clear), then we can break.
+                       if ((!checklist_is_locked(wrapper->back_cpu_list)) && 
+                          (checklist_is_clear(wrapper->back_cpu_list)))
+                               break;
+                       else
+                               reset_checklist(wrapper->front_cpu_list);       
                }
                vector = (vector + 1) % NUM_HANDLER_WRAPPERS;
        }
 
-       // If we want to wait, we set the mask for our backend too.  Wanting to
-       // wait is expressed by having a non-NULL handler_wrapper_t** passed in.
-       // no contention here, since this one is protected by the mask being present
-       // in the frontend.  the only way someone would try to commit on this would
-       // be if it holds the front.
-       // though someone could be still waiting on this backend (and be holding the
-       // checklist lock), and we would spin here for a while until they are done.
-       // We could consider erroring out and finding a new front-end.
+       // Wanting to wait is expressed by having a non-NULL handler_wrapper_t**
+       // passed in.
+       // Should be no contention here, since this one is protected by the mask
+       // being present in the frontend.  the only way someone would try to commit
+       // on this would be if it holds the front.
+       // Set this mask regardless if we want to wait or not.  the recipients will
+       // all down it anyway, and it protects any future users of this handler from
+       // waiting on a backend that is actually in use.  Note that above we did not
+       // allow the use of a wrapper if the backend still isn't clear - even if we
+       // won the front end.
        if (wait_wrapper) {
                commit_checklist_wait(wrapper->back_cpu_list, &cpu_mask);
                // pass out our reference to wrapper, so waiting can be done later.
                *wait_wrapper = wrapper;
-       }
+       } else
+               commit_checklist_nowait(wrapper->back_cpu_list, &cpu_mask);
 
        // now register our handler to run
        register_interrupt_handler(interrupt_handlers, wrapper->vector, handler);
index 71316a7..05d14be 100644 (file)
@@ -299,7 +299,7 @@ void test_null_handler(struct Trapframe *tf)
 
 void test_smp_call_functions(void)
 {
-       handler_wrapper_t* waiter;
+       handler_wrapper_t *waiter, *waiter2;
        uint8_t me = lapic_get_id();
        printk("\nCore %d: SMP Call Self (nowait):\n", me);
        printk("---------------------\n");
@@ -368,6 +368,12 @@ void test_smp_call_functions(void)
        smp_call_function_self(test_null_handler, &waiter);
        smp_call_wait(waiter);
        printk("A: %d, B: %d, C: %d (should be 19,19,19)\n", a, b, c);
+       printk("Attempting to deadlock by smp_calling on a current wait: ");
+       smp_call_function_self(test_null_handler, &waiter);
+       smp_call_function_self(test_null_handler, &waiter2);
+       smp_call_wait(waiter);
+       smp_call_wait(waiter2);
+       printk("Made it through!\n");
        printk("Done\n");
 }