Re: svn commit: r202889 - head/sys/kern
2010/1/30 Marius Strobl mar...@alchemy.franken.de:
On Sat, Jan 30, 2010 at 04:22:47PM +0100, Attilio Rao wrote:
2010/1/30 Marius Strobl mar...@alchemy.franken.de:
I think the idea behind using sched_lock in the sparc64 code is
to piggyback on it for protecting the pmap and take advantage of
the fact that it's held across cpu_switch() anyway. If that's
correct it should be possible to replace it with a separate
spinlock dedicated to protecting the pmap or given that due to
the macro madness involved in mtx_{,un}lock_spin() it's hard to
properly call these from asm by calling spinlock_{enter,exit}()
directly.
Even if that is the case there is no reason to not call
thread_lock()/thread_unlock() (which will acquire the correct
sched_lock or do the handover in the sched_4bsd in the right way) and
keep an unified spinlock in order to keep cpu_switch() simple and
still offering pmap protection over context switches.
It's not clear to me what threads to use in order to get this
to work the right way, for pmap_release() I'd use curthread and
for pmap_activate() I'd just pass td on to thread_{,un}lock()?
Whould the general approach of using thread_{,un}lock() for pmap
protection also do the right thing in case of ULE?
I loooked briefly at the protected paths and I guess what you really
want is to use a dedicated global spinlock (because you are going to
access to the pmap of all CPUs, not only the one where you are running
into) which doesn't necessary need to be sched_lock (cpushead is not
sched_lock protected btw). Maybe something more fancy may be done, but
that's simple enough and working.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
On Thu, Jan 28, 2010 at 11:16:55AM +0100, Attilio Rao wrote: 2010/1/27 Marius Strobl mar...@alchemy.franken.de: On Tue, Jan 26, 2010 at 08:10:25AM +0100, Attilio Rao wrote: 2010/1/26 Rob Farmer rfar...@predatorlabs.net: On Sat, Jan 23, 2010 at 7:54 AM, Attilio Rao atti...@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? Actually it just adds old-td_lock = mtx in a non-atomic fashion as soon as we're done with the old thread. It's big as I had to reshuffle the register usage in order to preserve %i0 (old) and %i2 (mtx) and in order to distinguish between cpu_switch() and cpu_throw() (no mtx and old maybe be NULL in that case). As it turns out it also works just fine, the problems I were seeing were due to another change in that tree. Sorry for the noise. My understanding is that for ULE, mtx should be assigned to old-td_lock atomically, is that correct? Said that, all the code, including MD parts
Re: svn commit: r202889 - head/sys/kern
2010/1/30 Marius Strobl mar...@alchemy.franken.de: On Thu, Jan 28, 2010 at 11:16:55AM +0100, Attilio Rao wrote: 2010/1/27 Marius Strobl mar...@alchemy.franken.de: On Tue, Jan 26, 2010 at 08:10:25AM +0100, Attilio Rao wrote: 2010/1/26 Rob Farmer rfar...@predatorlabs.net: On Sat, Jan 23, 2010 at 7:54 AM, Attilio Rao atti...@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? Actually it just adds old-td_lock = mtx in a non-atomic fashion as soon as we're done with the old thread. It's big as I had to reshuffle the register usage in order to preserve %i0 (old) and %i2 (mtx) and in order to distinguish between cpu_switch() and cpu_throw() (no mtx and old maybe be NULL in that case). As it turns out it also works just fine, the problems I were seeing were due to another change in that tree. Sorry for the noise. My understanding is that for ULE, mtx should be assigned to old-td_lock atomically, is that correct? More precisely what needs to happen is to use a memory barrier. If you can
Re: svn commit: r202889 - head/sys/kern
On Sat, Jan 30, 2010 at 04:22:47PM +0100, Attilio Rao wrote:
2010/1/30 Marius Strobl mar...@alchemy.franken.de:
I think the idea behind using sched_lock in the sparc64 code is
to piggyback on it for protecting the pmap and take advantage of
the fact that it's held across cpu_switch() anyway. If that's
correct it should be possible to replace it with a separate
spinlock dedicated to protecting the pmap or given that due to
the macro madness involved in mtx_{,un}lock_spin() it's hard to
properly call these from asm by calling spinlock_{enter,exit}()
directly.
Even if that is the case there is no reason to not call
thread_lock()/thread_unlock() (which will acquire the correct
sched_lock or do the handover in the sched_4bsd in the right way) and
keep an unified spinlock in order to keep cpu_switch() simple and
still offering pmap protection over context switches.
It's not clear to me what threads to use in order to get this
to work the right way, for pmap_release() I'd use curthread and
for pmap_activate() I'd just pass td on to thread_{,un}lock()?
Whould the general approach of using thread_{,un}lock() for pmap
protection also do the right thing in case of ULE?
Marius
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
2010/1/27 Marius Strobl mar...@alchemy.franken.de: On Tue, Jan 26, 2010 at 08:10:25AM +0100, Attilio Rao wrote: 2010/1/26 Rob Farmer rfar...@predatorlabs.net: On Sat, Jan 23, 2010 at 7:54 AM, Attilio Rao atti...@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 ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
On Tue, Jan 26, 2010 at 08:10:25AM +0100, Attilio Rao wrote: 2010/1/26 Rob Farmer rfar...@predatorlabs.net: On Sat, Jan 23, 2010 at 7:54 AM, Attilio Rao atti...@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? Marius ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
2010/1/26 Attilio Rao atti...@freebsd.org: 2010/1/25 John Baldwin j...@freebsd.org: On Saturday 23 January 2010 10:54:22 am Attilio Rao 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. Does this affect the various #ifdef's for handling the third argument to cpu_switch()? E.g. does 4BSD need to spin if td_lock is blocked_lock? Also, BLOCK_SPIN() on x86 is non-optimal. It should not do cmpxchg in a loop. Instead, it should do cmp in a loop, and if the cmp succeeds, then try cmpxchg. [CC'ing imp@ because he made a similar question privately] Basically it is recounduited to the difference, in terms of runqueue between sched_4bsd and sched_ule. sched_ule has one runqueue per-core and sched_4bsd has just one runqueue. The codepath you are referring to, for the ULE case, is used when threads need to migrate from one cpu to another. What should really happen is that threads should be both locked (thus the runqueues where the in out threads are present should be both locked) but it will create cyclic LOR and then a 'blocked_lock' is used (in order to avoid such LORs). When you are switching in a thread caming from another CPU you need to make sure its lock already changed to, possibly, the new runqueue one (by the CPU where it *was* running that is going to switch it out). That happens because we have not a global interlock between the CPUs and thread migration gets a bit trickier. Said that, the !SMP case just assume there is one runqueue, so this discussion is not useful (you won't have different runqueues from where the thread may be caming, but just one protected by sched_lock). Also sched_4bsd has just one runqueue, thus this discussion should not apply there. What the last 4bsd patch does is to keep track of the 'old' thread_lock and assign it to the switched out thread, which may now be blocked, when it is safe to do that (after the tlb shootdown in ia32/amd64 cpu_switch() case). However, this may generate a bit of confusion, because 'blocked' has a different meaning between the 2 cases: * ULE does that in order to prevent LOR between runqueues CPUs. * 4BSD now does that in order to be able to not end with 2 spinlocks held in some cases (note: this is not a LOR because there is the well defined order of sq_lock/ts_lock - sched_lock) but it makes cpu_switch() simpler, faster and reduces contention. It is enough that cpu_switch() does handle the third argument properly and sets back the old lock when it is safe. I see, however, that this may be a problem for architectures that were just supporting 4BSD and not ULE (at least to some extent), like in the sparc64 case. If an architecture does not ignore the third argument of cpu_switch() and does set it properly there should be a problem with sched_4bsd. I should probabilly poke the maintainers of any arch and check with them if there are further problems. I checked for available architectures and I think that sparc64 (and possibly sun4v) is the only one with missing bits. Finally, I really don't think the BLOCK_SPIN() performance improvement you suggest should really make a difference (assuming how rarely it should happen) because, please note, that the cmpxchg is necessary as we need to enforce a memory barrier when doing the check in anyway (so the first case should be however an atomic_cmpset_X_Y()). I think that ia64 is broken on that regard because it does use simple operation while it should use correct atomic and memory barriers (CC'ed marcel@ for that). Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein
Re: svn commit: r202889 - head/sys/kern
On Tuesday 26 January 2010 3:58:45 am Attilio Rao wrote:
Finally, I really don't think the BLOCK_SPIN() performance improvement
you suggest should really make a difference (assuming how rarely it
should happen) because, please note, that the cmpxchg is necessary as
we need to enforce a memory barrier when doing the check in anyway (so
the first case should be however an atomic_cmpset_X_Y()).
Note that Intel specifically mentions in the app note that introduced 'pause'
that one should not sit in a spin loop that bangs on cmpxchg due to the extra
cache contention it causes. Also, I did not say to remove the cmpxchg, but to
only do it if a 'cmp' indicates it should work. We already do this now for
mtx_lock_spin() in C with something like this:
while (!atomic_cmpset_acq_ptr(...)) {
while (m-mtx_lock != MTX_UNOWNED) {
cpu_spinwait();
}
}
The idea is to do cheap compares that do not require an exclusive hold on
the cache line until you have a reason to think that the cmpxchg will work.
Last thinking: it is not a very good idea cpu_switch() is made
conditional in regard of schedulers, but there is not an easy way to
do that as long as the safe points for releasing blocked_lock happens
within cpu_switch(). I think that is one of the reasons why some
people (maybe you or Jeff) pushed for having cpu_switch() made in C.
Would the complicated version of BLOCK_SWITCH() work ok for the sched_4bsd and
ULE/UP case and just devolve into normally not spinning?
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
On Jan 26, 2010, at 3:39 AM, Attilio Rao wrote: Does this affect the various #ifdef's for handling the third argument to cpu_switch()? E.g. does 4BSD need to spin if td_lock is blocked_lock? I think that ia64 is broken on that regard because it does use simple operation while it should use correct atomic and memory barriers (CC'ed marcel@ for that). Ok, so cpu_switch() handles the 3rd argument (the mutex) only when SCHED_ULE and SMP are defined, even on i386. Maybe it's just me, but if SCHED_4BSD now also uses the 3rd argument then all implementations of cpu_switch() are broken, right? Maybe what is in order right now is a description (using pseudo code if you like) of what exactly needs to happen with the 3rd argument, when and how (i.e. what must be atomic and what does not have to be atomic). I can deal with ia64 and powerpc once I know for certain what exactly needs to happen, because it seems to me that I can't really look at the i386 implementation and infer what needs to happen. Thanks, -- Marcel Moolenaar xcl...@mac.com ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
In message: c6a8f7a7-f0a9-4f63-b61e-ddc5332dc...@mac.com
Marcel Moolenaar xcl...@mac.com writes:
:
: On Jan 26, 2010, at 3:39 AM, Attilio Rao wrote:
:
: Does this affect the various #ifdef's for handling the third argument to
: cpu_switch()? E.g. does 4BSD need to spin if td_lock is blocked_lock?
:
:
: I think that ia64 is broken on that regard because it does use simple
: operation while it should use correct atomic and memory barriers
: (CC'ed marcel@ for that).
:
: Ok, so cpu_switch() handles the 3rd argument (the mutex) only
: when SCHED_ULE and SMP are defined, even on i386. Maybe it's
: just me, but if SCHED_4BSD now also uses the 3rd argument then
: all implementations of cpu_switch() are broken, right?
No. The current implementations always STORE the argument. One only
loops if newthread-td_lock == blocked_lock. which is something
else. That was part of my initial confusion as well...
: Maybe what is in order right now is a description (using pseudo
: code if you like) of what exactly needs to happen with the 3rd
: argument, when and how (i.e. what must be atomic and what does
: not have to be atomic).
I believe the proper pseudo code should be:
cpu_switch(struct thread *old, struct thread *new, struct mutext *mtx)
{
/* Save the registers to the pcb */
old-td_lock = mtx;
#if defined(SMP) defined(SCHED_ULE)
/* s/long/int/ if sizeof(long) != sizeof(void *) */
/* as we have no 'void *' version of the atomics */
while (atomic_load_acq_long(new-td_lock) == (long)blocked_lock)
continue;
#endif
/* Switch to new context */
}
although the #ifdef might not actually be strictly necessary (eg, 4BSD
doesn't migrate threads like ULE does, so you don't hit the case of
having to wait for the new thread to become unblocked). At least
that's the impression I have after reading Atillio's email on this
topic.
I also think that we should have that code somewhere for reference.
When I was bringing up the MIPS code from the 6.1 base to -current,
the third argument made no sense to me at all, so I didn't implement
it at all. Someone (go...@?) tried to implement it, but it was found
recently to have been implemented wrong. That's been fixed now, and
as part of fixing it, I started asking questions...
: I can deal with ia64 and powerpc once I know for certain what
: exactly needs to happen, because it seems to me that I can't
: really look at the i386 implementation and infer what needs to
: happen.
I think that the only difference between what mips has now and what is
needed is that mips implements the while loop as approximately:
while ((volatile)new-td_lock == blocked_lock)
continue;
so I need to figure out the right memory barrier assembler instruction
to make this right. I think sync might be it for mips, but maybe it
is sync or some other processor dependent instruction (Cavium has
special instructions for things like this with more tailored
semantics, for example).
Warner
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
In message: 3023270a-755a-4bcf-ac9a-c1f290052...@mac.com
Marcel Moolenaar xcl...@mac.com writes:
:
: On Jan 26, 2010, at 12:09 PM, M. Warner Losh wrote:
: cpu_switch(struct thread *old, struct thread *new, struct mutext *mtx)
: {
: /* Save the registers to the pcb */
: old-td_lock = mtx;
: #if defined(SMP) defined(SCHED_ULE)
: /* s/long/int/ if sizeof(long) != sizeof(void *) */
: /* as we have no 'void *' version of the atomics */
: while (atomic_load_acq_long(new-td_lock) == (long)blocked_lock)
: continue;
: #endif
: /* Switch to new context */
: }
:
: Ok. So this is what ia64 has already, except for the atomic_load()
: in the while loop. Since td_lock is volatile, I don't think we need
: atomic_load(). To be explicit, ia64 has:
:
: old-td_lock = mtx;
: #if defined(SCHED_ULE) defined(SMP)
: /* td_lock is volatile */
: while (new-td_lock == blocked_lock)
: ;
: #endif
:
: Am I right, or am I missing a critical aspect of using atomic load?
The Atomic load acq also has a memory barrier after the item is
fetched from memory.
: I also think that we should have that code somewhere for reference.
:
: Since ia64 has a C implementation of cpu_switch(), we could make
: that the reference implementation?
Most likely :)
Warner
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
On Tuesday 26 January 2010 3:09:32 pm M. Warner Losh wrote:
In message: c6a8f7a7-f0a9-4f63-b61e-ddc5332dc...@mac.com
Marcel Moolenaar xcl...@mac.com writes:
: Maybe what is in order right now is a description (using pseudo
: code if you like) of what exactly needs to happen with the 3rd
: argument, when and how (i.e. what must be atomic and what does
: not have to be atomic).
I believe the proper pseudo code should be:
cpu_switch(struct thread *old, struct thread *new, struct mutext *mtx)
{
/* Save the registers to the pcb */
old-td_lock = mtx;
#if defined(SMP) defined(SCHED_ULE)
/* s/long/int/ if sizeof(long) != sizeof(void *) */
/* as we have no 'void *' version of the atomics */
while (atomic_load_acq_long(new-td_lock) == (long)blocked_lock)
continue;
#endif
/* Switch to new context */
}
FYI, that is what the '_ptr' variants of atomic ops are for. I do think that
the 'acq' membar on the load is needed to ensure the CPU doesn't try to
speculatively read any of the 'new' thread's PCB until it sees the update to
td_lock.
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
2010/1/26 John Baldwin j...@freebsd.org:
On Tuesday 26 January 2010 3:09:32 pm M. Warner Losh wrote:
In message: c6a8f7a7-f0a9-4f63-b61e-ddc5332dc...@mac.com
Marcel Moolenaar xcl...@mac.com writes:
: Maybe what is in order right now is a description (using pseudo
: code if you like) of what exactly needs to happen with the 3rd
: argument, when and how (i.e. what must be atomic and what does
: not have to be atomic).
I believe the proper pseudo code should be:
cpu_switch(struct thread *old, struct thread *new, struct mutext *mtx)
{
/* Save the registers to the pcb */
old-td_lock = mtx;
#if defined(SMP) defined(SCHED_ULE)
/* s/long/int/ if sizeof(long) != sizeof(void *) */
/* as we have no 'void *' version of the atomics */
while (atomic_load_acq_long(new-td_lock) == (long)blocked_lock)
continue;
#endif
/* Switch to new context */
}
FYI, that is what the '_ptr' variants of atomic ops are for. I do think that
the 'acq' membar on the load is needed to ensure the CPU doesn't try to
speculatively read any of the 'new' thread's PCB until it sees the update to
td_lock.
I think it is more important to store the old lock via a rel barrier instead:
- old-td_lock = mtx;
+ atomic_store_rel_ptr(old-td_lock, mtx);
Thanks,
Attilio
--
Peace can only be achieved by understanding - A. Einstein
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
On Tuesday 26 January 2010 4:20:46 pm Attilio Rao wrote:
2010/1/26 John Baldwin j...@freebsd.org:
On Tuesday 26 January 2010 3:09:32 pm M. Warner Losh wrote:
In message: c6a8f7a7-f0a9-4f63-b61e-ddc5332dc...@mac.com
Marcel Moolenaar xcl...@mac.com writes:
: Maybe what is in order right now is a description (using pseudo
: code if you like) of what exactly needs to happen with the 3rd
: argument, when and how (i.e. what must be atomic and what does
: not have to be atomic).
I believe the proper pseudo code should be:
cpu_switch(struct thread *old, struct thread *new, struct mutext *mtx)
{
/* Save the registers to the pcb */
old-td_lock = mtx;
#if defined(SMP) defined(SCHED_ULE)
/* s/long/int/ if sizeof(long) != sizeof(void *) */
/* as we have no 'void *' version of the atomics */
while (atomic_load_acq_long(new-td_lock) == (long)blocked_lock)
continue;
#endif
/* Switch to new context */
}
FYI, that is what the '_ptr' variants of atomic ops are for. I do think
that
the 'acq' membar on the load is needed to ensure the CPU doesn't try to
speculatively read any of the 'new' thread's PCB until it sees the update to
td_lock.
I think it is more important to store the old lock via a rel barrier instead:
- old-td_lock = mtx;
+ atomic_store_rel_ptr(old-td_lock, mtx);
Both are required to ensure the new CPU does not load stale values from the pcb.
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
On Saturday 23 January 2010 10:54:22 am Attilio Rao 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. Does this affect the various #ifdef's for handling the third argument to cpu_switch()? E.g. does 4BSD need to spin if td_lock is blocked_lock? Also, BLOCK_SPIN() on x86 is non-optimal. It should not do cmpxchg in a loop. Instead, it should do cmp in a loop, and if the cmp succeeds, then try cmpxchg. -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
On Sat, Jan 23, 2010 at 7:54 AM, Attilio Rao atti...@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? Thanks, -- Rob Farmer ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r202889 - head/sys/kern
2010/1/26 Rob Farmer rfar...@predatorlabs.net: On Sat, Jan 23, 2010 at 7:54 AM, Attilio Rao atti...@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. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
svn commit: r202889 - head/sys/kern
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
Modified: head/sys/kern/kern_mutex.c
==
--- head/sys/kern/kern_mutex.c Sat Jan 23 15:28:18 2010(r202888)
+++ head/sys/kern/kern_mutex.c Sat Jan 23 15:54:21 2010(r202889)
@@ -616,7 +616,6 @@ thread_lock_block(struct thread *td)
{
struct mtx *lock;
- spinlock_enter();
THREAD_LOCK_ASSERT(td, MA_OWNED);
lock = td-td_lock;
td-td_lock = blocked_lock;
@@ -631,7 +630,6 @@ thread_lock_unblock(struct thread *td, s
mtx_assert(new, MA_OWNED);
MPASS(td-td_lock == blocked_lock);
atomic_store_rel_ptr((volatile void *)td-td_lock, (uintptr_t)new);
- spinlock_exit();
}
void
Modified: head/sys/kern/sched_4bsd.c
==
--- head/sys/kern/sched_4bsd.c Sat Jan 23 15:28:18 2010(r202888)
+++ head/sys/kern/sched_4bsd.c Sat Jan 23 15:54:21 2010(r202889)
@@ -920,9 +920,11 @@ sched_sleep(struct thread *td, int pri)
void
sched_switch(struct thread *td, struct thread *newtd, int flags)
{
+ struct mtx *tmtx;
struct td_sched *ts;
struct proc *p;
+ tmtx = NULL;
ts = td-td_sched;
p = td-td_proc;
@@ -931,10 +933,11 @@ sched_switch(struct thread *td, struct t
/*
* Switch to the sched lock to fix things up and pick
* a new thread.
+* Block the td_lock in order to avoid breaking the critical path.
*/
if (td-td_lock != sched_lock) {
mtx_lock_spin(sched_lock);
- thread_unlock(td);
+ tmtx = thread_lock_block(td);
}
if ((td-td_flags TDF_NOLOAD) == 0)
@@ -1004,7 +1007,7 @@ sched_switch(struct thread *td, struct t
(*dtrace_vtime_switch_func)(newtd);
#endif
- cpu_switch(td, newtd, td-td_lock);
+ cpu_switch(td, newtd, tmtx != NULL ? tmtx : td-td_lock);
lock_profile_obtain_lock_success(sched_lock.lock_object,
0, 0, __FILE__, __LINE__);
/*
Modified: head/sys/kern/sched_ule.c
==
--- head/sys/kern/sched_ule.c Sat Jan 23 15:28:18 2010(r202888)
+++ head/sys/kern/sched_ule.c Sat Jan 23 15:54:21 2010(r202889)
@@ -301,7 +301,6 @@ static int sched_pickcpu(struct thread *
static void sched_balance(void);
static int sched_balance_pair(struct tdq *, struct tdq *);
static inline struct tdq *sched_setcpu(struct thread *, int, int);
-static inline struct mtx *thread_block_switch(struct thread *);
static inline void thread_unblock_switch(struct thread *, struct mtx *);
static struct mtx *sched_switch_migrate(struct tdq *, struct thread *, int);
static int sysctl_kern_sched_topology_spec(SYSCTL_HANDLER_ARGS);
@@ -1106,9 +1105,11 @@ sched_setcpu(struct thread *td, int cpu,
* The hard case, migration, we need to block the thread first to
* prevent order reversals with other cpus locks.
*/
+ spinlock_enter();
thread_lock_block(td);
TDQ_LOCK(tdq);
thread_lock_unblock(td,
