Page MenuHomeFreeBSD

Handle cas failure when the compare succeeds
ClosedPublic

Authored by andrew on Mon, May 9, 3:02 PM.

Details

Summary

When locking a priority inherit mutex we perform a compare and swap
operation to try and acquire the mutex. This may fail even when the
compare succeeds.

Check and handle this case.

PR: 263825

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

andrew requested review of this revision.Mon, May 9, 3:02 PM

I checked for the described condition by testing for rv == 1. Then it does not need this long comment.

sys/kern/kern_umtx.c
2268

rv == 1 could be because the compare failed, or the store failed. The check is for the store case that was missing.

This revision is now accepted and ready to land.Fri, May 13, 2:10 PM
dchagin added inline comments.
sys/kern/kern_umtx.c
2268

May be put this under #if defined (arm)? Do we have some common define for ll/sc?

sys/kern/kern_umtx.c
2268

I don't believe we have any such define. It is not really possible to write one, I think: arm64 has two sets of atomic RMW operations, one using LL/SC (required) and one using atomic instructions (part of the LSE extension, not required in all ARMv8 implementations). We don't know at compile-time which flavour will be used; the kernel will look at the CPU capabilities and use LSE if available.

sys/kern/kern_umtx.c
2268

Oops, the last sentence is wrong, it applies only to atomic(9). Currently casueword32() is always implemented using LL/SC atomics. But I think my argument still holds. We should possibly provide LSE-based implementations of casueword32() etc., since some ARMv8 implementations (graviton 2 in particular) have significantly better performance with LSE.

Is there any problem if userspace uses LL/SC atomics and the kernel uses LSE atomics when both are modifying the same word? I'd hope not...

sys/kern/kern_umtx.c
2268

Thank you for your comprehensive answer.
The same fix is needed for linux_futex_lock_pi as it is modeled after umtx. I'm plan to ifdef it under aarch64.

sys/kern/kern_umtx.c
2268

Why the ifdef? What about other LL/SC platforms, like riscv? IMHO it is fine to simply check unconditionally.

sys/kern/kern_umtx.c
2268

If we think this is a problem we could add a macro for architectures where the casu functions may use ll/sc atomics.