Page MenuHomeFreeBSD

Fix pointer loads in uma_vm_zone_stats().
ClosedPublic

Authored by markj on Oct 18 2019, 4:51 PM.

Details

Summary

Ensure that each bucket pointer is loaded only once.
This happens already in practice, but without a change
like this one the compiler is permitted to issue two loads,
resulting in a race condition. This can happen when compiling
with -O0, for instance.

Diff Detail

Repository
rS 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

markj created this revision.Oct 18 2019, 4:51 PM
markj updated this revision to Diff 63452.Oct 18 2019, 4:56 PM

Restore the original loop, we need to always zero the structure.

markj updated this revision to Diff 63453.Oct 18 2019, 4:57 PM

Fix the right loop this time.

markj updated this revision to Diff 63454.Oct 18 2019, 4:57 PM

One more try.

markj edited the summary of this revision. (Show Details)Oct 18 2019, 5:15 PM
markj added reviewers: alc, jeff, kib, dougm.
markj added a subscriber: pho.Oct 18 2019, 5:22 PM

Peter, could you post the disassembly of uma_vm_zone_stats() from your -O0 kernel?

kib accepted this revision.Oct 18 2019, 5:33 PM

Perhaps adding a sentence to the preceding comment explaining why atomic_load would be useful.

This revision is now accepted and ready to land.Oct 18 2019, 5:33 PM
markj updated this revision to Diff 63458.Oct 18 2019, 5:39 PM

Add a comment.

This revision now requires review to proceed.Oct 18 2019, 5:39 PM
pho added a comment.Oct 18 2019, 5:54 PM

Peter, could you post the disassembly of uma_vm_zone_stats() from your -O0 kernel?

Sure.

markj added a comment.Oct 18 2019, 6:00 PM
In D22081#482499, @pho wrote:

Peter, could you post the disassembly of uma_vm_zone_stats() from your -O0 kernel?

Sure.

Thanks. Indeed, that code contains a race that should be fixed by this diff.

pho added a comment.Oct 19 2019, 7:42 AM

I tested 63458 for 17 hours with a "-O0" build of uma_core.c. LGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 22 2019, 2:20 PM
This revision was automatically updated to reflect the committed changes.