pf: Avoid taking the pf rules write lock in a couple of ioctls
ClosedPublic
Actions

Authored by markj on Thu, Dec 18, 6:13 PM.

Details

Reviewers

kp
allanjude

Commits

rGae96ff302f8a: pf: Avoid taking the pf rules write lock in a couple of ioctls

Summary

The DIOCGETRULES ioctl handlers has taken the write lock ever since
fine-grained locking was merged to pf, but I believe it's unneeded. Use
the read lock instead.

DIOCGETRULENV takes the write lock as well but I believe this is only
required when clearing rule counters. (It might not be required even
then, on platforms where counter increment is done atomically.) Acquire
the read lock if that is not the case.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 69378
Build 66261: arc lint + arc unit

Event Timeline

markj created this revision.Thu, Dec 18, 6:13 PM

Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro and 2 others. · View Herald TranscriptThu, Dec 18, 6:13 PM

markj requested review of this revision.Thu, Dec 18, 6:13 PM

Harbormaster completed remote builds in B69378: Diff 168334.Thu, Dec 18, 6:13 PM

allanjude accepted this revision.Thu, Dec 18, 8:04 PM

This revision is now accepted and ready to land.Thu, Dec 18, 8:04 PM

DIOCGETRULENV takes the write lock as well but I believe this is only
required when clearing rule counters. (It might not be required even
then, on platforms where counter increment is done atomically.) Acquire
the read lock if that is not the case.

I believe the intent behind the write lock is to enable a userspace pattern of repeated fetch+clear without missing counts. If we'd take the read lock to retrieve the counters we could be incrementing them while this is going on, before we clear them again. That'd mean we'd miss packets/bytes/evaluations.

In D54292#1241041, @kp wrote:

DIOCGETRULENV takes the write lock as well but I believe this is only
required when clearing rule counters. (It might not be required even
then, on platforms where counter increment is done atomically.) Acquire
the read lock if that is not the case.

I believe the intent behind the write lock is to enable a userspace pattern of repeated fetch+clear without missing counts. If we'd take the read lock to retrieve the counters we could be incrementing them while this is going on, before we clear them again. That'd mean we'd miss packets/bytes/evaluations.

Ah, that makes sense.

Closed by commit rGae96ff302f8a: pf: Avoid taking the pf rules write lock in a couple of ioctls (authored by markj). · Explain WhyFri, Dec 19, 2:13 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rGae96ff302f8a: pf: Avoid taking the pf rules write lock in a couple of ioctls.