Hello,
patch below is missing piece to stuff, which I commit on n2k18 [1].
Fairly quickly people, who have PF deployed with pfsync (and are
willing to experiment), discovered a panic on PF_LOCK recursion.
The recursion is identified by stack as follows:
login: panic: rw_enter: pf_lock locking against myself
Stopped at db_enter+0x12: popq %r11
TID PID UID PRFLAGS PFLAGS CPU COMMAND
147548 82409 0 0x100002 0 3 ld
*192136 5192 0 0x14000 0x200 0 softnet
db_enter() at db_enter+0x12
panic() at panic+0x120
_rw_enter() at _rw_enter+0x2a3
pfsync_update_state() at pfsync_update_state+0x90
pf_test() at pf_test+0x608
ip_output() at ip_output+0x6cd
pfsync_sendout() at pfsync_sendout+0x645
pfsync_bulk_start() at pfsync_bulk_start+0x127
pfsync_in_ureq() at pfsync_in_ureq+0x71
pfsync_input() at pfsync_input+0x3ab
ip_deliver() at ip_deliver+0x203
ipintr() at ipintr+0x6a
if_netisr(ffffffff813abf00) at if_netisr+0x5a
taskq_thread(0) at taskq_thread+0x6d
Big thanks goes to hrvoje@, who provided me with proper set up so
I could debug the patch. And also a fair amount of credit goes
to Christiano Haesbaert for idea to dispatch outbound psync update
packet to task.
The change in patch is conservative. The pfsync behavior does not
change unless PF gets compiled with '-DWITH_PF_LOCK' option. '-DWITH_PF_LOCK'
tells pfsync to use a softnet task to transmit packet with
update (see net/if.c [2])
Also let me clarify my earlier commit message [1] as I've received
few questions about it. At the moment my changes, which unlock PF
are experimental. The code is disabled by default. In order to
test my code you need to compile kernel with '-DWITH_PF_LOCK'. The
easiest thing is to patch CONFIG.MP with change below:
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/arch/amd64/conf/GENERIC.MP b/sys/arch/amd64/conf/GENERIC.MP
index 7f195a53092..69c62d23cf8 100644
--- a/sys/arch/amd64/conf/GENERIC.MP
+++ b/sys/arch/amd64/conf/GENERIC.MP
@@ -2,6 +2,7 @@
include "arch/amd64/conf/GENERIC"
+option WITH_PF_LOCK
option MULTIPROCESSOR
#option MP_LOCKDEBUG
#option WITNESS
--------8<---------------8<---------------8<------------------8<--------
Compiling kernel with -DWITH_PF_LOCK will get you PF with two rw-locks:
pf_lock, which serializes packets with ioctls. Currently pf_lock,
is always grabbed exclusively.
pf_state_lock, which packet grabs as a reader to look up a state,
if no state is found, then packet drops the pf_state_lock and proceeds
to rules. At that point packet grabs pf_lock as a writer. If the packet
matches a pass rule, then it must grab pf_state_lock as a writer in
order to insert a new state to table.
The pf_state_lock allows two pf_test() function to run in parallel for
packets, which match existing state. Packets, which don't match the state,
are serialized on pf_lock.
Mentioning parallelism: there is yet another change you need to perform
in order to get more pf_test() instances running. Currently there
is only single input task, which processes inbound packets. In order
to allow more input tasks one has to change NET_TASKQ constant found
in net/if.c. Patch below tells kernel to use two input tasks:
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/if.c b/sys/net/if.c
index 78d7d3f3af6..87fca3921f1 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -232,7 +232,7 @@ int ifq_congestion;
int netisr;
-#define NET_TASKQ 1
+#define NET_TASKQ 4
struct taskq *nettqmp[NET_TASKQ];
struct task if_input_task_locked = TASK_INITIALIZER(if_netisr, NULL);
--------8<---------------8<---------------8<------------------8<--------
Finally let me explain few details found in my patch:
@@ -288,6 +296,9 @@ pfsyncattach(int npfsync)
{
if_clone_attach(&pfsync_cloner);
pfsynccounters = counters_alloc(pfsyncs_ncounters);
+#ifdef WITH_PF_LOCK
+ mq_init(&pfsync_mq, 4096, IPL_SOFTNET);
+#endif /* WITH_PF_LOCK */
}
The queue limit of 4k is empirically determined by testing on
set up provided hrvoje@. The tests were running on intel 10Gb NICs.
Using 4k provides sufficient buffer to limit drops of pfsync
packets to <5%. The drops, which I could see when using 4k limit,
were coming from ip_output() here:
680 for (off = hlen + len; off < ntohs(ip->ip_len); off += len) {
681 MGETHDR(m, M_DONTWAIT, MT_HEADER);
682 if (m == NULL) {
683 ipstat_inc(ips_odropped);
684 error = ENOBUFS;
685 goto sendorfree;
686 }
clearly the system ran out of packet header buffers, so more tuning would
be needed. But this is something I'd like to keep out of scope of my
changes.
OK to pfsync change?
thanks and
regards
sashan
[1]
https://github.com/openbsd/src/commit/ddb22b9530d7ad1b4698803d7b6dcb21c96a3f7d#diff-9517dfce4e8db974781a4536fd38cfc1
[2] https://github.com/openbsd/src/blob/master/sys/net/if.c#L261
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index 8d86f4210fd..61f2a6df4cd 100644
--- a/sys/net/if_pfsync.c
+++ b/sys/net/if_pfsync.c
@@ -276,6 +276,14 @@ void pfsync_bulk_start(void);
void pfsync_bulk_status(u_int8_t);
void pfsync_bulk_update(void *);
void pfsync_bulk_fail(void *);
+#ifdef WITH_PF_LOCK
+void pfsync_send_dispatch(void *);
+void pfsync_send_pkt(struct mbuf *);
+
+static struct mbuf_queue pfsync_mq;
+static struct task pfsync_task =
+ TASK_INITIALIZER(pfsync_send_dispatch, &pfsync_mq);
+#endif /* WITH_PF_LOCK */
#define PFSYNC_MAX_BULKTRIES 12
int pfsync_sync_ok;
@@ -288,6 +296,9 @@ pfsyncattach(int npfsync)
{
if_clone_attach(&pfsync_cloner);
pfsynccounters = counters_alloc(pfsyncs_ncounters);
+#ifdef WITH_PF_LOCK
+ mq_init(&pfsync_mq, 4096, IPL_SOFTNET);
+#endif /* WITH_PF_LOCK */
}
int
@@ -1497,6 +1508,54 @@ pfsync_drop(struct pfsync_softc *sc)
sc->sc_len = PFSYNC_MINPKT;
}
+#ifdef WITH_PF_LOCK
+void
+pfsync_send_dispatch(void *xmq)
+{
+ struct mbuf_queue *mq = xmq;
+ struct pfsync_softc *sc;
+ struct mbuf *m;
+ struct mbuf_list ml;
+ int e;
+
+ mq_delist(mq, &ml);
+ if (ml_empty(&ml))
+ return;
+
+ NET_RLOCK();
+ sc = pfsyncif;
+ if (sc == NULL) {
+ ml_purge(&ml);
+ goto done;
+ }
+
+ while ((m = ml_dequeue(&ml)) != NULL) {
+ if ((e = ip_output(m, NULL, NULL, IP_RAWOUTPUT, &sc->sc_imo,
+ NULL, 0)) == 0)
+ pfsyncstat_inc(pfsyncs_opackets);
+ else {
+ DPFPRINTF(LOG_DEBUG,
+ "ip_output() @ %s failed (%d)\n", __func__, e);
+ pfsyncstat_inc(pfsyncs_oerrors);
+ }
+ }
+done:
+ NET_RUNLOCK();
+}
+
+void
+pfsync_send_pkt(struct mbuf *m)
+{
+ if (mq_enqueue(&pfsync_mq, m) != 0) {
+ pfsyncstat_inc(pfsyncs_oerrors);
+ DPFPRINTF(LOG_DEBUG, "mq_enqueue() @ %s failed, queue full\n",
+ __func__);
+ }
+ else
+ task_add(net_tq(0), &pfsync_task);
+}
+#endif /* WITH_PF_LOCK */
+
void
pfsync_sendout(void)
{
@@ -1669,10 +1728,14 @@ pfsync_sendout(void)
m->m_pkthdr.ph_rtableid = sc->sc_if.if_rdomain;
+#ifdef WITH_PF_LOCK
+ pfsync_send_pkt(m);
+#else /* !WITH_PF_LOCK */
if (ip_output(m, NULL, NULL, IP_RAWOUTPUT, &sc->sc_imo, NULL, 0) == 0)
pfsyncstat_inc(pfsyncs_opackets);
else
pfsyncstat_inc(pfsyncs_oerrors);
+#endif /* WITH_PF_LOCK */
}
void
