On Tue, Mar 08, 2022 at 05:49:22PM +0100, Alexandr Nedvedicky wrote: > 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 ?
I have thrown it on my performance test machine with pfsync enabled. No issues seen. http://bluhm.genua.de/perform/results/2022-03-06T22%3A40%3A13Z/perform.html OK bluhm@ > --------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); > } >
