umtx: convert hand-rolled busy lock to sx lock
AbandonedPublic
Actions

Authored by mjg on Aug 8 2022, 7:24 PM.

Details

Reviewers

kib
markj

Summary

The current implementation spins for an arbitrary time and goes off cpu, which I can see on dtrace when doing -j 104 tinderbox on a kernel with other patches. Use an sx lock instead.

commit 1d17a05b7ecad6028d2ec9ee1e65d840dc4fafb1
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date:   Fri Aug 12 16:35:23 2022 +0000

    linux: employ umtxq_busy_unlocked
    
    Reviewed by:
    Differential Revision:

commit 2bdd0429def9352ce0820285a020b92eed709653
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date:   Thu Aug 4 16:08:32 2022 +0000

    umtx: employ umtxq_busy_unlocked

commit 05ed23714fbf385e299f8ba2a6702317b5014130
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date:   Thu Aug 4 16:04:02 2022 +0000

    umtx: convert busy to sx lock
    
    This avoids the highly pessimal scheme of arbitrarily spinning
    BUSY_SPINS times. In face of contention this almost always
    ends in going off cpu anyway.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mjg requested review of this revision.Aug 8 2022, 7:24 PM

mjg created this revision.

Does the linuxulator futex code need some changes as well? Looks like it is still using the umtxq_lock(); umtxq_busy(); pattern in some places.

If the busy lock becomes a proper mutex, then why exactly do we need both uc_busy and uc_lock?

The to linux emul will be made later, they are not *necessary* to keep things rolling.

To my reading umtx lock and busy lock have different scopes, despite most of the time being used together. The patch at hand is supposed to be the minimum which gets the work done.

patch futexes
macroify busy unlocked

mjg edited the summary of this revision. (Show Details)Aug 12 2022, 4:44 PM

mjg updated this revision to Diff 109256.Aug 12 2022, 4:47 PM

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

In D36083#820643, @mjg wrote:

To my reading umtx lock and busy lock have different scopes, despite most of the time being used together. The patch at hand is supposed to be the minimum which gets the work done.

I agree with Mark, it seems that you can get away with the single (sleepable) lock there. If umtxq_sleep() is changed to sxsleep(), I do not see much need in the separate mutex. This would half the number of atomics used, if worked, typically.

mjg abandoned this revision.Mar 12 2024, 10:33 PM