Re: [Sbcl-devel] with-locked-hash-table and interrupts

On 28 May 2011 22:14, Nikodemus Siivola <nikodemus@...> wrote:
> WITH-LOCKED-HASH-TABLE should IMO be considered a user lock. To me it
> is morally analogous to a user have an external lock and grabbing that
> around accesses to a hash-table.
> I'm not sure if the meaning here is that SBCL's internal hash-tables
> should keep interrupts disabled -- which I can certainly understand --
With that theory in mind:
https://github.com/nikodemus/SBCL/tree/with-locked-system-table
Cheers,
-- Nikodemus

Thread view

As of 1.0.25.15 (below for convenience) WITH-LOCKED-HASH-TABLE also
disables interrupts for its body.
>From the commit message it is not clear to me why.
WITH-LOCKED-HASH-TABLE should IMO be considered a user lock. To me it
is morally analogous to a user have an external lock and grabbing that
around accesses to a hash-table.
I'm not sure if the meaning here is that SBCL's internal hash-tables
should keep interrupts disabled -- which I can certainly understand --
or if there is something non-obvious in the hash-table code that makes
interrupts arriving during a locked section fundamentally unsafe?
The disabled interrupts are really quite nasty here, since safe
iteration over a table in threaded environment is very likely to need
them -- which in turns makes those iterations both opaque to profiling
and really nasty to debug.
Cheers,
-- nikodemus
commit 78c7dbe937e6783e8da6f7a39c3eba87294d197c
Author: Gabor Melis <mega@...>
Date: Mon Feb 16 22:26:25 2009 +0000
1.0.25.51: use WITH-RECURSIVE-SYSTEM-SPINLOCK
... instead of WITH-RECURSIVE-SPINLOCK because it's possible to
deadlock due to lock ordering with sufficiently unlucky interrupts as
demonstrated by test (:timer :parallel-unschedule) with low
probability.
This affects hash tables and some pcl locks.
Also, use WITH-RECURSIVE-MUTEX for packages.
Not a spinlock becuase it can be held for a long time and not a system
lock (i.e. with WITHOUT-INTERRUPTS) because conflicts are signalled
while holding the lock which I think this warrants a FIXME.
diff --git a/src/code/hash-table.lisp b/src/code/hash-table.lisp
index ca66744..784bc11 100644
--- a/src/code/hash-table.lisp
+++ b/src/code/hash-table.lisp
@@ -147,5 +147,6 @@ unspecified."
;; Needless to say, this also excludes some internal bits, but
;; getting there is too much detail when "unspecified" says what
;; is important -- unpredictable, but harmless.
- `(sb!thread::with-recursive-spinlock ((hash-table-spinlock ,hash-table))
+ `(sb!thread::with-recursive-system-spinlock
+ ((hash-table-spinlock ,hash-table))
,@body))
diff --git a/src/pcl/std-class.lisp b/src/pcl/std-class.lisp
index bf80f1b..b077674 100644
--- a/src/pcl/std-class.lisp
+++ b/src/pcl/std-class.lisp
@@ -181,13 +181,13 @@
(defmethod add-direct-method :around ((specializer specializer) method)
;; All the actions done under this lock are done in an order
;; that is safe to unwind at any point.
- (sb-thread::with-recursive-spinlock (*specializer-lock*)
+ (sb-thread::with-recursive-system-spinlock (*specializer-lock*)
(call-next-method)))
(defmethod remove-direct-method :around ((specializer specializer) method)
;; All the actions done under this lock are done in an order
;; that is safe to unwind at any point.
- (sb-thread::with-recursive-spinlock (*specializer-lock*)
+ (sb-thread::with-recursive-system-spinlock (*specializer-lock*)
(call-next-method)))
(defmethod add-direct-method ((specializer class) (method method))

On 28 May 2011 22:14, Nikodemus Siivola <nikodemus@...> wrote:
> WITH-LOCKED-HASH-TABLE should IMO be considered a user lock. To me it
> is morally analogous to a user have an external lock and grabbing that
> around accesses to a hash-table.
> I'm not sure if the meaning here is that SBCL's internal hash-tables
> should keep interrupts disabled -- which I can certainly understand --
With that theory in mind:
https://github.com/nikodemus/SBCL/tree/with-locked-system-table
Cheers,
-- Nikodemus

Nikodemus Siivola <nikodemus@...> writes:
> As of 1.0.25.15 (below for convenience) WITH-LOCKED-HASH-TABLE also
> disables interrupts for its body.
>
>>From the commit message it is not clear to me why.
>
> WITH-LOCKED-HASH-TABLE should IMO be considered a user lock. To me it
> is morally analogous to a user have an external lock and grabbing that
> around accesses to a hash-table.
I agree.
> I'm not sure if the meaning here is that SBCL's internal hash-tables
> should keep interrupts disabled -- which I can certainly understand --
I think this was the intention.
> or if there is something non-obvious in the hash-table code that makes
> interrupts arriving during a locked section fundamentally unsafe?
It doesn't tickle my memory, and I almost surely would have mentioned
such a thing in the commit message, had what you are asking about been
found to be the case.
Cheers,
Gabor

2011/5/29 Gábor Melis <mega@...>:
>> or if there is something non-obvious in the hash-table code that makes
>> interrupts arriving during a locked section fundamentally unsafe?
>
> It doesn't tickle my memory, and I almost surely would have mentioned
> such a thing in the commit message, had what you are asking about been
> found to be the case.
Thanks! I made the WITH-LOCKED-SYSTEM-TABLE change in 1.0.48.31.
Cheers,
-- nikodemus