Date: Thu, 04 Oct 2001 02:31:06 -0700 (PDT) From: John Baldwin <jhb@FreeBSD.org> To: Brian Fundakowski Feldman <green@FreeBSD.org> Cc: arch@FreeBSD.org Subject: RE: sx-ifying src/sys/vm Message-ID: <XFMail.011004023106.jhb@FreeBSD.org> In-Reply-To: <200110040105.f9415av15297@green.bikeshed.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 04-Oct-01 Brian Fundakowski Feldman wrote:
> I've taken a shot at changing the vm_map locking from using lockmgr() locks
> (EAAARGHH...) to sx locks. Already I can tell that it 1. seems to work and
> 2. seems to not give me bogus lock ordering messages like lockmgr() used to.
> I'd appreciate if people took a look at it and told me of any concerns,
> whether it also looks fine to more than just myself, etc.
>
> I added a few more vm_map locking operations than were there already for the
> purpose of removing the hard-coded lockmgr() ops that were in a few places
> previously. Instead of having a sx_locked() operation I used
> sx_try_xlock(), and instead of recursion I saved the state of the one case
> where a lock was attempted to be "recursed" upon, and removed the actual
> recursion on the lock.
Note that recursion of shared locks is allowed. Also, the name
vm_map_lookup2() is really gross. However, if you only recurse on a read lock,
then you can get rid of lockheld it seems and remove the vm_map_lookup()
ugliness.
> Index: vm_fault.c
> ===================================================================
> RCS file: /home/freebsd/ncvs/src/sys/vm/vm_fault.c,v
> retrieving revision 1.125
> diff -u -r1.125 vm_fault.c
> --- vm_fault.c 2001/09/12 08:38:11 1.125
> +++ vm_fault.c 2001/10/04 00:41:36
> @@ -213,12 +214,16 @@
> int hardfault;
> int faultcount;
> struct faultstate fs;
> + boolean_t user_wire;
>
> GIANT_REQUIRED;
>
> cnt.v_vm_faults++;
> hardfault = 0;
> + user_wire = (fault_flags & VM_FAULT_WIRE_MASK) == VM_FAULT_USER_WIRE;
> + fs.lockheld = user_wire;
>
> +
> RetryFault:;
>
> /*
Extra blank line?
> Index: vm_glue.c
> ===================================================================
> RCS file: /home/freebsd/ncvs/src/sys/vm/vm_glue.c,v
> retrieving revision 1.119
> diff -u -r1.119 vm_glue.c
> --- vm_glue.c 2001/09/12 08:38:11 1.119
> +++ vm_glue.c 2001/10/04 00:41:37
> @@ -577,9 +577,7 @@
> * data structures there is a
> * possible deadlock.
> */
> - if (lockmgr(&vm->vm_map.lock,
> - LK_EXCLUSIVE | LK_NOWAIT,
> - NULL, curthread)) {
> + if (vm_map_trylock(&vm->vm_map)) {
> vmspace_free(vm);
> PROC_UNLOCK(p);
> goto nextproc;
Hrm, this reads weird. It should be 'if (!vm_map_trylock()) {'. I would make
vm_map_trylock() return true on success and false on failure. Same for
vm_map_lock_upgrade().
> Index: vm_map.c
> ===================================================================
> RCS file: /home/freebsd/ncvs/src/sys/vm/vm_map.c,v
> retrieving revision 1.207
> diff -u -r1.207 vm_map.c
> --- vm_map.c 2001/09/12 08:38:11 1.207
> +++ vm_map.c 2001/10/04
> @@ -403,7 +392,7 @@
> map->first_free = &map->header;
> map->hint = &map->header;
> map->timestamp = 0;
> - lockinit(&map->lock, PVM, "thrd_sleep", 0, LK_NOPAUSE);
> + sx_init(&map->lock, "thrd_sleep");
> }
>
> void
Why not a more recognizable name such as "vm_map"?
> @@ -1490,7 +1479,6 @@
> estart = entry->start;
>
> /* First we need to allow map modifications */
> - vm_map_set_recursive(map);
> vm_map_lock_downgrade(map);
> map->timestamp++;
>
Old bug: why is the timestamp updated while not holding an exclusive lock? If
anything, it should be bumped before the downgrade. Otherwise there isn't a
memory barrier there to ensure other slock holders will see the right timestamp
on other CPU's.
> @@ -1500,14 +1488,12 @@
> entry->wired_count--;
> entry->eflags &= ~MAP_ENTRY_USER_WIRED;
>
> - vm_map_clear_recursive(map);
> - vm_map_unlock(map);
> + vm_map_unlock_read(map);
>
> (void) vm_map_user_pageable(map, start,
entry->start, TRUE);
> return rv;
> }
>
> - vm_map_clear_recursive(map);
> if (vm_map_lock_upgrade(map)) {
> vm_map_lock(map);
> if (vm_map_lookup_entry(map, estart, &entry)
Another old bug: why is the lock being modified w/o holding an exclusive lock?
Looks like this one is now moot, however.
> @@ -2678,6 +2670,22 @@
> vm_prot_t *out_prot, /* OUT */
> boolean_t *wired) /* OUT */
> {
> +
> + return (vm_map_lookup2(var_map, vaddr, fault_typea, out_entry,
> + object, pindex, out_prot, wired, 0));
> +}
> +
> +int
> +vm_map_lookup2(vm_map_t *var_map, /* IN/OUT */
> + vm_offset_t vaddr,
> + vm_prot_t fault_typea,
> + vm_map_entry_t *out_entry, /* OUT */
> + vm_object_t *object, /* OUT */
> + vm_pindex_t *pindex, /* OUT */
> + vm_prot_t *out_prot, /* OUT */
> + boolean_t *wired, /* OUT */
> + boolean_t lockheld)
> +{
> vm_map_entry_t entry;
> vm_map_t map = *var_map;
> vm_prot_t prot;
> @@ -2690,11 +2698,13 @@
> * Lookup the faulting address.
> */
>
> - vm_map_lock_read(map);
> + if (!lockheld)
> + vm_map_lock_read(map);
>
> #define RETURN(why) \
> { \
> - vm_map_unlock_read(map); \
> + if (!lockheld) \
> + vm_map_unlock_read(map); \
> return(why); \
> }
>
> @@ -2730,7 +2740,8 @@
> vm_map_t old_map = map;
>
> *var_map = map = entry->object.sub_map;
> - vm_map_unlock_read(old_map);
> + if (!lockheld)
> + vm_map_unlock_read(old_map);
> goto RetryLookup;
> }
>
Ok, I don't like the lockheld parameter here. :) It looks like the recursive
locks are read locks. Is the problem that you can call this function with an
exclusive lock held? If so, I would rather that you add an assertion
SX_ASSERT_LOCKED(&map->lock) that the map is locked (either read or write) by
the caller. Then vm_map_lookup() can simply assume the map is already locked.
> Index: vm_zone.c
> ===================================================================
> RCS file: /home/freebsd/ncvs/src/sys/vm/vm_zone.c,v
> retrieving revision 1.48
> diff -u -r1.48 vm_zone.c
> --- vm_zone.c 2001/08/05 03:55:02 1.48
> +++ vm_zone.c 2001/10/04 00:41:48
> @@ -409,11 +409,12 @@
> * map.
> */
> mtx_unlock(&z->zmtx);
> - if (lockstatus(&kernel_map->lock, NULL)) {
> + if (vm_map_trylock(kernel_map)) {
> item = (void *) kmem_malloc(kmem_map, nbytes, M_WAITOK);
> if (item != NULL)
> atomic_add_int(&zone_kmem_pages, z->zalloc);
> } else {
> + vm_map_unlock(kernel_map);
> item = (void *) kmem_alloc(kernel_map, nbytes);
> if (item != NULL)
> atomic_add_int(&zone_kern_pages, z->zalloc);
Question: why does the zone allocator care if the map is locked or not? Also,
this has a race: some other thread could easily come in and lock the map in
between the vm_map_unlock() and the kmem_alloc(), thus rendering this test
useless. This entire test here is not thread safe and needs to be rethought.
--
John Baldwin <jhb@FreeBSD.org> -- http://www.FreeBSD.org/~jhb/
PGP Key: http://www.baldwin.cx/~john/pgpkey.asc
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.011004023106.jhb>