Re: Spurious witness warning when destroying spin mtx
On Saturday, November 24, 2012 10:01:39 am Attilio Rao wrote: On Sat, Nov 24, 2012 at 3:08 AM, Ryan Stone ryst...@gmail.com wrote: Today I saw a spurious witness warning for acquiring duplicate lock of same type. The root cause is that when running mtx_destroy on a spinlock that is held by the current thread, mtx_destroy calls spinlock_exit() before calling WITNESS_UNLOCK, which opens up a window in which the CPU can be interrupted and attempt to acquire another spinlock of the same type as the one being destroyed. This patch should fix it: I seriously wonder why right now we don't assume the lock is unheld. There are likely historically reasons for that, but I would like to know which one are those and eventually fix them out. FWIK, all the other locking primitives assume the lock is already unheld when destroying and I think it would be good to have that for mutexes as well. That is simply behavior we inherited from BSD/OS. I didn't find it all that useful so all of the other locking primitives I've added since then have not had this behavior. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Spurious witness warning when destroying spin mtx
On Friday, November 23, 2012 10:08:28 PM Ryan Stone wrote:
Today I saw a spurious witness warning for acquiring duplicate lock of
same type. The root cause is that when running mtx_destroy on a spinlock
that is held by the current thread, mtx_destroy calls spinlock_exit()
before calling WITNESS_UNLOCK, which opens up a window in which the CPU can
be interrupted and attempt to acquire another spinlock of the same type as
the one being destroyed. This patch should fix it:
diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c
index 2f13863..96f43f8 100644
--- a/sys/kern/kern_mutex.c
+++ b/sys/kern/kern_mutex.c
@@ -918,16 +918,16 @@ _mtx_destroy(volatile uintptr_t *c)
else {
MPASS((m-mtx_lock (MTX_RECURSED|MTX_CONTESTED)) == 0);
+ lock_profile_release_lock(m-lock_object);
+ /* Tell witness this isn't locked to make it happy. */
+ WITNESS_UNLOCK(m-lock_object, LOP_EXCLUSIVE, __FILE__,
+ __LINE__);
+
/* Perform the non-mtx related part of mtx_unlock_spin().
*/ if (LOCK_CLASS(m-lock_object) == lock_class_mtx_spin)
spinlock_exit();
else
curthread-td_locks--;
-
- lock_profile_release_lock(m-lock_object);
- /* Tell witness this isn't locked to make it happy. */
- WITNESS_UNLOCK(m-lock_object, LOP_EXCLUSIVE, __FILE__,
- __LINE__);
}
m-mtx_lock = MTX_DESTROYED
Ah, I would tweak this slightly perhaps to match mtx_unlock() and
mtx_unlock_spin():
if (LOCK_CLASS() == lock_class_mtx_sleep)
curthread-td_locks--;
/* lock profile and witness stuff */
if (LOCK_CLASS() == lock_class_mtx_spin)
spinlock_exit();
You are correct that it is broken for the spin case.
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Spurious witness warning when destroying spin mtx
On Sat, Nov 24, 2012 at 3:01 PM, Attilio Rao atti...@freebsd.org wrote: On Sat, Nov 24, 2012 at 3:08 AM, Ryan Stone ryst...@gmail.com wrote: Today I saw a spurious witness warning for acquiring duplicate lock of same type. The root cause is that when running mtx_destroy on a spinlock that is held by the current thread, mtx_destroy calls spinlock_exit() before calling WITNESS_UNLOCK, which opens up a window in which the CPU can be interrupted and attempt to acquire another spinlock of the same type as the one being destroyed. This patch should fix it: I seriously wonder why right now we don't assume the lock is unheld. There are likely historically reasons for that, but I would like to know which one are those and eventually fix them out. FWIK, all the other locking primitives assume the lock is already unheld when destroying and I think it would be good to have that for mutexes as well. Can you please show which lock triggers the panic you saw? Ryan, however I'm sure this patch would introduce a major switch in our KPI/POLA and eventually it would be a bigger work. Your patch is certainly right and I think you should commit it for the time being. For the long-term, maybe you would like to work on such a patch, rather. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Spurious witness warning when destroying spin mtx
On Sat, Nov 24, 2012 at 3:08 AM, Ryan Stone ryst...@gmail.com wrote: Today I saw a spurious witness warning for acquiring duplicate lock of same type. The root cause is that when running mtx_destroy on a spinlock that is held by the current thread, mtx_destroy calls spinlock_exit() before calling WITNESS_UNLOCK, which opens up a window in which the CPU can be interrupted and attempt to acquire another spinlock of the same type as the one being destroyed. This patch should fix it: I seriously wonder why right now we don't assume the lock is unheld. There are likely historically reasons for that, but I would like to know which one are those and eventually fix them out. FWIK, all the other locking primitives assume the lock is already unheld when destroying and I think it would be good to have that for mutexes as well. Can you please show which lock triggers the panic you saw? Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Spurious witness warning when destroying spin mtx
On Sat, Nov 24, 2012 at 10:01 AM, Attilio Rao atti...@freebsd.org wrote:
I seriously wonder why right now we don't assume the lock is unheld.
There are likely historically reasons for that, but I would like to
know which one are those and eventually fix them out.
FWIK, all the other locking primitives assume the lock is already
unheld when destroying and I think it would be good to have that for
mutexes as well.
Can you please show which lock triggers the panic you saw?
Thanks,
Attilio
It was taskqueue_free:
void
taskqueue_free(struct taskqueue *queue)
{
TQ_LOCK(queue);
queue-tq_flags = ~TQ_FLAGS_ACTIVE;
taskqueue_terminate(queue-tq_threads, queue);
KASSERT(TAILQ_EMPTY(queue-tq_active), (Tasks still running?));
KASSERT(queue-tq_callouts == 0, (Armed timeout tasks));
mtx_destroy(queue-tq_mutex);
free(queue-tq_threads, M_TASKQUEUE);
free(queue, M_TASKQUEUE);
}
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Spurious witness warning when destroying spin mtx
On Sat, Nov 24, 2012 at 3:46 PM, Ryan Stone ryst...@gmail.com wrote: On Sat, Nov 24, 2012 at 10:01 AM, Attilio Rao atti...@freebsd.org wrote: I seriously wonder why right now we don't assume the lock is unheld. There are likely historically reasons for that, but I would like to know which one are those and eventually fix them out. FWIK, all the other locking primitives assume the lock is already unheld when destroying and I think it would be good to have that for mutexes as well. Can you please show which lock triggers the panic you saw? Thanks, Attilio It was taskqueue_free: taskqueue_free() must not be called in places where there are still races, so the lock is not really meaningful and should be acquired. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Spurious witness warning when destroying spin mtx
On Sat, Nov 24, 2012 at 3:51 PM, Attilio Rao atti...@freebsd.org wrote: On Sat, Nov 24, 2012 at 3:46 PM, Ryan Stone ryst...@gmail.com wrote: On Sat, Nov 24, 2012 at 10:01 AM, Attilio Rao atti...@freebsd.org wrote: I seriously wonder why right now we don't assume the lock is unheld. There are likely historically reasons for that, but I would like to know which one are those and eventually fix them out. FWIK, all the other locking primitives assume the lock is already unheld when destroying and I think it would be good to have that for mutexes as well. Can you please show which lock triggers the panic you saw? Thanks, Attilio It was taskqueue_free: taskqueue_free() must not be called in places where there are still races, so the lock is not really meaningful and should be acquired. Herm, I mean to say after taskqueue_termintate() returns must not be races Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Spurious witness warning when destroying spin mtx
Today I saw a spurious witness warning for acquiring duplicate lock of
same type. The root cause is that when running mtx_destroy on a spinlock
that is held by the current thread, mtx_destroy calls spinlock_exit()
before calling WITNESS_UNLOCK, which opens up a window in which the CPU can
be interrupted and attempt to acquire another spinlock of the same type as
the one being destroyed. This patch should fix it:
diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c
index 2f13863..96f43f8 100644
--- a/sys/kern/kern_mutex.c
+++ b/sys/kern/kern_mutex.c
@@ -918,16 +918,16 @@ _mtx_destroy(volatile uintptr_t *c)
else {
MPASS((m-mtx_lock (MTX_RECURSED|MTX_CONTESTED)) == 0);
+ lock_profile_release_lock(m-lock_object);
+ /* Tell witness this isn't locked to make it happy. */
+ WITNESS_UNLOCK(m-lock_object, LOP_EXCLUSIVE, __FILE__,
+ __LINE__);
+
/* Perform the non-mtx related part of mtx_unlock_spin(). */
if (LOCK_CLASS(m-lock_object) == lock_class_mtx_spin)
spinlock_exit();
else
curthread-td_locks--;
-
- lock_profile_release_lock(m-lock_object);
- /* Tell witness this isn't locked to make it happy. */
- WITNESS_UNLOCK(m-lock_object, LOP_EXCLUSIVE, __FILE__,
- __LINE__);
}
m-mtx_lock = MTX_DESTROYED
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
