svn commit: r260898 - head/sys/kern

[John-Mark Gurney]

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?

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."