Hello,
my original idea was to have a one mutex per pfsync queue. however it creates
things more complicated then necessary for queues, which keep pf(4) state.
hrvoje@ hit some panics in this area recently. bluhm@ and I are still looking
at those issues. However there is the first change in order to make code more
simple: diff below merges the array of ->sc_mtx[] into a single mutex.
OK ?
thanks and
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index edeb2209161..9a92fc4d21f 100644
--- a/sys/net/if_pfsync.c
+++ b/sys/net/if_pfsync.c
@@ -212,7 +212,7 @@ struct pfsync_softc {
struct ip sc_template;
struct pf_state_queue sc_qs[PFSYNC_S_COUNT];
- struct mutex sc_mtx[PFSYNC_S_COUNT];
+ struct mutex sc_st_mtx;
size_t sc_len;
struct pfsync_upd_reqs sc_upd_req_list;
@@ -324,13 +324,6 @@ pfsync_clone_create(struct if_clone *ifc, int unit)
struct pfsync_softc *sc;
struct ifnet *ifp;
int q;
- static const char *mtx_names[] = {
- "iack_mtx",
- "upd_c_mtx",
- "del_mtx",
- "ins_mtx",
- "upd_mtx",
- "" };
if (unit != 0)
return (EINVAL);
@@ -338,10 +331,9 @@ pfsync_clone_create(struct if_clone *ifc, int unit)
pfsync_sync_ok = 1;
sc = malloc(sizeof(*pfsyncif), M_DEVBUF, M_WAITOK|M_ZERO);
- for (q = 0; q < PFSYNC_S_COUNT; q++) {
+ for (q = 0; q < PFSYNC_S_COUNT; q++)
TAILQ_INIT(&sc->sc_qs[q]);
- mtx_init_flags(&sc->sc_mtx[q], IPL_SOFTNET, mtx_names[q], 0);
- }
+ mtx_init_flags(&sc->sc_st_mtx, IPL_SOFTNET, "st_mtx", 0);
pool_init(&sc->sc_pool, PFSYNC_PLSIZE, 0, IPL_SOFTNET, 0, "pfsync",
NULL);
@@ -1585,9 +1577,7 @@ pfsync_grab_snapshot(struct pfsync_snapshot *sn, struct
pfsync_softc *sc)
sn->sn_sc = sc;
- for (q = 0; q < PFSYNC_S_COUNT; q++)
- mtx_enter(&sc->sc_mtx[q]);
-
+ mtx_enter(&sc->sc_st_mtx);
mtx_enter(&sc->sc_upd_req_mtx);
mtx_enter(&sc->sc_tdb_mtx);
@@ -1612,9 +1602,7 @@ pfsync_grab_snapshot(struct pfsync_snapshot *sn, struct
pfsync_softc *sc)
mtx_leave(&sc->sc_tdb_mtx);
mtx_leave(&sc->sc_upd_req_mtx);
-
- for (q = (PFSYNC_S_COUNT - 1); q >= 0; q--)
- mtx_leave(&sc->sc_mtx[q]);
+ mtx_leave(&sc->sc_st_mtx);
}
void
@@ -2420,14 +2408,14 @@ pfsync_q_ins(struct pf_state *st, int q)
panic("pfsync pkt len is too low %zd", sc->sc_len);
#endif
do {
- mtx_enter(&sc->sc_mtx[q]);
+ mtx_enter(&sc->sc_st_mtx);
/*
* If two threads are competing to insert the same state, then
* there must be just single winner.
*/
if (st->sync_state != PFSYNC_S_NONE) {
- mtx_leave(&sc->sc_mtx[q]);
+ mtx_leave(&sc->sc_st_mtx);
break;
}
@@ -2439,7 +2427,7 @@ pfsync_q_ins(struct pf_state *st, int q)
sc_len = atomic_add_long_nv(&sc->sc_len, nlen);
if (sc_len > sc->sc_if.if_mtu) {
atomic_sub_long(&sc->sc_len, nlen);
- mtx_leave(&sc->sc_mtx[q]);
+ mtx_leave(&sc->sc_st_mtx);
pfsync_sendout();
continue;
}
@@ -2448,7 +2436,7 @@ pfsync_q_ins(struct pf_state *st, int q)
TAILQ_INSERT_TAIL(&sc->sc_qs[q], st, sync_list);
st->sync_state = q;
- mtx_leave(&sc->sc_mtx[q]);
+ mtx_leave(&sc->sc_st_mtx);
} while (0);
}
@@ -2456,18 +2444,19 @@ void
pfsync_q_del(struct pf_state *st)
{
struct pfsync_softc *sc = pfsyncif;
- int q = st->sync_state;
+ int q;
KASSERT(st->sync_state != PFSYNC_S_NONE);
- mtx_enter(&sc->sc_mtx[q]);
+ mtx_enter(&sc->sc_st_mtx);
+ q = st->sync_state;
atomic_sub_long(&sc->sc_len, pfsync_qs[q].len);
TAILQ_REMOVE(&sc->sc_qs[q], st, sync_list);
if (TAILQ_EMPTY(&sc->sc_qs[q]))
atomic_sub_long(&sc->sc_len, sizeof (struct pfsync_subheader));
- mtx_leave(&sc->sc_mtx[q]);
-
st->sync_state = PFSYNC_S_NONE;
+ mtx_leave(&sc->sc_st_mtx);
+
pf_state_unref(st);
}
