On Mon, 27 Mar 2023 02:46:31 GMT, David Holmes <dhol...@openjdk.org> wrote:
> > AFAIU, these places now could get outdated information from the contains > > function: either getting a false positive or a false negative when asking > > for a given object. But if I understand correctly, this had been the case > > before, too. Since the information about ownership may be outdated as soon > > as one read the BasicLock* address from the markwork. So these APIs do only > > report a state that once had been, but never the current state. > > Sure but my concern is not stale data, it is whether the lock-stack code may > crash (or otherwise misbehave) if there are concurrent changes to it whilst > it is being queried. Lockstack is a fixed sized array of oop with a ptr-sized member indicating the current offset. It is embedded into Thread. Modifying the LockStack (push, pop, remove) is non-atomic: write the element, then update the current offset. The contains function - the only function not only called from current thread (and we should assert that) - reads current offset, then iterates the whole array up to offset, comparing each oop with the given one. The only way I see this crashing is if we read the current offset in a half-written state. Not sure if any of our platforms read/write a pointer-sized field non-atomic. All other misreads - would "just" result in a time window where contains() gives the wrong answer. So we should read the offset atomically. Alternatively, have a second contains(), e.g. `contains_full()`, just for the sake of concurrent iteration, and let that one iterate the whole stack incl. unused slots. Iteration limits would be hardcoded offsets from Thread*, no need to read offset. The obvious disadvantage would be that we'd need to mark/zero out popped slots in release builds. ------------- PR Comment: https://git.openjdk.org/jdk/pull/10907#issuecomment-1484583227
