Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 19 Jul 2007 20:34:16 GMT
From:      Fredrik Lindberg <fli@FreeBSD.org>
To:        Perforce Change Reviews <perforce@FreeBSD.org>
Subject:   PERFORCE change 123751 for review
Message-ID:  <200707192034.l6JKYGGa040518@repoman.freebsd.org>

next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=123751

Change 123751 by fli@fli_nexus on 2007/07/19 20:33:30

	- Must hold event lock during initialization of an event, otherwise
	  there is a removal race on events that are removed directly 
	  after they have been added (such as EOF on a new socket).
	- While here, use global locking macros and sort header files.

Affected files ...

.. //depot/projects/soc2007/fli-mdns_sd/mdnsd/event.c#6 edit
.. //depot/projects/soc2007/fli-mdns_sd/mdnsd/event.h#5 edit

Differences ...

==== //depot/projects/soc2007/fli-mdns_sd/mdnsd/event.c#6 (text+ko) ====

@@ -34,28 +34,15 @@
 #include <strings.h>
 #include <unistd.h>
 
+#include "debug.h"
 #include "event.h"
+#include "log.h"
+#include "threads.h"
 #include "wqueue.h"
-#include "log.h"
-#include "debug.h"
 
 static int remove_event(struct eventlist *, struct event *, ev_arg *);
 static int event_engine(wq_arg);
 
-#ifdef HAVE_PTHREAD
-#define EV_RLOCK(ev) pthread_rwlock_rdlock(&(ev)->ev_lock)
-#define EV_WLOCK(ev) pthread_rwlock_wrlock(&(ev)->ev_lock)
-#define EV_UNLOCK(ev) pthread_rwlock_unlock(&(ev)->ev_lock)
-#define EVL_LOCK(evl) pthread_mutex_lock(&(evl)->evl_mtx)
-#define EVL_UNLOCK(evl) pthread_mutex_unlock(&(evl)->evl_mtx)
-#else
-#define EV_RLOCK(ev)
-#define EV_WLOCK(ev)
-#define EV_UNLOCK(ev)
-#define EVL_LOCK(evl)
-#define EVL_UNLOCK(evl)
-#endif
-
 /*
  * Initialize a new event list
  */
@@ -75,9 +62,7 @@
 	TAILQ_INIT(&evl->evl_events);
 
 	evl->evl_kq = kq;
-#ifdef HAVE_PTHREAD
-	pthread_mutex_init(&evl->evl_mtx, NULL);
-#endif
+	MTX_INIT(evl, evl_mtx, NULL);
 	MDNS_INIT_SET(evl, evl_magic);
 	return (evl);
 }
@@ -95,13 +80,13 @@
 	int empty;
 
 	MDNS_INIT_ASSERT(evl, evl_magic);
-	EVL_LOCK(evl);
+	MTX_LOCK(evl, evl_mtx);
 	while (!TAILQ_EMPTY(&evl->evl_events)) {
 		ev = TAILQ_FIRST(&evl->evl_events);
-		EV_WLOCK(ev);
+		RW_WLOCK(ev, ev_lock);
 		ev->ev_flags |= EVENT_FLAG_DYING;
 		if (remove_event(evl, ev, &ev->ev_init_arg) == 1) {
-			EV_UNLOCK(ev);
+			RW_UNLOCK(ev, ev_lock);
 		}
 	}
 	empty = TAILQ_EMPTY(&evl->evl_events);
@@ -115,7 +100,7 @@
 		free(evl);
 	}
 	else {
-		EVL_UNLOCK(evl);
+		MTX_UNLOCK(evl, evl_mtx);
 	}
 
 	return (!empty);
@@ -153,10 +138,10 @@
 		break;
 	}
 
-	EV_WLOCK(ev);
+	RW_WLOCK(ev, ev_lock);
 	if ((ev->ev_flags & EVENT_FLAG_EX) && ev->ev_redo > 0) {
 		ev->ev_redo--;
-		EV_UNLOCK(ev);
+		RW_UNLOCK(ev, ev_lock);
 		dprintf(DEBUG_EVENT, "Executing event again ev=%x", ev);
 		goto again;
 	}
@@ -164,11 +149,11 @@
 	if (ev->ev_flags & EVENT_FLAG_DYING) {
 		evl = ev->ev_evl;
 		if (remove_event(evl, ev, &ev->ev_init_arg) == 1) {
-			EV_UNLOCK(ev);
+			RW_UNLOCK(ev, ev_lock);
 		}
 	}
 	else {
-		EV_UNLOCK(ev);
+		RW_UNLOCK(ev, ev_lock);
 	}
 	return (0);
 }
@@ -191,9 +176,9 @@
 	struct kevent kev;
 
 	MDNS_INIT_ASSERT(evl, evl_magic);
-	EVL_LOCK(evl);
+	MTX_LOCK(evl, evl_mtx);
 	kq = evl->evl_kq;
-	EVL_UNLOCK(evl);
+	MTX_UNLOCK(evl, evl_mtx);
 
 	/*
 	 * We (ab)use SIGUSR1 for our own evil purposes, when an event handler
@@ -217,10 +202,10 @@
 		if (ev == NULL)
 			continue;
 
-		EV_WLOCK(ev);
+		RW_WLOCK(ev, ev_lock);
 		if (kev.flags & EV_EOF) {
 			if (remove_event(evl, ev, &ev->ev_init_arg) == 1) {
-				EV_UNLOCK(ev);
+				RW_UNLOCK(ev, ev_lock);
 			}
 			continue;
 		}
@@ -230,7 +215,7 @@
 			if (ev->ev_refcnt == 0)
 				remove_event(evl, ev, &ev->ev_init_arg);
 			else {
-				EV_UNLOCK(ev);
+				RW_UNLOCK(ev, ev_lock);
 			}
 			continue;
 		}
@@ -240,12 +225,12 @@
 			dprintf(DEBUG_EVENT,
 			    "Exclusive event already in progress ev=%x", ev);
 			ev->ev_redo++;	
-			EV_UNLOCK(ev);
+			RW_UNLOCK(ev, ev_lock);
 			continue;
 		}
 
 		ev->ev_refcnt++;
-		EV_UNLOCK(ev);
+		RW_UNLOCK(ev, ev_lock);
 		dprintf(DEBUG_EVENT,
 		    "Event fired, dispatched to queue=%x, ev=%x", wq, ev);
 		wa.ptr = ev;
@@ -272,10 +257,10 @@
 	pid_t self;
 
 	MDNS_INIT_ASSERT(evl, evl_magic);
-	EVL_LOCK(evl);
+	MTX_LOCK(evl, evl_mtx);
 	dprintf(DEBUG_EVENT, "Event exit called");
 	evl->evl_flags |= EVL_FLAG_DYING;
-	EVL_UNLOCK(evl);
+	MTX_UNLOCK(evl, evl_mtx);
 
 	self = getpid();
 	kill(self, SIGUSR1);
@@ -327,10 +312,11 @@
 #ifdef HAVE_PTHREAD
 	pthread_rwlock_init(&ev->ev_lock, NULL);
 #endif
+	RW_WLOCK(ev, ev_lock);
 
-	EVL_LOCK(evl);
+	MTX_LOCK(evl, evl_mtx);
 	ev->ev_id = evl->evl_len++;
-	EVL_UNLOCK(evl);
+	MTX_UNLOCK(evl, evl_mtx);
 
 	switch (type) {
 	case EVENT_TYPE_IO:
@@ -377,19 +363,21 @@
 	kev.flags |= EV_ADD | EV_CLEAR;
 	ret = kevent(evl->evl_kq, &kev, 1, NULL, 0, NULL);
 	if (ret != 0) {
+		RW_UNLOCK(ev, ev_lock);
 		free(ev);
 		dprintf(DEBUG_EVENT, "Failed to install kevent");	
 		return (-1);
 	}
 	else {
-		EVL_LOCK(evl);
+		MTX_LOCK(evl, evl_mtx);
 		TAILQ_INSERT_TAIL(&evl->evl_events, ev, ev_evlist);
-		EVL_UNLOCK(evl);
+		MTX_UNLOCK(evl, evl_mtx);
 	}
 	MDNS_INIT_SET(ev, ev_magic);
 
 	dprintf(DEBUG_EVENT, "Event added type=%d, id=%d, ev=%x, handler=%x",
 		type, ev->ev_id, ev, handler);
+	RW_UNLOCK(ev, ev_lock);
 	return (ev->ev_id);
 }
 
@@ -408,17 +396,17 @@
 	struct event *ev = NULL;
 
 	MDNS_INIT_ASSERT(evl, evl_magic);
-	EVL_LOCK(evl);
+	MTX_LOCK(evl, evl_mtx);
 	TAILQ_FOREACH(ev, &evl->evl_events, ev_evlist) {
 		if (ev->ev_id == id)
 			break;
 	}
-	EVL_UNLOCK(evl);
+	MTX_UNLOCK(evl, evl_mtx);
 
 	if (ev == NULL) {
 		return (-1);
 	}
-	EV_WLOCK(ev);
+	RW_WLOCK(ev, ev_lock);
 	dprintf(DEBUG_EVENT, "Removing event ev=%x", ev);
 
 	if (remove_event(evl, ev, arg) == 1) {
@@ -429,7 +417,7 @@
 		 * can be removed.
 		 */
 		memcpy(&ev->ev_init_arg, arg, sizeof(ev_arg));
-		EV_UNLOCK(ev);
+		RW_UNLOCK(ev, ev_lock);
 	}
 
 	return (0);
@@ -461,9 +449,9 @@
 	}
 
 	bzero(&kev, sizeof(struct kevent));
-	EVL_LOCK(evl);
+	MTX_LOCK(evl, evl_mtx);
 	TAILQ_REMOVE(&evl->evl_events, ev, ev_evlist);
-	EVL_UNLOCK(evl);
+	MTX_UNLOCK(evl, evl_mtx);
 
 	if (arg != NULL)
 		memcpy(&ev_arg_init, arg, sizeof(ev_arg));

==== //depot/projects/soc2007/fli-mdns_sd/mdnsd/event.h#5 (text+ko) ====

@@ -30,8 +30,10 @@
 #include <stdint.h>
 #include <sys/queue.h>
 
+#include "debug.h"
+#include "threads.h"
 #include "wqueue.h"
-#include "debug.h"
+
 
 /*
  * Holds event id and backpointer to event list, included
@@ -100,9 +102,7 @@
 struct eventlist {
 	MAGIC(evl_magic);
 	TAILQ_HEAD(, event) evl_events; /* list of events */
-#ifdef HAVE_PTHREAD
-	pthread_mutex_t evl_mtx;
-#endif
+	DEF_MTX(evl_mtx);
 	int evl_len;
 	int evl_flags;
 #define EVL_FLAG_DYING	0x1
@@ -116,9 +116,7 @@
 	MAGIC(ev_magic);
 	TAILQ_ENTRY(event) ev_evlist; /* global event list */
 	struct eventlist *ev_evl;	/* back-pointer to list */
-#ifdef HAVE_PTHREAD
-	pthread_rwlock_t ev_lock;
-#endif
+	DEF_RW(ev_lock);
 	int ev_refcnt;	/* reference counter */
 	int ev_flags;
 #define EVENT_FLAG_EX		0x1 /* exclusive event (refcnt=1) */



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200707192034.l6JKYGGa040518>