svn commit: r212964 - head/sys/kern

[mdf at FreeBSD.org]

On Tue, Sep 21, 2010 at 9:50 AM, John Baldwin <jhb at freebsd.org> wrote:
> On Tuesday, September 21, 2010 12:31:01 pm mdf at freebsd.org wrote:
>> On Tue, Sep 21, 2010 at 8:40 AM, Andriy Gapon <avg at freebsd.org> wrote:
>> > on 21/09/2010 18:27 Andriy Gapon said the following:
>> >> on 21/09/2010 18:17 mdf at FreeBSD.org said the following:
>> >>>
>> >>> I'd recommend using stack_print_ddb(), as that avoids any locking
>> >>> which may hang depending on how the kernel panic'd.
>> >>
>> >> It seems that stack_print_ddb() depends on DDB?
>> >
>> > But the point about locking is very good.
>> > How do you suggest we can deal with it?
>> >
>> > A dirty hack would be to check panicstr in linker_search_symbol_name and avoid
>> > locking, but I don't like that at all.
>> > Perhaps, some code in subr_stack.c could be taken outside DDB ifdef?
>>
>> I keep forgetting, but actually _mtx_lock_sleep() will just return if
>> panicstr is set.  _mtx_assert() is similarly guarded, so actually it
>> should be mostly okay.
>
> Err, I don't think _mtx_lock_sleep() is guarded in that fashion?  I have an
> old patch to do that but have never committed it.  If we want that we should
> probably change rwlocks and sxlocks to have also not block when panicstr is
> set.

D'oh!  Once again I was looking at Isilon source but not CURRENT.  We
had patches for mtx (back on 6.1) and haven't updated them for sx/rw
for 7.  We're also running with local patches to stop CPUs in panic()
that Mr Jacob has a copy of.

Regarding the original issue, then, I think in the short term using a
safe stack_print() would be the correct thing.  Changing the locking
and stop_cpus logic will not happen in the next week. :-)

Thanks,
matthew

>
> --- //depot/vendor/freebsd/src/sys/kern/kern_mutex.c    2010-05-11 18:31:25.000000000 0000
> +++ //depot/projects/smpng/sys/kern/kern_mutex.c        2010-06-01 20:12:02.000000000 0000
> @@ -348,6 +348,15 @@
>                return;
>        }
>
> +       /*
> +        * If we have already panic'd and this is the thread that called
> +        * panic(), then don't block on any mutexes but silently succeed.
> +        * Otherwise, the kernel will deadlock since the scheduler isn't
> +        * going to run the thread that holds the lock we need.
> +        */
> +       if (panicstr != NULL && curthread->td_flags & TDF_INPANIC)
> +               return;
> +
>        lock_profile_obtain_lock_failed(&m->lock_object,
>                    &contested, &waittime);
>        if (LOCK_LOG_TEST(&m->lock_object, opts))
> @@ -664,6 +673,15 @@
>        }
>
>        /*
> +        * If we failed to unlock this lock and we are a thread that has
> +        * called panic(), it may be due to the bypass in _mtx_lock_sleep()
> +        * above.  In that case, just return and leave the lock alone to
> +        * avoid changing the state.
> +        */
> +       if (panicstr != NULL && curthread->td_flags & TDF_INPANIC)
> +               return;
> +
> +       /*
>         * We have to lock the chain before the turnstile so this turnstile
>         * can be removed from the hash list if it is empty.
>         */
>
> --
> John Baldwin
>