From 5a3b9507d784aaa6a7ce35432b2111a7eec12cba Mon Sep 17 00:00:00 2001 From: Kristof Provost Date: Sun, 13 Dec 2020 17:20:02 +0100 Subject: [PATCH] pf: Convert pfi_kkif to use counter_u64 Improve caching behaviour by using counter_u64 rather than variables shared between cores. The result of converting all counters to counter(9) (i.e. this full patch series) is a significant improvement in throughput. As tested by olivier@, on Intel Xeon E5-2697Av4 (16Cores, 32 threads) hardware with Mellanox ConnectX-4 MCX416A-CCAT (100GBase-SR4) nics we see: x FreeBSD 20201223: inet packets-per-second + FreeBSD 20201223 with pf patches: inet packets-per-second +--------------------------------------------------------------------------+ | + | | xx + | |xxx +++| ||A| | | |A|| +--------------------------------------------------------------------------+ N Min Max Median Avg Stddev x 5 9216962 9526356 9343902 9371057.6 116720.36 + 5 19427190 19698400 19502922 19546509 109084.92 Difference at 95.0% confidence 1.01755e+07 +/- 164756 108.584% +/- 2.9359% (Student's t, pooled s = 112967) Reviewed by: philip MFC after: 2 weeks Sponsored by: Orange Business Services Differential Revision: https://reviews.freebsd.org/D27763 --- sys/net/pfvar.h | 5 ++-- sys/netpfil/pf/pf.c | 12 ++++++--- sys/netpfil/pf/pf_if.c | 61 +++++++++++++++++++++++++++++++++++------- 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index 65328941ff01..c7df8ffd82c5 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -879,8 +879,8 @@ struct pfi_kkif { } _pfik_glue; #define pfik_tree _pfik_glue._pfik_tree #define pfik_list _pfik_glue._pfik_list - u_int64_t pfik_packets[2][2][2]; - u_int64_t pfik_bytes[2][2][2]; + counter_u64_t pfik_packets[2][2][2]; + counter_u64_t pfik_bytes[2][2][2]; u_int32_t pfik_tzero; u_int pfik_flags; struct ifnet *pfik_ifp; @@ -1678,6 +1678,7 @@ struct pf_state_key *pf_state_key_clone(struct pf_state_key *); struct pfi_kkif *pf_kkif_create(int); void pf_kkif_free(struct pfi_kkif *); +void pf_kkif_zero(struct pfi_kkif *); #endif /* _KERNEL */ #endif /* _NET_PFVAR_H_ */ diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index 6f4ccb99ad1f..4cccb0101650 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -6196,8 +6196,10 @@ pf_test(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb * (s == NULL)); } - kif->pfik_bytes[0][dir == PF_OUT][action != PF_PASS] += pd.tot_len; - kif->pfik_packets[0][dir == PF_OUT][action != PF_PASS]++; + counter_u64_add(kif->pfik_bytes[0][dir == PF_OUT][action != PF_PASS], + pd.tot_len); + counter_u64_add(kif->pfik_packets[0][dir == PF_OUT][action != PF_PASS], + 1); if (action == PF_PASS || r->action == PF_DROP) { dirndx = (dir == PF_OUT); @@ -6598,8 +6600,10 @@ pf_test6(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb &pd, (s == NULL)); } - kif->pfik_bytes[1][dir == PF_OUT][action != PF_PASS] += pd.tot_len; - kif->pfik_packets[1][dir == PF_OUT][action != PF_PASS]++; + counter_u64_add(kif->pfik_bytes[1][dir == PF_OUT][action != PF_PASS], + pd.tot_len); + counter_u64_add(kif->pfik_packets[1][dir == PF_OUT][action != PF_PASS], + 1); if (action == PF_PASS || r->action == PF_DROP) { dirndx = (dir == PF_OUT); diff --git a/sys/netpfil/pf/pf_if.c b/sys/netpfil/pf/pf_if.c index 5c389d7a45c5..850a6dc1ca05 100644 --- a/sys/netpfil/pf/pf_if.c +++ b/sys/netpfil/pf/pf_if.c @@ -223,7 +223,26 @@ pf_kkif_create(int flags) { struct pfi_kkif *kif; - kif = malloc(sizeof(*kif), PFI_MTYPE, flags); + kif = malloc(sizeof(*kif), PFI_MTYPE, flags | M_ZERO); + if (! kif) + return (kif); + + for (int i = 0; i < 2; i++) { + for (int j = 0; j < 2; j++) { + for (int k = 0; k < 2; k++) { + kif->pfik_packets[i][j][k] = + counter_u64_alloc(flags); + kif->pfik_bytes[i][j][k] = + counter_u64_alloc(flags); + + if (! kif->pfik_packets[i][j][k] || + ! kif->pfik_bytes[i][j][k]) { + pf_kkif_free(kif); + return (NULL); + } + } + } + } return (kif); } @@ -234,9 +253,35 @@ pf_kkif_free(struct pfi_kkif *kif) if (! kif) return; + for (int i = 0; i < 2; i++) { + for (int j = 0; j < 2; j++) { + for (int k = 0; k < 2; k++) { + if (kif->pfik_packets[i][j][k]) + counter_u64_free(kif->pfik_packets[i][j][k]); + if (kif->pfik_bytes[i][j][k]) + counter_u64_free(kif->pfik_bytes[i][j][k]); + } + } + } + free(kif, PFI_MTYPE); } +void +pf_kkif_zero(struct pfi_kkif *kif) +{ + + for (int i = 0; i < 2; i++) { + for (int j = 0; j < 2; j++) { + for (int k = 0; k < 2; k++) { + counter_u64_zero(kif->pfik_packets[i][j][k]); + counter_u64_zero(kif->pfik_bytes[i][j][k]); + } + } + } + kif->pfik_tzero = time_second; +} + struct pfi_kkif * pfi_kkif_find(const char *kif_name) { @@ -264,7 +309,7 @@ pfi_kkif_attach(struct pfi_kkif *kif, const char *kif_name) return (kif1); } - bzero(kif, sizeof(*kif)); + pf_kkif_zero(kif); strlcpy(kif->pfik_name, kif_name, sizeof(kif->pfik_name)); /* * It seems that the value of time_second is in unintialzied state @@ -754,18 +799,16 @@ pfi_update_status(const char *name, struct pf_status *pfs) /* just clear statistics */ if (pfs == NULL) { - bzero(p->pfik_packets, sizeof(p->pfik_packets)); - bzero(p->pfik_bytes, sizeof(p->pfik_bytes)); - p->pfik_tzero = time_second; + pf_kkif_zero(p); continue; } for (i = 0; i < 2; i++) for (j = 0; j < 2; j++) for (k = 0; k < 2; k++) { pfs->pcounters[i][j][k] += - p->pfik_packets[i][j][k]; + counter_u64_fetch(p->pfik_packets[i][j][k]); pfs->bcounters[i][j] += - p->pfik_bytes[i][j][k]; + counter_u64_fetch(p->pfik_bytes[i][j][k]); } } } @@ -780,9 +823,9 @@ pf_kkif_to_kif(const struct pfi_kkif *kkif, struct pfi_kif *kif) for (int j = 0; j < 2; j++) { for (int k = 0; k < 2; k++) { kif->pfik_packets[i][j][k] = - kkif->pfik_packets[i][j][k]; + counter_u64_fetch(kkif->pfik_packets[i][j][k]); kif->pfik_bytes[i][j][k] = - kkif->pfik_bytes[i][j][k]; + counter_u64_fetch(kkif->pfik_bytes[i][j][k]); } } }