From owner-svn-src-all@FreeBSD.ORG Thu Jan 23 19:39:25 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8868C783; Thu, 23 Jan 2014 19:39:25 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 4E08711FB; Thu, 23 Jan 2014 19:39:25 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 89D28B94F; Thu, 23 Jan 2014 14:39:21 -0500 (EST) From: John Baldwin To: Alfred Perlstein Subject: Re: svn commit: r260898 - head/sys/kern Date: Thu, 23 Jan 2014 14:29:50 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20130906; KDE/4.5.5; amd64; ; ) References: <201401200159.s0K1xa5X012123@svn.freebsd.org> <20140122225221.GV75135@funkthat.com> <52E04C40.1080303@freebsd.org> In-Reply-To: <52E04C40.1080303@freebsd.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201401231429.50980.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 23 Jan 2014 14:39:24 -0500 (EST) Cc: src-committers@freebsd.org, Scott Long , Neel Natu , John-Mark Gurney , svn-src-all@freebsd.org, Rui Paulo , svn-src-head@freebsd.org, Alexander Kabaev X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Jan 2014 19:39:25 -0000 On Wednesday, January 22, 2014 5:54:56 pm Alfred Perlstein wrote: > > On 1/22/14, 2:52 PM, John-Mark Gurney wrote: > > 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? > > > Isn't the backtrace pretty obvious given IP/PC? Erm, have you used WITNESS? It outputs the filename and line number of both locks involved in any LOR which is even more useful than a specific lock name. I highly doubt that there is any real gain from having separate lock names and types at this point. I will take a gander at removing it and seeing what the diff looks like. -- John Baldwin