Fix a bug in umtx_sleep(). This function sleeps on the mutex's physical
authorMatthew Dillon <dillon@dragonflybsd.org>
Mon, 14 Apr 2008 20:00:29 +0000 (20:00 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Mon, 14 Apr 2008 20:00:29 +0000 (20:00 +0000)
address and will get lost if the physical page underlying the VM address is
copied on write.  This case can occur when a threaded program fork()'s.

Introduce a VM page event notification mechanism and use it to wake-up
the umtx_sleep() if the underlying page takes a COW fault.

Reported-by: Jordan Gordeev <jgordeev@dir.bg>,
     "Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx>

sys/kern/kern_umtx.c
sys/vm/vm_fault.c
sys/vm/vm_page.c
sys/vm/vm_page.h
sys/vm/vm_page2.h

index 67100c3..64b51b2 100644 (file)
@@ -31,7 +31,7 @@
  * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  * 
- * $DragonFly: src/sys/kern/kern_umtx.c,v 1.7 2007/07/02 01:30:07 dillon Exp $
+ * $DragonFly: src/sys/kern/kern_umtx.c,v 1.8 2008/04/14 20:00:28 dillon Exp $
  */
 
 /*
 #include <vm/vm_page.h>
 #include <vm/vm_kern.h>
 
+#include <vm/vm_page2.h>
+
+static void umtx_sleep_page_action_cow(vm_page_t m, vm_page_action_t action);
+
 /*
  * If the contents of the userland-supplied pointer matches the specified
  * value enter an interruptable sleep for up to <timeout> microseconds.
  * If the contents does not match then return immediately.
  *
- * The specified timeout may not exceed 1 second.
- *
- * Returns 0 if we slept and were woken up, -1 and ETIMEDOUT if we slept
- * and timed out, and EBUSY if the contents of the pointer did not match
- * the specified value.  A timeout of 0 indicates an unlimited sleep.
+ * Returns 0 if we slept and were woken up, -1 and EWOULDBLOCK if we slept
+ * and timed out, and EBUSY if the contents of the pointer already does
+ * not match the specified value.  A timeout of 0 indicates an unlimited sleep.
  * EINTR is returned if the call was interrupted by a signal (even if
  * the signal specifies that the system call should restart).
  *
  * safely race against changes in *ptr as long as we are properly interlocked
  * against the umtx_wakeup() call.
  *
- * The VM page associated with the mutex is held to prevent reuse in order
- * to guarentee that the physical address remains consistent.
+ * The VM page associated with the mutex is held in an attempt to keep
+ * the mutex's physical address consistent, allowing umtx_sleep() and
+ * umtx_wakeup() to use the physical address as their rendezvous.  BUT
+ * situations can arise where the physical address may change, particularly
+ * if a threaded program fork()'s and the mutex's memory becomes
+ * copy-on-write.  We register an event on the VM page to catch COWs.
  *
  * umtx_sleep { const int *ptr, int value, int timeout }
  */
@@ -93,26 +99,49 @@ sys_umtx_sleep(struct umtx_sleep_args *uap)
 {
     int error = EBUSY;
     struct sf_buf *sf;
+    struct vm_page_action action;
     vm_page_t m;
     void *waddr;
     int offset;
     int timeout;
 
-    if ((unsigned int)uap->timeout > 1000000)
+    if (uap->timeout < 0)
        return (EINVAL);
     if ((vm_offset_t)uap->ptr & (sizeof(int) - 1))
        return (EFAULT);
-    m = vm_fault_page_quick((vm_offset_t)uap->ptr, VM_PROT_READ, &error);
+
+    /*
+     * When faulting in the page, force any COW pages to be resolved.
+     * Otherwise the physical page we sleep on my not match the page
+     * being woken up.
+     */
+    m = vm_fault_page_quick((vm_offset_t)uap->ptr, VM_PROT_READ|VM_PROT_WRITE, &error);
     if (m == NULL)
        return (EFAULT);
     sf = sf_buf_alloc(m, SFB_CPUPRIVATE);
     offset = (vm_offset_t)uap->ptr & PAGE_MASK;
 
+    /*
+     * The critical section is required to interlock the tsleep against
+     * a wakeup from another cpu.  The lfence forces synchronization.
+     */
     if (*(int *)(sf_buf_kva(sf) + offset) == uap->value) {
-       if ((timeout = uap->timeout) != 0)
-           timeout = (timeout * hz + 999999) / 1000000;
+       if ((timeout = uap->timeout) != 0) {
+           timeout = (timeout / 1000000) * hz +
+                     ((timeout % 1000000) * hz + 999999) / 1000000;
+       }
        waddr = (void *)((intptr_t)VM_PAGE_TO_PHYS(m) + offset);
-       error = tsleep(waddr, PCATCH|PDOMAIN_UMTX, "umtxsl", timeout);
+       crit_enter();
+       tsleep_interlock(waddr);
+       if (*(int *)(sf_buf_kva(sf) + offset) == uap->value) {
+           vm_page_init_action(&action, umtx_sleep_page_action_cow, waddr);
+           vm_page_register_action(m, &action, VMEVENT_COW);
+           error = tsleep(waddr, PCATCH|PDOMAIN_UMTX, "umtxsl", timeout);
+           vm_page_unregister_action(m, &action);
+       } else {
+           error = EBUSY;
+       }
+       crit_exit();
        /* Always break out in case of signal, even if restartable */
        if (error == ERESTART)
                error = EINTR;
@@ -125,6 +154,17 @@ sys_umtx_sleep(struct umtx_sleep_args *uap)
     return(error);
 }
 
+/*
+ * If this page is being copied it may no longer represent the page
+ * underlying our virtual address.  Wake up any umtx_sleep()'s
+ * that were waiting on its physical address to force them to retry.
+ */
+static void
+umtx_sleep_page_action_cow(vm_page_t m, vm_page_action_t action)
+{
+    wakeup_domain(action->data, PDOMAIN_UMTX);
+}
+
 /*
  * umtx_wakeup { const int *ptr, int count }
  *
index d8ec499..b4fbf64 100644 (file)
@@ -67,7 +67,7 @@
  * rights to redistribute these changes.
  *
  * $FreeBSD: src/sys/vm/vm_fault.c,v 1.108.2.8 2002/02/26 05:49:27 silby Exp $
- * $DragonFly: src/sys/vm/vm_fault.c,v 1.44 2007/08/28 01:09:07 dillon Exp $
+ * $DragonFly: src/sys/vm/vm_fault.c,v 1.45 2008/04/14 20:00:29 dillon Exp $
  */
 
 /*
@@ -1348,6 +1348,7 @@ readrest:
                                 * Oh, well, lets copy it.
                                 */
                                vm_page_copy(fs->m, fs->first_m);
+                               vm_page_event(fs->m, VMEVENT_COW);
                        }
 
                        if (fs->m) {
@@ -1650,6 +1651,7 @@ vm_fault_copy_entry(vm_map_t dst_map, vm_map_t src_map,
                        panic("vm_fault_copy_wired: page missing");
 
                vm_page_copy(src_m, dst_m);
+               vm_page_event(src_m, VMEVENT_COW);
 
                /*
                 * Enter it in the pmap...
index 2971ddd..7afcad6 100644 (file)
@@ -35,7 +35,7 @@
  *
  *     from: @(#)vm_page.c     7.4 (Berkeley) 5/7/91
  * $FreeBSD: src/sys/vm/vm_page.c,v 1.147.2.18 2002/03/10 05:03:19 alc Exp $
- * $DragonFly: src/sys/vm/vm_page.c,v 1.36 2007/12/06 22:25:49 dillon Exp $
+ * $DragonFly: src/sys/vm/vm_page.c,v 1.37 2008/04/14 20:00:29 dillon Exp $
  */
 
 /*
@@ -1596,6 +1596,24 @@ vm_page_test_dirty(vm_page_t m)
        }
 }
 
+/*
+ * Issue an event on a VM page.  Corresponding action structures are
+ * removed from the page's list and called.
+ */
+void
+vm_page_event_internal(vm_page_t m, vm_page_event_t event)
+{
+       struct vm_page_action *scan, *next;
+
+       LIST_FOREACH_MUTABLE(scan, &m->action_list, entry, next) {
+               if (scan->event == event) {
+                       scan->event = VMEVENT_NONE;
+                       LIST_REMOVE(scan, entry);
+                       scan->func(m, scan);
+               }
+       }
+}
+
 #include "opt_ddb.h"
 #ifdef DDB
 #include <sys/kernel.h>
index aa450e5..07e68ff 100644 (file)
@@ -62,7 +62,7 @@
  * rights to redistribute these changes.
  *
  * $FreeBSD: src/sys/vm/vm_page.h,v 1.75.2.8 2002/03/06 01:07:09 dillon Exp $
- * $DragonFly: src/sys/vm/vm_page.h,v 1.26 2007/07/02 15:57:48 dillon Exp $
+ * $DragonFly: src/sys/vm/vm_page.h,v 1.27 2008/04/14 20:00:29 dillon Exp $
  */
 
 /*
 
 #endif
 
+typedef enum vm_page_event { VMEVENT_NONE, VMEVENT_COW } vm_page_event_t;
+
+struct vm_page_action {
+       LIST_ENTRY(vm_page_action) entry;
+       vm_page_event_t         event;
+       void                    (*func)(struct vm_page *,
+                                       struct vm_page_action *);
+       void                    *data;
+};
+
+typedef struct vm_page_action *vm_page_action_t;
+
 /*
  *     Management of resident (logical) pages.
  *
@@ -153,10 +165,10 @@ struct vm_page {
        u_short queue;                  /* page queue index */
        u_short flags;                  /* see below */
        u_short pc;                     /* page color */
-       u_short wire_count;             /* wired down maps refs (P) */
-       int     hold_count;             /* page hold count */
        u_char  act_count;              /* page usage count */
        u_char  busy;                   /* page busy count */
+       u_int   wire_count;             /* wired down maps refs (P) */
+       int     hold_count;             /* page hold count */
 
        /*
         * NOTE that these must support one bit per DEV_BSIZE in a page!!!
@@ -170,6 +182,7 @@ struct vm_page {
        u_short dirty;                  /* map of dirty DEV_BSIZE chunks */
 #endif
        struct msf_buf *msf_hint;       /* first page of an msfbuf map */
+       LIST_HEAD(,vm_page_action) action_list;
 };
 
 #ifndef __VM_PAGE_T_DEFINED__
@@ -480,6 +493,7 @@ void vm_page_zero_invalid(vm_page_t m, boolean_t setvalid);
 void vm_page_free_toq(vm_page_t m);
 vm_offset_t vm_contig_pg_kmap(int, u_long, vm_map_t, int);
 void vm_contig_pg_free(int, u_long);
+void vm_page_event_internal(vm_page_t, vm_page_event_t);
 
 /*
  * Holding a page keeps it from being reused.  Other parts of the system
index 7fb1c71..63dfc62 100644 (file)
@@ -32,7 +32,7 @@
  *
  *     @(#)vmmeter.h   8.2 (Berkeley) 7/10/94
  * $FreeBSD: src/sys/sys/vmmeter.h,v 1.21.2.2 2002/10/10 19:28:21 dillon Exp $
- * $DragonFly: src/sys/vm/vm_page2.h,v 1.2 2006/05/20 02:42:15 dillon Exp $
+ * $DragonFly: src/sys/vm/vm_page2.h,v 1.3 2008/04/14 20:00:29 dillon Exp $
  */
 
 #ifndef _VM_VM_PAGE2_H_
 #ifndef _SYS_VMMETER_H_
 #include <sys/vmmeter.h>
 #endif
+#ifndef _SYS_QUEUE_H_
+#include <sys/queue.h>
+#endif
+#ifndef _VM_PAGE_H_
+#include <vm/vm_page.h>
+#endif
 
 #ifdef _KERNEL
 
@@ -131,6 +137,42 @@ vm_paging_needed(void)
     );
 }
 
+static __inline
+void
+vm_page_event(vm_page_t m, vm_page_event_t event)
+{
+    if (LIST_FIRST(&m->action_list))
+       vm_page_event_internal(m, event);
+}
+
+static __inline
+void
+vm_page_init_action(vm_page_action_t action,
+                   void (*func)(vm_page_t, vm_page_action_t), void *data)
+{
+    action->func = func;
+    action->data = data;
+}
+
+static __inline
+void
+vm_page_register_action(vm_page_t m, vm_page_action_t action,
+                       vm_page_event_t event)
+{
+    action->event = event;
+    LIST_INSERT_HEAD(&m->action_list, action, entry);
+}
+
+static __inline
+void
+vm_page_unregister_action(vm_page_t m, vm_page_action_t action)
+{
+    if (action->event != VMEVENT_NONE) {
+       action->event = VMEVENT_NONE;
+       LIST_REMOVE(action, entry);
+    }
+}
+
 #endif /* _KERNEL */
 #endif /* _VM_VM_PAGE2_H_ */