From owner-svn-src-all@FreeBSD.ORG Wed Jan 22 22:15:18 2014 Return-Path: Delivered-To: svn-src-all@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 A59F0791; Wed, 22 Jan 2014 22:15:18 +0000 (UTC) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 811241971; Wed, 22 Jan 2014 22:15:18 +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 6FA371A3C26; Wed, 22 Jan 2014 14:15:16 -0800 (PST) Message-ID: <52E042F4.5080408@freebsd.org> Date: Wed, 22 Jan 2014 14:15:16 -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 Baldwin 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> In-Reply-To: <201401221622.42789.jhb@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Wed, 22 Jan 2014 22:15:18 -0000 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. I would say that maybe given this it's just better to grow WITNESS_PENDING based on maxcpu like the PR I pointed out, that way we do not introduce churn AND we maintain the debug-ability. http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/185831 -Alfred