From owner-svn-src-head@FreeBSD.ORG Wed Jan 22 22:54:57 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 B866B717; Wed, 22 Jan 2014 22:54:57 +0000 (UTC) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 9667C1D77; Wed, 22 Jan 2014 22:54:57 +0000 (UTC) Received: from Alfreds-MacBook-Pro.local (c-71-202-25-4.hsd1.ca.comcast.net [71.202.25.4]) by elvis.mu.org (Postfix) with ESMTPSA id 646201A3C25; Wed, 22 Jan 2014 14:54:56 -0800 (PST) Message-ID: <52E04C40.1080303@freebsd.org> Date: Wed, 22 Jan 2014 14:54:56 -0800 From: Alfred Perlstein Organization: FreeBSD User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: John-Mark Gurney Subject: Re: svn commit: r260898 - head/sys/kern References: <201401200159.s0K1xa5X012123@svn.freebsd.org> <201401221527.12779.jhb@freebsd.org> <52E03139.2020902@freebsd.org> <201401221622.42789.jhb@freebsd.org> <52E042F4.5080408@freebsd.org> <20140122225221.GV75135@funkthat.com> In-Reply-To: <20140122225221.GV75135@funkthat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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:54:57 -0000 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? -Alfred