Date: Wed, 22 Jan 2014 14:54:56 -0800 From: Alfred Perlstein <alfred@freebsd.org> To: John-Mark Gurney <jmg@funkthat.com> Cc: src-committers@FreeBSD.org, Scott Long <scott4long@yahoo.com>, Neel Natu <neel@FreeBSD.org>, John Baldwin <jhb@FreeBSD.org>, svn-src-all@FreeBSD.org, Rui Paulo <rpaulo@felyko.com>, svn-src-head@FreeBSD.org, Alexander Kabaev <kabaev@gmail.com> Subject: Re: svn commit: r260898 - head/sys/kern Message-ID: <52E04C40.1080303@freebsd.org> In-Reply-To: <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> <20140122225221.GV75135@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52E04C40.1080303>