svn commit: r202889 - head/sys/kern

[Attilio Rao]

2010/1/27 Marius Strobl <marius at alchemy.franken.de>:
> On Tue, Jan 26, 2010 at 08:10:25AM +0100, Attilio Rao wrote:
>> 2010/1/26 Rob Farmer <rfarmer at predatorlabs.net>:
>> > On Sat, Jan 23, 2010 at 7:54 AM, Attilio Rao <attilio at freebsd.org> wrote:
>> >> Author: attilio
>> >> Date: Sat Jan 23 15:54:21 2010
>> >> New Revision: 202889
>> >> URL: http://svn.freebsd.org/changeset/base/202889
>> >>
>> >> Log:
>> >>  - Fix a race in sched_switch() of sched_4bsd.
>> >>    In the case of the thread being on a sleepqueue or a turnstile, the
>> >>    sched_lock was acquired (without the aid of the td_lock interface) and
>> >>    the td_lock was dropped. This was going to break locking rules on other
>> >>    threads willing to access to the thread (via the td_lock interface) and
>> >>    modify his flags (allowed as long as the container lock was different
>> >>    by the one used in sched_switch).
>> >>    In order to prevent this situation, while sched_lock is acquired there
>> >>    the td_lock gets blocked. [0]
>> >>  - Merge the ULE's internal function thread_block_switch() into the global
>> >>    thread_lock_block() and make the former semantic as the default for
>> >>    thread_lock_block(). This means that thread_lock_block() will not
>> >>    disable interrupts when called (and consequently thread_unlock_block()
>> >>    will not re-enabled them when called). This should be done manually
>> >>    when necessary.
>> >>    Note, however, that ULE's thread_unblock_switch() is not reaped
>> >>    because it does reflect a difference in semantic due in ULE (the
>> >>    td_lock may not be necessarilly still blocked_lock when calling this).
>> >>    While asymmetric, it does describe a remarkable difference in semantic
>> >>    that is good to keep in mind.
>> >>
>> >>  [0] Reported by:      Kohji Okuno
>> >>                        <okuno dot kohji at jp dot panasonic dot com>
>> >>  Tested by:            Giovanni Trematerra
>> >>                        <giovanni dot trematerra at gmail dot com>
>> >>  MFC:                  2 weeks
>> >>
>> >> Modified:
>> >>  head/sys/kern/kern_mutex.c
>> >>  head/sys/kern/sched_4bsd.c
>> >>  head/sys/kern/sched_ule.c
>> >
>> > Hi,
>> >
>> > This commit seems to be causing me a kernel panic on sparc64 - details
>> > are in PR 143215. Could you take a look before MFCing this?
>>
>> I think that the bug may be in cpu_switch() where the mutex parameter
>> for sched_4bsd is not handled correctly.
>> Does sparc64 support ULE? I don't think it does and I think that it
>> simply ignores the third argument of cpu_switch() which is vital now
>> for for sched_4bsd too (what needs to happen is to take the passed
>> mutex and to set the TD_LOCK of old thread to be the third argument).
>> Unluckilly, I can't do that in sparc64 asm right now, but it should
>> not be too difficult to cope with it.
>>
>
> The following patch adds handling of the mutex parameter to the
> sparc64 cpu_switch():
> http://people.freebsd.org/~marius/sparc64_cpu_switch_mtx.diff
> This patch works fine with r202888. With r202889 it allows the
> machine to boot again, however putting some load on the machine
> causes it to issue a reset without a chance to debug. I've also
> tried with some variations like duplicating the old cpu_switch()
> for cpu_throw() so the altered cpu_switch() doesn't need to
> distinguish between the to cases and only assigning old->td_lock
> right before return but nothing made a difference. Given that
> this leaves little room for a bug in the cpu_switch() changes I
> suspect r202889 also breaks additional assumptions. For example
> the sparc64 pmap code used sched_lock, does that need to change
> to be td_lock now maybe? Is there anything else that comes to
> your mind in this regard?

Sorry for being lame with sparc64 assembly (so that I can't make much
more productive help here), but the required patch, sched_4bsd only,
should simply save the extra-argument of cpu_switch() (and cpu_throw()
is not involved, so I'm not sure what is changing there) and move in
TD_LOCK(%oldthreadreg) when it is safe to do (just after the oldthread
switched out completely). It doesn't even require a memory barrier.
This patch seems a bit too big and I wonder what else it does (or I'm
entirely wrong and that's just what I asked here), maybe adding the
ULE support as well?

Said that, all the code, including MD parts should always use
td_lock() and not doing explicit acquisitions/drops of sched_lock, if
they want to support ULE (but probabilly even if they do not want),
unless there is a big compelling reason (that I expect to be justified
in comments at least).
I'm not sure how to debug a possible reset or I'm not aware of further
broken asserts, at least for tier-1 architectures.

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein