Skip to content

Commit

Permalink
pf: Stop using net_epoch to synchronize access to eth rules
Browse files Browse the repository at this point in the history
Commit 20c4899 modified pf_test_eth_rule() to not acquire the
rules read lock, so pf_commit_eth() was changed to wait until the
now-inactive rules are no longer in use before freeing them.  In
particular, it uses the net_epoch to schedule callbacks once the
inactive rules are no longer visible to packet processing threads.

However, since commit 812839e, pf_test_eth_rule() acquires the
rules read lock, so this deferred action is unneeded.  This patch
reverts a portion of 20c4899 such that we avoid using deferred
callbacks to free inactive rules.

The main motivation is performance: epoch_drain_callbacks() is quite
slow, especially on busy systems, and its use in the DIOCXBEGIN handler
in particular causes long stalls in relayd when reloading configuration.

Reviewed by:	kp
MFC after:	2 weeks
Sponsored by:	Klara, Inc.
Sponsored by:	Modirum MDPay
Differential Revision:	https://reviews.freebsd.org/D48822

(cherry picked from commit 7a66b3008693ce61957e8b2a3d99829063e1e4af)
  • Loading branch information
markjdb authored and fichtner committed Feb 21, 2025
1 parent 97415db commit 1114065
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 36 deletions.
1 change: 0 additions & 1 deletion sys/net/pfvar.h
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,6 @@ struct pf_keth_ruleset {
int open;
uint32_t ticket;
} active, inactive;
struct epoch_context epoch_ctx;
struct vnet *vnet;
struct pf_keth_anchor *anchor;
};
Expand Down
9 changes: 3 additions & 6 deletions sys/netpfil/pf/pf.c
Original file line number Diff line number Diff line change
Expand Up @@ -4436,11 +4436,6 @@ pf_test_eth_rule(int dir, struct pfi_kkif *kif, struct mbuf **m0)
return (PF_PASS);
}

ruleset = V_pf_keth;
rules = ck_pr_load_ptr(&ruleset->active.rules);
r = TAILQ_FIRST(rules);
rm = NULL;

e = mtod(m, struct ether_header *);
proto = ntohs(e->ether_type);

Expand Down Expand Up @@ -4477,7 +4472,9 @@ pf_test_eth_rule(int dir, struct pfi_kkif *kif, struct mbuf **m0)

PF_RULES_RLOCK();

while (r != NULL) {
ruleset = V_pf_keth;
rules = atomic_load_ptr(&ruleset->active.rules);
for (r = TAILQ_FIRST(rules), rm = NULL; r != NULL;) {
counter_u64_add(r->evaluations, 1);
SDT_PROBE2(pf, eth, test_rule, test, r->nr, r);

Expand Down
32 changes: 3 additions & 29 deletions sys/netpfil/pf/pf_ioctl.c
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ static void pf_empty_kpool(struct pf_kpalist *);
static int pfioctl(struct cdev *, u_long, caddr_t, int,
struct thread *);
static int pf_begin_eth(uint32_t *, const char *);
static void pf_rollback_eth_cb(struct epoch_context *);
static int pf_rollback_eth(uint32_t, const char *);
static int pf_commit_eth(uint32_t, const char *);
static void pf_free_eth_rule(struct pf_keth_rule *);
Expand Down Expand Up @@ -782,23 +781,6 @@ pf_begin_eth(uint32_t *ticket, const char *anchor)
return (0);
}

static void
pf_rollback_eth_cb(struct epoch_context *ctx)
{
struct pf_keth_ruleset *rs;

rs = __containerof(ctx, struct pf_keth_ruleset, epoch_ctx);

CURVNET_SET(rs->vnet);

PF_RULES_WLOCK();
pf_rollback_eth(rs->inactive.ticket,
rs->anchor ? rs->anchor->path : "");
PF_RULES_WUNLOCK();

CURVNET_RESTORE();
}

static int
pf_rollback_eth(uint32_t ticket, const char *anchor)
{
Expand Down Expand Up @@ -892,15 +874,12 @@ pf_commit_eth(uint32_t ticket, const char *anchor)
pf_eth_calc_skip_steps(rs->inactive.rules);

rules = rs->active.rules;
ck_pr_store_ptr(&rs->active.rules, rs->inactive.rules);
atomic_store_ptr(&rs->active.rules, rs->inactive.rules);
rs->inactive.rules = rules;
rs->inactive.ticket = rs->active.ticket;

/* Clean up inactive rules (i.e. previously active rules), only when
* we're sure they're no longer used. */
NET_EPOCH_CALL(pf_rollback_eth_cb, &rs->epoch_ctx);

return (0);
return (pf_rollback_eth(rs->inactive.ticket,
rs->anchor ? rs->anchor->path : ""));
}

#ifdef ALTQ
Expand Down Expand Up @@ -5208,8 +5187,6 @@ pfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags, struct thread *td
free(ioes, M_TEMP);
break;
}
/* Ensure there's no more ethernet rules to clean up. */
NET_EPOCH_DRAIN_CALLBACKS();
PF_RULES_WLOCK();
for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
ioe->anchor[sizeof(ioe->anchor) - 1] = '\0';
Expand Down Expand Up @@ -6852,9 +6829,6 @@ pf_unload_vnet(void)
shutdown_pf();
PF_RULES_WUNLOCK();

/* Make sure we've cleaned up ethernet rules before we continue. */
NET_EPOCH_DRAIN_CALLBACKS();

ret = swi_remove(V_pf_swi_cookie);
MPASS(ret == 0);
ret = intr_event_destroy(V_pf_swi_ie);
Expand Down

0 comments on commit 1114065

Please sign in to comment.