Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c
Matthew Dillon [EMAIL PROTECTED] wrote:
:
:Hi,
:
:Sorry to unwantedly butt in, but the patch supplied by Seigo actually
:solved the vm_map.c locking problems which used to come up on my system.
:Just some info. :)
:
:Regards,
:
: -- Hiten Pandya
: -- [EMAIL PROTECTED]
It fixed some things, it broke some things. Pretty much standard
fare for anyone who has ever done work on the vm_map lock, including
yours truely. John Dyson couldn't get it right, David Greenman couldn't
get it right, I couldn't get it completely right... getting it to do
the right thing even under -stable is difficult, which is why I am
suggesting that it simply be made into an exclusive lock in -current.
In addition to the fact it doesn't actually serialize the general
modification of the vm_map {} structure itself, just certain kinds of
changes, so is easily a primary reason that the VM system as it stands now
is inherently single-threaded.
--
Brian Fundakowski Feldman \'[ FreeBSD ]''\
[EMAIL PROTECTED] [EMAIL PROTECTED] \ The Power to Serve! \
Opinions expressed are my own. \,,\
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c
Hi, Sorry to unwantedly butt in, but the patch supplied by Seigo actually solved the vm_map.c locking problems which used to come up on my system. Just some info. :) Regards, -- Hiten Pandya -- [EMAIL PROTECTED] --- Matthew Dillon [EMAIL PROTECTED] wrote: I have no idea what the frig you guys are doing in vm_map.c. I would recommend that all the changes be backed out. Then I would recommend that the following be done: * change the lockmgr vm_map lock to be exclusive-only * test commit * change the lockmgr vm_map lock to an exclusive SX lock * test commit * Then work on re-optimizing the vm_map locks You guys are trying to bite off too much all at once. __ Do You Yahoo!? Yahoo! Sports - live college hoops coverage http://sports.yahoo.com/ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c
: :Hi, : :Sorry to unwantedly butt in, but the patch supplied by Seigo actually :solved the vm_map.c locking problems which used to come up on my system. :Just some info. :) : :Regards, : : -- Hiten Pandya : -- [EMAIL PROTECTED] It fixed some things, it broke some things. Pretty much standard fare for anyone who has ever done work on the vm_map lock, including yours truely. John Dyson couldn't get it right, David Greenman couldn't get it right, I couldn't get it completely right... getting it to do the right thing even under -stable is difficult, which is why I am suggesting that it simply be made into an exclusive lock in -current. -Matt Matthew Dillon [EMAIL PROTECTED] :--- Matthew Dillon [EMAIL PROTECTED] wrote: : I have no idea what the frig you guys are doing in vm_map.c. I would : recommend that all the changes be backed out. Then I would recommend : that the following be done: : : * change the lockmgr vm_map lock to be exclusive-only : * test commit : * change the lockmgr vm_map lock to an exclusive SX lock : * test commit : * Then work on re-optimizing the vm_map locks : : You guys are trying to bite off too much all at once. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c
On Sun, 17 Mar 2002, Matthew Dillon wrote: It fixed some things, it broke some things. Pretty much standard fare for anyone who has ever done work on the vm_map lock, including yours truely. John Dyson couldn't get it right, David Greenman couldn't get it right, I couldn't get it completely right... getting it to do the right thing even under -stable is difficult, which is why I am suggesting that it simply be made into an exclusive lock in -current. Hmm. Ok, so right now the code that has been branched for DP1 makes use of Brian's recent locking commits. My original thought was we'd simply back it out of that branch to make sure that the DP is reliable. How hard would it be for us to switch to what you propose (just convert all slocks into xlocks, and simplify out the upgrade semantics), in particular, before we cut the DP? It sounds to me like the right approach is simply to fall back to lockmgr for the DP, and get these fixes into the main tree and they'll be there for the next DP in a few months. Can you take ownership of the task since you clearly know what's going on? Can I stick your name in the smp web page saying so? :-) Robert N M Watson FreeBSD Core Team, TrustedBSD Project [EMAIL PROTECTED] NAI Labs, Safeport Network Services To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c
:Hmm. Ok, so right now the code that has been branched for DP1 makes use :of Brian's recent locking commits. My original thought was we'd simply :back it out of that branch to make sure that the DP is reliable. How hard :would it be for us to switch to what you propose (just convert all slocks :into xlocks, and simplify out the upgrade semantics), in particular, :before we cut the DP? It sounds to me like the right approach is simply :to fall back to lockmgr for the DP, and get these fixes into the main tree :and they'll be there for the next DP in a few months. : :Can you take ownership of the task since you clearly know what's going on? :Can I stick your name in the smp web page saying so? :-) : :Robert N M Watson FreeBSD Core Team, TrustedBSD Project :[EMAIL PROTECTED] NAI Labs, Safeport Network Services I think the developers currently working on it have the expertise to clean it up. My recommendation is that developer who committed the SX lock stuff back it out (fall back to the lockmgr locks), and then that same developer scan the code and determine if the lockmgr lock can just be made exclusive for all cases and, if so, to test and commit that. If the exclusive-only lockmgr lock works then that is what should go into the DP, else the original lockmgr stuff should go in. Until the critical_*() function issue is resolved I do not want to make extensive modifications or updates to my -current tree so I couldn't do this stuff in the time frame you are talking about even if I wanted to. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
sx_upgrade() (was: Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c)
[Add jhb and move to -current]
On Fri, 15 Mar 2002 10:53:20 -0800,
Alfred Perlstein [EMAIL PROTECTED] said:
Alfred * Brian F. Feldman [EMAIL PROTECTED] [020315 03:22] wrote:
Alfred Perlstein [EMAIL PROTECTED] wrote:
What is the problem?
Damn good question. Are the tracebacks related? If not, what are you
supposed to be telling me it's deadlocking on? I don't see the system being
deadlocked. What is it actually supposed to be blocked on?
Well basically you changed:
! static __inline__ int
! _vm_map_lock_upgrade(vm_map_t map, struct thread *td) {
! int error;
!
! vm_map_printf(locking map LK_EXCLUPGRADE: %p\n, map);
! error = lockmgr(map-lock, LK_EXCLUPGRADE, NULL, td);
! if (error == 0)
! map-timestamp++;
! return error;
}
into:
! _vm_map_lock_upgrade(vm_map_t map, const char *file, int line)
{
! vm_map_printf(locking map LK_EXCLUPGRADE: %p\n, map);
! if (_sx_try_upgrade(map-lock, file, line)) {
! map-timestamp++;
! return (0);
! }
! return (EWOULDBLOCK);
}
It doesn't need LK_EXCLUPGRADE semantics, only LK_UPGRADE, if it's not
blocking. It backs out completely and unlocks the shared reference and
tries for an exclusive lock.
Alfred Sigh, you're making me do all the work here... :(
Alfred lockmgr(map-lock, LK_EXCLUPGRADE, NULL, td);
Alfred means:
Alfred Turn my shared lock into an exclusive lock,
Alfred if it's shared then wait for all shared locks to drain,
Alfred however if someone else is requesting an exclusive lock, then fail.
Alfred _sx_try_upgrade(map-lock, file, line)
Alfred means:
Alfred Give me an exclusive lock
Alfred if anyone else has a shared lock then fail immediately.
Alfred What happens in your case is that you get into a busy loop
Alfred because you never yeild the processor.
Alfred This happens in vm_map_lookup() because of this:
Alfred if (fault_type VM_PROT_WRITE) {
Alfred /*
Alfred * Make a new object, and place it in the object
Alfred * chain. Note that no new references have appeared
Alfred * -- one just moved from the map to the new
Alfred * object.
Alfred */
Alfred if (vm_map_lock_upgrade(map))
Alfred goto RetryLookup;
Alfred So, you fail your sx_lock upgrade and cause the cpu to spin.
Alfred You basically need to add logic to the sxlock subsystem to do what
Alfred lockmgr does, actually wait for the others to go away or fail if
Alfred someone else wants an upgrade.
Attached patch implements sx_upgrade() which should work as you said
above. This compiles fine, but is not tested yet.
sx_upgrader records the thread that wants to upgrade. If sx_upgrader
is non-NULL, sx_sunlock() wakes up the upgrader rather than other
exclusive lock waiters.
sx_upgrade.diff
Description: Binary data
--
Seigo Tanimura [EMAIL PROTECTED] [EMAIL PROTECTED]
Re: sx_upgrade() (was: Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c)
On Sat, 16 Mar 2002 19:08:53 +0900, Seigo Tanimura [EMAIL PROTECTED] said: Seigo Attached patch implements sx_upgrade() which should work as you said Seigo above. This compiles fine, but is not tested yet. The last patch breaks INVARIANTS. This one compiles and seems to work. sx_upgrade.diff Description: Binary data -- Seigo Tanimura [EMAIL PROTECTED] [EMAIL PROTECTED]
