From owner-svn-src-head@FreeBSD.ORG Wed Jan 22 22:52:31 2014 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 9596E5D3; Wed, 22 Jan 2014 22:52:31 +0000 (UTC) Received: from h2.funkthat.com (gate2.funkthat.com [208.87.223.18]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 6A6911D70; Wed, 22 Jan 2014 22:52:31 +0000 (UTC) Received: from h2.funkthat.com (localhost [127.0.0.1]) by h2.funkthat.com (8.14.3/8.14.3) with ESMTP id s0MMqL4k065209 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 22 Jan 2014 14:52:22 -0800 (PST) (envelope-from jmg@h2.funkthat.com) Received: (from jmg@localhost) by h2.funkthat.com (8.14.3/8.14.3/Submit) id s0MMqLAR065208; Wed, 22 Jan 2014 14:52:21 -0800 (PST) (envelope-from jmg) Date: Wed, 22 Jan 2014 14:52:21 -0800 From: John-Mark Gurney To: Alfred Perlstein Subject: Re: svn commit: r260898 - head/sys/kern Message-ID: <20140122225221.GV75135@funkthat.com> References: <201401200159.s0K1xa5X012123@svn.freebsd.org> <201401221527.12779.jhb@freebsd.org> <52E03139.2020902@freebsd.org> <201401221622.42789.jhb@freebsd.org> <52E042F4.5080408@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52E042F4.5080408@freebsd.org> User-Agent: Mutt/1.4.2.3i X-Operating-System: FreeBSD 7.2-RELEASE i386 X-PGP-Fingerprint: 54BA 873B 6515 3F10 9E88 9322 9CB1 8F74 6D3F A396 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? can i haz chizburger? X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.2.2 (h2.funkthat.com [127.0.0.1]); Wed, 22 Jan 2014 14:52:22 -0800 (PST) Cc: src-committers@FreeBSD.org, Scott Long , Neel Natu , John Baldwin , svn-src-all@FreeBSD.org, Rui Paulo , svn-src-head@FreeBSD.org, Alexander Kabaev X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2014 22:52:31 -0000 Alfred Perlstein wrote this message on Wed, Jan 22, 2014 at 14:15 -0800: > > On 1/22/14, 1:22 PM, John Baldwin wrote: > >On Wednesday, January 22, 2014 3:59:37 pm Alfred Perlstein wrote: > >>On 1/22/14, 12:27 PM, John Baldwin wrote: > >>>On Wednesday, January 22, 2014 2:06:39 pm Alfred Perlstein wrote: > >>>>Hmm, what if locks had a pointer to a 2 element char * array, the first > >>>>being the name, the second the type. That would keep the size of the > >>>>lock down and most locks could share a common tuple of name/type in each > >>>>subsystem. This would allow us to get rid of the pending static list. > >>>> > >>>>effectively: > >>>>struct lock_object { > >>>> char *lo_name; /* Individual lock name. */ > >>>> u_int lo_flags; > >>>> u_int lo_data; /* General class specific > >>>> data. > >*/ > >>>> struct witness *lo_witness; /* Data for witness. */ > >>>>}; > >>>> > >>>>would change to: > >>>>struct lock_object { > >>>> char **lo_name_type; /* Individual lock > >>>>name[0]/type[1]. */ > >>>> u_int lo_flags; > >>>> u_int lo_data; /* General class specific > >>>> data. > >*/ > >>>> struct witness *lo_witness; /* Data for witness. */ > >>>>}; > >>>> > >>>>This may be somewhat disruptive, I haven't played with how it would > >>>>actually change driver/etc/code. > >>>Where would the memory for the char* array come from? > >>> > >>That is a good question. I suspect it would be up to the subsystem to > >>allocate it. > >> > >>Wouldn't it be trivial for *most* of the subsystems to simply have this > >>either as a static global or static function variable: > >> > >>static char *mutex_typename = { "kqueue", "foo" }; > >> > >>Under kern I see this: > >>grep mtx_init * | grep -v NULL > >>... > >>kern_rmlock.c: mtx_init(&rm->rm_lock_mtx, name, "rmlock_mtx", > >>MTX_NOWITNESS); > >>subr_bus.c: mtx_init(&devsoftc.mtx, "dev mtx", "devd", MTX_DEF); > >> > >>Those are solved with statics. > >> > >>Another example: > >> > >>sys/dev/ae/if_ae.c > >> mtx_init(&sc->mtx, device_get_nameunit(dev), MTX_NETWORK_LOCK, > >>MTX_DEF); > >> > >>I think the array could be in the softc here? sc->mutex_name_type[0] = > >>device_get_nameunit(dev); sc->mutex_name_type[1] = MTX_NETWORK_LOCK; > >> > >>Do we want to do that? It moves "wasting space" to another variable. > >> > >>I'm not sure where there isn't the possibility of using either static > >>(for a global mutex) or space inside the equiv of the softc (or proc or > >>whatever) for this? > >> > >>I'm not sure this is a good idea, just an idea. Are there places where > >>it's not as simple as doing this? > >To be honest, the whole name vs type thing isn't widely used, and it makes > >the mtx_init() function kind of fugly. I think what I would actually > >prefer > >is to just kill it, changing the various places that pass a separate name > >to > >just pass the type instead. Note that none of the other lock APIs even > >allow > >setting a separate type. This would let us remove the static pending list > >array as well. > > > >(And yes, I added the name vs type thing, but at this point I think it did > >not turn out nearly as useful as I had thought it would be.) > > > >The original issue of picking useful-to-witness lock names (i.e. not just > >using device_get_nameunit()) still remains of course. > > > I really want to agree, but anything that reduces the immediate ability > for people to diagnose problems really makes me worry. > > This would mean that you would see "network device lock" or some "type" > but not know the actual owner. isn't it usually apparent which lock it is from the backtrace? -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."