Date: Thu, 16 Aug 2007 17:41:17 GMT From: Marko Zec <zec@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 125224 for review Message-ID: <200708161741.l7GHfHfv001084@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=125224 Change 125224 by zec@zec_tpx32 on 2007/08/16 17:40:55 Initial attempt at locking of the global vnet list. The global vnet list is quite static, i.e. it is being read-only traversed relatively often, mostly from timers, currently at around 110 times per second regardless of HZ setting; will be more in HZ range once dummynet gets loaded. It only needs to be updated when new vnets are created or existing ones are deleted. Hence, the vnet list might look like an ideal candidate for read-often write-seldom type of locks, such as sx(9) or rwlock(9). However, so far my experiments with locking the vnet list while processing timers using sx, rwlock, or default mutexes, have been futile; each variant leading to a different LOR storm and / or to temporary system lockups. Therefore I'm attempting to protect the vnet list using a handcrafted shared / exclusive locking scheme... The basic idea is to allow shared read-only access to the vnet list using a refcounted scheme, with the refcount itself being protected by a mutex, while for granting exclusive access the shared refcount must be zero and the mutex must be held during the entire critical section. I.e. the mutex is not held during the shared access sections, but only while bumping the refcount. A condvar(9) is used by read-only threads to wake up the thread(s) waiting to enter an exclusively-locked section. The extra overhead this scheme introduces is that for each read-only section we need two mtx_lock() and two mtx_unlock() calls plus one cv_signal() invocation. This mechanism has yet to be stress-tested on a SMP machine. Given that those are basically my first steps in the strange world of kernel-level multithreading, any comments from more knowledgeable people would be much appreciated... Affected files ... .. //depot/projects/vimage/src/sys/kern/kern_vimage.c#34 edit .. //depot/projects/vimage/src/sys/sys/vimage.h#34 edit Differences ... ==== //depot/projects/vimage/src/sys/kern/kern_vimage.c#34 (text+ko) ==== @@ -33,31 +33,19 @@ #include "opt_ddb.h" #include "opt_vimage.h" -#include <sys/types.h> #include <sys/param.h> -#include <sys/systm.h> +#include <sys/kernel.h> +#include <sys/linker.h> #include <sys/malloc.h> -#include <sys/domain.h> -#include <sys/protosw.h> -#include <sys/kernel.h> -#include <sys/sysproto.h> -#include <sys/sysent.h> #include <sys/sockio.h> -#include <sys/proc.h> -#include <sys/linker.h> -#include <sys/queue.h> -#include <sys/socketvar.h> -#include <sys/sysctl.h> #include <sys/priv.h> #include <sys/vimage.h> -#include <sys/vmmeter.h> #ifdef DDB #include <ddb/ddb.h> #endif #include <net/vnet.h> -#include <net/bpf.h> #include <net/if_types.h> #include <net/if_dl.h> #include <net/if_clone.h> @@ -121,6 +109,21 @@ struct vprocg_list_head vprocg_head; struct vcpu_list_head vcpu_head; +struct cv vnet_list_condvar; +struct mtx vnet_list_refc_mtx; +int vnet_list_refc = 0; + +#define VNET_LIST_LOCK() \ + mtx_lock(&vnet_list_refc_mtx); \ + while (vnet_list_refc != 0) { \ + cv_wait(&vnet_list_condvar, &vnet_list_refc_mtx); \ + printf("XXX vnet_list_refc = %d in %s\n", \ + vnet_list_refc, __FUNCTION__); \ + } + +#define VNET_LIST_UNLOCK() \ + mtx_unlock(&vnet_list_refc_mtx); + static int last_vi_id = 0; static TAILQ_HEAD(vnet_modlink_head, vnet_modlink) vnet_modlink_head; @@ -430,6 +433,7 @@ if (error) return (error); + VNET_LIST_LOCK(); /* XXX should lock vimage list... */ if (strlen(vi_req->vi_name)) { LIST_FOREACH(tvip, &vimage_head, vi_le) if (strcmp(vi_req->vi_name, tvip->vi_name)==0) { @@ -437,9 +441,11 @@ break; } if (vip_r == NULL && !(vi_req->req_action & VI_CREATE)) { + VNET_LIST_UNLOCK(); /* XXX */ return (ESRCH); } if (vip_r != NULL && vi_req->req_action & VI_CREATE) { + VNET_LIST_UNLOCK(); /* XXX */ return (EADDRINUSE); } if (vi_req->req_action == VI_GETNEXT) { @@ -447,6 +453,7 @@ if ((vip_r = LIST_NEXT(vip_r, vi_le)) == 0) vip_r = LIST_FIRST(&vimage_head); if (vip_r == vip) { + VNET_LIST_UNLOCK(); /* XXX */ return (ESRCH); } if (!vi_child_of(vip, vip_r)) @@ -455,6 +462,7 @@ } else vip_r = vip; + VNET_LIST_UNLOCK(); /* XXX */ if (vip_r && !vi_child_of(vip, vip_r) && vi_req->req_action != VI_GET && vi_req->req_action != VI_GETNEXT) @@ -515,9 +523,8 @@ vip_r->vi_parent = vip; } - if (vip == vip_r && vip != &vimage_0) { + if (vip == vip_r && vip != &vimage_0) return (EPERM); - } } return (error); @@ -606,10 +613,12 @@ CURVNET_RESTORE(); - LIST_INSERT_HEAD(&vimage_head, vip, vi_le); + VNET_LIST_LOCK(); /* XXX should lock other lists separately */ LIST_INSERT_HEAD(&vnet_head, vnet, vnet_le); LIST_INSERT_HEAD(&vprocg_head, vprocg, vprocg_le); LIST_INSERT_HEAD(&vcpu_head, vcpu, vcpu_le); + LIST_INSERT_HEAD(&vimage_head, vip, vi_le); + VNET_LIST_UNLOCK(); vi_alloc_done: return (vip); @@ -630,8 +639,9 @@ struct ifnet *ifp, *nifp; struct vnet_modlink *vml; - /* XXX should have the vnet list locked here!!! */ + VNET_LIST_LOCK(); LIST_REMOVE(vnet, vnet_le); + VNET_LIST_UNLOCK(); CURVNET_SET_QUIET(vnet); INIT_VNET_NET(vnet); @@ -663,6 +673,7 @@ vnet->vnet_magic_n = -1; vi_free(vnet, M_VNET); + /* XXX lock those bellow... */ LIST_REMOVE(vprocg, vprocg_le); vi_free(vprocg, M_VPROCG); @@ -772,6 +783,9 @@ vnet_0.vnet_magic_n = VNET_MAGIC_N; + mtx_init(&vnet_list_refc_mtx, "vnet_list_refc_mtx", NULL, MTX_DEF); + cv_init(&vnet_list_condvar, "vnet_list_condvar"); + /* We MUST clear curvnet in vi_init_done before going SMP. */ curvnet = &vnet_0; } ==== //depot/projects/vimage/src/sys/sys/vimage.h#34 (text+ko) ==== @@ -30,17 +30,13 @@ * XXX RCS tag goes here */ - #ifndef _NET_VIMAGE_H_ #define _NET_VIMAGE_H_ - -#include <sys/resource.h> -#include <sys/dkstat.h> -#include <sys/msgbuf.h> -#include <sys/select.h> -#include <sys/callout.h> +#include <sys/lock.h> #include <sys/proc.h> +#include <sys/condvar.h> +#include <sys/mutex.h> #ifdef INVARIANTS #define VNET_DEBUG @@ -218,20 +214,20 @@ #define VNET_ITERLOOP_BEGIN() \ struct vnet *vnet_iter; \ - /* XXX LOCK vnet list */ \ + VNET_LIST_REF(); \ LIST_FOREACH(vnet_iter, &vnet_head, vnet_le) { \ CURVNET_SET(vnet_iter); #define VNET_ITERLOOP_BEGIN_QUIET() \ struct vnet *vnet_iter; \ - /* XXX LOCK vnet list */ \ + VNET_LIST_REF(); \ LIST_FOREACH(vnet_iter, &vnet_head, vnet_le) { \ CURVNET_SET_QUIET(vnet_iter); #define VNET_ITERLOOP_END() \ CURVNET_RESTORE(); \ } \ - /* XXX UNLOCK vnet list */ + VNET_LIST_UNREF(); #else /* !VNET_DEBUG */ @@ -252,7 +248,7 @@ #define VNET_ITERLOOP_BEGIN() \ struct vnet *vnet_iter; \ - /* XXX LOCK vnet list */ \ + VNET_LIST_REF(); \ LIST_FOREACH(vnet_iter, &vnet_head, vnet_le) { \ CURVNET_SET(vnet_iter); @@ -261,7 +257,7 @@ #define VNET_ITERLOOP_END() \ CURVNET_RESTORE(); \ } \ - /* XXX UNLOCK vnet list */ + VNET_LIST_UNREF(); #endif /* !VNET_DEBUG */ @@ -326,7 +322,24 @@ extern struct vnet vnet_0; LIST_HEAD(vnet_list_head, vnet); extern struct vnet_list_head vnet_head; +extern int vnet_list_refc; +extern struct mtx vnet_list_refc_mtx; +extern struct cv vnet_list_condvar; +#define VNET_LIST_REF() \ + mtx_lock(&vnet_list_refc_mtx); \ + vnet_list_refc++; \ + if (vnet_list_refc > 1) \ + printf ("XXX vnet_list_refc = %d in %s\n", \ + vnet_list_refc, __FUNCTION__); \ + mtx_unlock(&vnet_list_refc_mtx); + +#define VNET_LIST_UNREF() \ + mtx_lock(&vnet_list_refc_mtx); \ + vnet_list_refc--; \ + mtx_unlock(&vnet_list_refc_mtx); \ + cv_signal(&vnet_list_condvar); + #define IS_VNET_0(arg) ((arg) == &vnet_0 ? 1 : 0) /* @@ -345,7 +358,6 @@ struct vnet *v_vnet; }; - struct vprocg { LIST_ENTRY(vprocg) vprocg_le; @@ -383,7 +395,6 @@ #endif }; - struct vcpu { LIST_ENTRY(vcpu) vcpu_le;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200708161741.l7GHfHfv001084>