svn commit: r260898 - head/sys/kern

[Alfred Perlstein]

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