Record part of the owner struct thread pointer into busy_lock.
ClosedPublic
Actions

Authored by kib on Nov 9 2019, 8:05 PM.

Details

Reviewers

alc
markj
jeff

Commits

rS355062: Record part of the owner struct thread pointer into busy_lock.

Summary

Record as much bits from curthread into busy_lock. Low bits for struct thread * representation are zero due to struct and zone alignment, and they leave space for busy flags (perhaps except statically allocated thread0). Upper bits are not very interesting for assert, and in most practical situations recorded value should allow to identify the owner with certainty.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib created this revision.Nov 9 2019, 8:05 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 9 2019, 8:05 PM

Harbormaster completed remote builds in B27420: Diff 64125.Nov 9 2019, 8:05 PM

I used 'tid' for debugging in the past. You can get the whole tid in. We can do somewhat more meaningful asserts with that but we'd have to deal with I/O ala BUF_KERNPROC

I don't know what the significance of the R postfix on "EXCLUSIVER" means. If you are going to change these macros I would suggest we rename it to EXCLUSIVE.

Is there a bug you are chasing?

In D22298#487577, @jeff wrote:

I used 'tid' for debugging in the past. You can get the whole tid in. We can do somewhat more meaningful asserts with that but we'd have to deal with I/O ala BUF_KERNPROC

I considered using tid, and I did not like it. The bit pattern for curthread allows to immediately recognize the value, while tid would need to be extracted. Also, tid could be reused, while struct thread is type-stable.

I don't know what the significance of the R postfix on "EXCLUSIVER" means. If you are going to change these macros I would suggest we rename it to EXCLUSIVE.

Ok.

Is there a bug you are chasing?

I principle yes, but I am not sure that this would directly points the finger. Still it should be useful.

jeff added inline comments.Nov 9 2019, 9:33 PM

sys/vm/vm_page.h
692 ↗	(On Diff #64125)	This assert may not work in all cases as written due to busy being released in a different thread than the one that acquired it.

Drop 'R'.

Harbormaster completed remote builds in B27421: Diff 64132.Nov 9 2019, 9:34 PM

kib added inline comments.Nov 9 2019, 10:01 PM

sys/vm/vm_page.h
692 ↗	(On Diff #64125)	I do not remember such places. I think I will introduce weaker asserts in addition to this stronger kind.

jeff added inline comments.Nov 9 2019, 10:49 PM

sys/vm/vm_page.h
692 ↗	(On Diff #64125)	I only know of specific examples with sbusy. I'm not 100% sure it does happen with xbusy. If it passes stress tests it's probably ok.