Date: Fri, 21 Feb 2003 09:42:51 -0800 (PST) From: John Baldwin <jhb@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 25536 for review Message-ID: <200302211742.h1LHgptx095984@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=25536 Change 25536 by jhb@jhb_laptop on 2003/02/21 09:41:54 Real eventhandler locking without evilness. Highlights: - Uses mutex rather than a sx lock for each list. - We use a runcount that we increment when starting to invoke a list and decrement when finished invoking a list. - When deleteing a handler, if the runcount is not 0, we mark the event as dead using a reserved priority value (-1) rather than removing it from the list right then and there. - When we finish invoking a list and the runcount goes down to 0, we prune the list of any dead handlers. - A bit more sane locking is used. - All the %^&#ing code duplication in the invoke methods is gone by having the two invoke macros lookup and lock the list before calling a common macro. - The pruning of the list is a function so as to not bloat the invoke macros too much. Affected files ... .. //depot/projects/smpng/sys/kern/subr_eventhandler.c#9 edit .. //depot/projects/smpng/sys/sys/eventhandler.h#9 edit Differences ... ==== //depot/projects/smpng/sys/kern/subr_eventhandler.c#9 (text+ko) ==== @@ -55,8 +55,8 @@ eventhandler_init(void *dummy __unused) { TAILQ_INIT(&eventhandler_lists); - mtx_init(&eventhandler_mutex, "eventhandler", NULL, MTX_DEF | MTX_RECURSE); - eventhandler_lists_initted = 1; + mtx_init(&eventhandler_mutex, "eventhandler", NULL, MTX_DEF); + atomic_store_rel_int(&eventhandler_lists_initted, 1); } SYSINIT(eventhandlers, SI_SUB_EVENTHANDLER, SI_ORDER_FIRST, eventhandler_init, NULL) @@ -80,43 +80,49 @@ /* Do we need to find/create the (slow) list? */ if (list == NULL) { /* look for a matching, existing list */ - list = eventhandler_find_list(name); + list = _eventhandler_find_list(name); /* Do we need to create the list? */ if (list == NULL) { - if ((list = malloc(sizeof(struct eventhandler_list) + strlen(name) - + 1, M_EVENTHANDLER, M_NOWAIT)) == NULL) { - mtx_unlock(&eventhandler_mutex); - return(NULL); + mtx_unlock(&eventhandler_mutex); + + new_list = malloc(sizeof(struct eventhandler_list) + + strlen(name) + 1, M_EVENTHANDLER, M_WAITOK); + + /* If someone else created it already, then use that one. */ + mtx_lock(&eventhandler_mutex); + list = _eventhandler_find_list(name); + if (list != NULL) { + free(new_list, M_EVENTHANDLER); + } else { + list = new_list; + list->el_flags = 0; + bzero(&list->el_lock, sizeof(list->el_lock)); + list->el_name = (char *)list + sizeof(struct eventhandler_list); + strcpy(list->el_name, name); + TAILQ_INSERT_HEAD(&eventhandler_lists, list, el_link); } - list->el_flags = 0; - bzero(&list->el_lock, sizeof(list->el_lock)); - list->el_name = (char *)list + sizeof(struct eventhandler_list); - strcpy(list->el_name, name); - TAILQ_INSERT_HEAD(&eventhandler_lists, list, el_link); } } - if (!(list->el_flags & EHE_INITTED)) { + if (!(list->el_flags & EHL_INITTED)) { TAILQ_INIT(&list->el_entries); - sx_init(&list->el_lock, name); - list->el_flags = EHE_INITTED; + mtx_init(&list->el_lock, name, "eventhandler list", MTX_DEF); + atomic_store_rel_int(&list->el_flags, EHL_INITTED); } mtx_unlock(&eventhandler_mutex); - + /* allocate an entry for this handler, populate it */ - if ((eg = malloc(sizeof(struct eventhandler_entry_generic), - M_EVENTHANDLER, M_NOWAIT)) == NULL) { - return(NULL); - } + eg = malloc(sizeof(struct eventhandler_entry_generic), M_EVENTHANDLER, + M_WAITOK | M_ZERO); eg->func = func; eg->ee.ee_arg = arg; eg->ee.ee_priority = priority; - + KASSERT(priority != EHE_DEAD_PRIORITY, + ("%s: handler for %s registered with dead priority", __func__, name)); + /* sort it into the list */ - EHE_LOCK(list); - for (ep = TAILQ_FIRST(&list->el_entries); - ep != NULL; - ep = TAILQ_NEXT(ep, ee_link)) { + EHL_LOCK(list); + TAILQ_FOREACH(ep, &list->el_entries, ee_link) { if (eg->ee.ee_priority < ep->ee_priority) { TAILQ_INSERT_BEFORE(ep, &eg->ee, ee_link); break; @@ -124,7 +130,7 @@ } if (ep == NULL) TAILQ_INSERT_TAIL(&list->el_entries, &eg->ee, ee_link); - EHE_UNLOCK(list); + EHL_UNLOCK(list); return(&eg->ee); } @@ -133,23 +139,49 @@ { struct eventhandler_entry *ep = tag; - /* XXX insert diagnostic check here? */ - EHE_LOCK(list); + EHL_LOCK_ASSERT(list, MA_OWNED); if (ep != NULL) { /* remove just this entry */ - TAILQ_REMOVE(&list->el_entries, ep, ee_link); - free(ep, M_EVENTHANDLER); + if (list->el_runcount == 0) { + TAILQ_REMOVE(&list->el_entries, ep, ee_link); + free(ep, M_EVENTHANDLER); + } else + ep->ee_priority = EHE_DEAD_PRIORITY; } else { /* remove entire list */ - while (!TAILQ_EMPTY(&list->el_entries)) { - ep = TAILQ_FIRST(&list->el_entries); - TAILQ_REMOVE(&list->el_entries, ep, ee_link); - free(ep, M_EVENTHANDLER); + if (list->el_runcount == 0) { + while (!TAILQ_EMPTY(&list->el_entries)) { + ep = TAILQ_FIRST(&list->el_entries); + TAILQ_REMOVE(&list->el_entries, ep, ee_link); + free(ep, M_EVENTHANDLER); + } + } else { + TAILQ_FOREACH(ep, &list->el_entries, ee_link) + ep->ee_priority = EHE_DEAD_PRIORITY; } } - EHE_UNLOCK(list); + EHL_UNLOCK(list); +} + +/* + * Internal version for use when eventhandler list is already locked. + */ +static struct eventhandler_list * +_eventhandler_find_list(char *name) +{ + struct eventhandler_list *list; + + mtx_assert(&eventhandler_mutex, MA_OWNED); + TAILQ_FOREACH(list, &eventhandler_lists, el_link) { + if (!strcmp(name, list->el_name)) + break; + } + return (list); } +/* + * Lookup a "slow" list by name. Returns with the list locked. + */ struct eventhandler_list * eventhandler_find_list(char *name) { @@ -160,14 +192,30 @@ /* scan looking for the requested list */ mtx_lock(&eventhandler_mutex); - for (list = TAILQ_FIRST(&eventhandler_lists); - list != NULL; - list = TAILQ_NEXT(list, el_link)) { - if (!strcmp(name, list->el_name)) - break; - } + list = _eventhandler_find_list(name); + EHL_LOCK(list); mtx_unlock(&eventhandler_mutex); return(list); } +/* + * Prune "dead" entries from an eventhandler list. + */ +void +eventhandler_prune_list(struct eventhandler_list *list) +{ + struct eventhandler_entry *ep, *en; + + EHL_LOCK_ASSERT(list, MA_OWNED); + ep = TAILQ_FIRST(&list->el_entries)); + while (ep != NULL) { + en = TAILQ_NEXT(ep, ee_link); + if (ep->ee_priority == EHE_DEAD_PRIORITY) { + TAILQ_REMOVE(&list->el_entries, ep, ee_link); + free(ep, M_EVENTHANDLER); + } + ep = en; + } +} + ==== //depot/projects/smpng/sys/sys/eventhandler.h#9 (text+ko) ==== @@ -30,28 +30,59 @@ #define SYS_EVENTHANDLER_H #include <sys/lock.h> -#include <sys/sx.h> +#include <sys/mutex.h> #include <sys/queue.h> struct eventhandler_entry { TAILQ_ENTRY(eventhandler_entry) ee_link; int ee_priority; +#define EHE_DEAD_PRIORITY (-1) void *ee_arg; }; struct eventhandler_list { char *el_name; int el_flags; -#define EHE_INITTED (1<<0) - struct sx el_lock; +#define EHL_INITTED (1<<0) + u_int el_runcount; + struct mtx el_lock; TAILQ_ENTRY(eventhandler_list) el_link; TAILQ_HEAD(,eventhandler_entry) el_entries; }; typedef struct eventhandler_entry *eventhandler_tag; -#define EHE_LOCK(p) sx_xlock(&(p)->el_lock) -#define EHE_UNLOCK(p) sx_xunlock(&(p)->el_lock) +#define EHL_LOCK(p) mtx_lock(&(p)->el_lock) +#define EHL_UNLOCK(p) mtx_unlock(&(p)->el_lock) +#define EHL_LOCK_ASSERT(p, x) mtx_assert(&(p)->el_lock, x) + +/* + * Macro to invoke the handlers for a given event. + */ +#define _EVENTHANDLER_INVOKE(list, ...) do { \ + struct eventhandler_entry *_ep, *_en; \ + \ + KASSERT((list)->el_flags & EHL_INITTED, \ + ("eventhandler_invoke: running non-inited list")); \ + EHL_LOCK_ASSERT((list), MA_OWNED); \ + (list)->el_runcount++; \ + KASSERT((list)->el_runcount > 0, \ + ("eventhandler_invoke: runcount overflow")); \ + TAILQ_FOREACH(_ep, &((list)->el_entries), ee_link) { \ + if (!(_ep->ee_priority != EHE_DEAD_PRIORITY)) { \ + EHL_UNLOCK((list)); \ + _ep->eh_func(_ep->ee_arg , __VA_ARGS__); \ + EHL_LOCK((list)); \ + } \ + } \ + KASSERT((list)->el_runcount > 0, \ + ("eventhandler_invoke: runcount underflow")); \ + (list)->el_runcount--; \ + if ((list)->el_runcount == 0) \ + eventhandler_prune_list(list); \ + EHL_UNLOCK((list)); \ +} while (0) + /* * Fast handler lists require the eventhandler list be present @@ -75,22 +106,12 @@ struct eventhandler_list Xeventhandler_list_ ## name = { #name }; \ struct __hack -#define EVENTHANDLER_FAST_INVOKE(name, ...) \ -do { \ +#define EVENTHANDLER_FAST_INVOKE(name, ...) do { \ struct eventhandler_list *_el = &Xeventhandler_list_ ## name ; \ - struct eventhandler_entry *_ep, *_en; \ - struct eventhandler_entry_ ## name *_t; \ \ - if (_el->el_flags & EHE_INITTED) { \ - EHE_LOCK(_el); \ - _ep = TAILQ_FIRST(&(_el->el_entries)); \ - while (_ep != NULL) { \ - _en = TAILQ_NEXT(_ep, ee_link); \ - _t = (struct eventhandler_entry_ ## name *)_ep; \ - _t->eh_func(_ep->ee_arg , __VA_ARGS__); \ - _ep = _en; \ - } \ - EHE_UNLOCK(_el); \ + if (_el->el_flags & EHL_INITTED) { \ + EHL_LOCK(_el); \ + _EVENTHANDLER_INVOKE(_el, __VA_ARGS__); \ } \ } while (0) @@ -98,8 +119,14 @@ eventhandler_register(&Xeventhandler_list_ ## name, \ #name, func, arg, priority) -#define EVENTHANDLER_FAST_DEREGISTER(name, tag) \ - eventhandler_deregister(&Xeventhandler_list_ ## name, tag) +#define EVENTHANDLER_FAST_DEREGISTER(name, tag) do { \ + struct eventhandler_list *_el = &Xeventhandler_list_ ## name ; \ + \ + KASSERT(_el->el_flags & EHL_INITTED, \ + ("eventhandler_fast_deregister on un-inited list %s", ## name)); \ + EHL_LOCK(el); \ + eventhandler_deregister(_el, tag); \ +} while (0) /* * Slow handlers are entirely dynamic; lists are created @@ -119,21 +146,9 @@ #define EVENTHANDLER_INVOKE(name, ...) \ do { \ struct eventhandler_list *_el; \ - struct eventhandler_entry *_ep, *_en; \ - struct eventhandler_entry_ ## name *_t; \ \ - if (((_el = eventhandler_find_list(#name)) != NULL) && \ - (_el->el_flags & EHE_INITTED)) { \ - EHE_LOCK(_el); \ - _ep = TAILQ_FIRST(&(_el->el_entries)); \ - while (_ep != NULL) { \ - _en = TAILQ_NEXT(_ep, ee_link); \ - _t = (struct eventhandler_entry_ ## name *)_ep; \ - _t->eh_func(_ep->ee_arg , __VA_ARGS__); \ - _ep = _en; \ - } \ - EHE_UNLOCK(_el); \ - } \ + if ((_el = eventhandler_find_list(#name)) != NULL) \ + _EVENTHANDLER_INVOKE(el, __VA_ARGS__); \ } while (0) #define EVENTHANDLER_REGISTER(name, func, arg, priority) \ @@ -148,14 +163,12 @@ } while(0) -extern eventhandler_tag eventhandler_register(struct eventhandler_list *list, - char *name, - void *func, - void *arg, - int priority); -extern void eventhandler_deregister(struct eventhandler_list *list, - eventhandler_tag tag); -extern struct eventhandler_list *eventhandler_find_list(char *name); +eventhandler_tag eventhandler_register(struct eventhandler_list *list, + char *name, void *func, void *arg, int priority); +void eventhandler_deregister(struct eventhandler_list *list, + eventhandler_tag tag); +struct eventhandler_list *eventhandler_find_list(char *name); +void eventhandler_prune_list(struct eventhandler_list *list); /* * Standard system event queues. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe p4-projects" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200302211742.h1LHgptx095984>