Details

Reviewers

kib
olce
kevans

Commits

rG4b79443927ec: umtx: Fix a bug in do_lock_pp()

Summary

If the lock is unowned (i.e., owner == UMUTEX_CONTESTED), we might get a
spurious failure, and in that case we need to retry the loop.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 62524
Build 59408: arc lint + arc unit

Event Timeline

markj created this revision.Mon, Feb 17, 4:24 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, Feb 17, 4:24 PM

markj requested review of this revision.Mon, Feb 17, 4:24 PM

Harbormaster completed remote builds in B62485: Diff 151110.Mon, Feb 17, 4:25 PM

kib accepted this revision.Tue, Feb 18, 8:05 AM

This revision is now accepted and ready to land.Tue, Feb 18, 8:05 AM

Looks good but I have a question (please see the inline comment).

sys/kern/kern_umtx.c
2634–2635	I'm wondering if this block shouldn't be moved before the `if (try != 0)` just above, as `EBUSY` is supposed to only be returned if the target mutex is known to be locked as per POSIX.

Ah, and since the sequence of 4 calls umtx_lock()/umtx_unbusy()/umtx_remove()/umtx_unlock() is repeated 3 times, I'd suggest factoring it out in a small helper.

Unconditionally retry after a spurious failure.

Fix the same bugs in do_set_ceiling(), which internally acquires a PP mutex
in order to update lock state.

This revision now requires review to proceed.Tue, Feb 18, 2:35 PM

Harbormaster completed remote builds in B62499: Diff 151141.Tue, Feb 18, 2:35 PM

In D49031#1118249, @olce wrote:

Ah, and since the sequence of 4 calls umtx_lock()/umtx_unbusy()/umtx_remove()/umtx_unlock() is repeated 3 times, I'd suggest factoring it out in a small helper.

There's quite a lot of duplication of this and related sequences across all of the umtx routines, not just here. I'll try to do a cleanup pass after this bugfix lands.

sys/kern/kern_umtx.c
2634–2635	That would be consistent with handling of regular umutexes, yes.

In D49031#1118281, @markj wrote:

There's quite a lot of duplication of this and related sequences across all of the umtx routines, not just here.

Indeed. And I'm pretty sure other bugs are lurking, albeit less important ones.

I'll try to do a cleanup pass after this bugfix lands.

Would be great. Will help by reviewing if you do so.

This revision is now accepted and ready to land.Tue, Feb 18, 2:49 PM

Enqueue the thread earlier in the loop to avoid missed wakeups. In particular, ensure that we call umtxq_insert() before attempting the CAS.

Actually, this isn't necessary. do_unlock_pp() busies the chain around its store and wakeup, so the race I was thinking of can't happen.

Revert part of the change: it's not necessary to insert the
queue entry earlier after all, since the busy bit provides
enough synchronization.

This revision now requires review to proceed.Tue, Feb 18, 2:54 PM

Harbormaster completed remote builds in B62500: Diff 151142.Tue, Feb 18, 2:54 PM

markj retitled this revision from umtx: Fix a couple of bugs in do_lock_pp() to umtx: Fix a bug in do_lock_pp().Tue, Feb 18, 3:06 PM

markj edited the summary of this revision. (Show Details)

In D49031#1118290, @markj wrote:

Revert part of the change: it's not necessary to insert the
queue entry earlier after all, since the busy bit provides
enough synchronization.

Oh, that's right. And now that you point that out, it seems to ring a bell. I seem to recall having analyzed these functions quite in details, but unfortunately could not find any notes on that work.

sys/kern/kern_umtx.c
2601–2616	Removing `error = 0;` is an unrelated change. I think it is correct, as the code that would have been executed at the end of the `for` block (priority restore) is also present after it under a guard that passes. However, the comment above should be amended (e.g., by removing the outdated text).
2635–2642	I would move this as an `else` for `if (owner == UMUTEX_RB_NOTRECOV)` above (and then not change the original code around). Is there any specific reason to put it here instead?

Simplify control flow.

sys/kern/kern_umtx.c
2601–2616	I think I'll just leave this alone, as I'm not really set up to test the robust mutex case.

Harbormaster completed remote builds in B62515: Diff 151188.Wed, Feb 19, 2:57 PM

olce added inline comments.Wed, Feb 19, 4:09 PM

sys/kern/kern_umtx.c
2844–2846	For consistency with `do_lock_pp()` (of behavior foremost, but code also), I'd move this before the `if (error != 0)`, e.g., in the form of an `else` of `if (owner == UMUTEX_RB_NOTRECOV)`.