netmap: Tell the compiler to avoid reloading ring indices
ClosedPublic
Actions

Authored by markj on Jan 15 2023, 5:03 PM.

Details

Reviewers

Commits

rGb547900689ec: netmap: Tell the compiler to avoid reloading ring indices
rG56c438fcd4a7: netmap: Tell the compiler to avoid reloading ring indices

Summary

Per the removed comments these fields should be loaded only once, since
they can in principle by modified concurrently. Use atomic(9) to
declare this intent to the compiler. No functional change intended.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Jan 15 2023, 5:03 PM

Herald added a subscriber: imp. · View Herald TranscriptJan 15 2023, 5:03 PM

markj requested review of this revision.Jan 15 2023, 5:03 PM

Harbormaster completed remote builds in B49113: Diff 115152.Jan 15 2023, 5:03 PM

markj added a child revision: D38062: netmap: Fix a queue length check in the generic port rx path.Jan 15 2023, 5:03 PM

Keep in mind that if the user writes to cur and head while the syscall is executing, then the user is breaking the ABI and the result is undefined (but no harm can happen because of the checks of the prologue).

However, we have a NM_ACCESS_ONCE macro to be portable across Linux and FreeBSD. It looks like it is equivalent to atomic_load_32 on FreeBSD (right?)
If you feel, you could at most change the definition for NM_ACCESS_ONCE #ifdef FreeBSD.

In D38061#865725, @vmaffione wrote:

Keep in mind that if the user writes to cur and head while the syscall is executing, then the user is breaking the ABI and the result is undefined (but no harm can happen because of the checks of the prologue).

Yes, that's true. Perhaps we should simply remove the (somewhat misleading) comments instead, since it looks like a bug but isn't.

However, we have a NM_ACCESS_ONCE macro to be portable across Linux and FreeBSD. It looks like it is equivalent to atomic_load_32 on FreeBSD (right?)
If you feel, you could at most change the definition for NM_ACCESS_ONCE #ifdef FreeBSD.

We don't have a generic atomic_load(), which is what we'd want there. Perhaps it's best to just drop this patch.

I'm not sure on what you mean about the generic atomic_load...
Anyway, the original meaning of the comment is "Load the variable(s) only once, in order to avoid TOCTOU issues". We may update it to make it more clear.

In D38061#865751, @vmaffione wrote:

I'm not sure on what you mean about the generic atomic_load...

Sorry, I misunderstood your comment. I thought that NM_READ_ONCE() had a Linux-specific implementation, but we can use it on FreeBSD too.

Anyway, the original meaning of the comment is "Load the variable(s) only once, in order to avoid TOCTOU issues". We may update it to make it more clear.

I think that using NM_READ_ONCE() is fine. I'll update the diff.