Date: Thu, 23 Jan 2014 14:29:50 -0500 From: John Baldwin <jhb@freebsd.org> To: Alfred Perlstein <alfred@freebsd.org> Cc: src-committers@freebsd.org, Scott Long <scott4long@yahoo.com>, Neel Natu <neel@freebsd.org>, John-Mark Gurney <jmg@funkthat.com>, 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: <201401231429.50980.jhb@freebsd.org> In-Reply-To: <52E04C40.1080303@freebsd.org> References: <201401200159.s0K1xa5X012123@svn.freebsd.org> <20140122225221.GV75135@funkthat.com> <52E04C40.1080303@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201401231429.50980.jhb>