On Sat, Feb 18, 2017 at 06:06:01PM +0100, Jeremie Courreges-Anglas wrote:
>
> This one is a bit weird, the driver doesn't just increment the stats but
> also uses them at runtime, hence the additional helper functions.
I'm wondering if we should just drop the reading.
We have two cases, the init case and the packet sending case.
First the sending case:
Isn't this always true?
if (pflowstats.pflow_flows == sc->sc_gcounter)
If yes we can just skip that and do the inc.
The init case tries to preserve the flow counter betwen ifdown/ifup
Maybe we should just init the global counter to 0, like on reboot.
Benno?
>
> ok?
>
>
> Index: net/if_pflow.h
> ===================================================================
> RCS file: /d/cvs/src/sys/net/if_pflow.h,v
> retrieving revision 1.15
> diff -u -p -r1.15 if_pflow.h
> --- net/if_pflow.h 24 Jan 2017 10:08:30 -0000 1.15
> +++ net/if_pflow.h 18 Feb 2017 13:41:35 -0000
> @@ -263,6 +263,25 @@ struct pflowreq {
> };
>
> #ifdef _KERNEL
> +
> +#include <sys/percpu.h>
> +
> +enum pflowstat_counters {
> + pflows_flows,
> + pflows_packets,
> + pflows_onomem,
> + pflows_oerrors,
> + pflows_ncounters,
> +};
> +
> +extern struct cpumem *pflowcounters;
> +
> +static inline void
> +pflowstat_inc(enum pflowstat_counters c)
> +{
> + counters_inc(pflowcounters, c);
> +}
> +
> int export_pflow(struct pf_state *);
> int pflow_sysctl(int *, u_int, void *, size_t *, void *, size_t);
> #endif /* _KERNEL */
> Index: net/if_pflow.c
> ===================================================================
> RCS file: /d/cvs/src/sys/net/if_pflow.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 if_pflow.c
> --- net/if_pflow.c 16 Feb 2017 10:15:12 -0000 1.74
> +++ net/if_pflow.c 18 Feb 2017 13:41:35 -0000
> @@ -62,7 +62,7 @@
> #endif
>
> SLIST_HEAD(, pflow_softc) pflowif_list;
> -struct pflowstats pflowstats;
> +struct cpumem *pflowcounters;
>
> void pflowattach(int);
> int pflow_output(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst,
> @@ -73,6 +73,8 @@ int pflow_set(struct pflow_softc *, stru
> void pflow_init_timeouts(struct pflow_softc *);
> int pflow_calc_mtu(struct pflow_softc *, int, int);
> void pflow_setmtu(struct pflow_softc *, int);
> +uint64_t pflowstat_get_flows(void);
> +void pflowstat_update_flows(uint64_t);
> int pflowvalidsockaddr(const struct sockaddr *, int);
> int pflowioctl(struct ifnet *, u_long, caddr_t);
>
> @@ -114,6 +116,7 @@ pflowattach(int npflow)
> {
> SLIST_INIT(&pflowif_list);
> if_clone_attach(&pflow_cloner);
> + pflowcounters = counters_alloc(pflows_ncounters);
> }
>
> int
> @@ -283,6 +286,30 @@ pflow_clone_destroy(struct ifnet *ifp)
> return (error);
> }
>
> +uint64_t
> +pflowstat_get_flows(void)
> +{
> + uint64_t *counters, flows;
> + struct counters_ref ref;
> +
> + counters = counters_enter(&ref, pflowcounters);
> + flows = counters[pflows_flows];
> + counters_leave(&ref, pflowcounters);
> + return flows;
> +}
> +
> +void
> +pflowstat_update_flows(uint64_t gcounter)
> +{
> + uint64_t *counters;
> + struct counters_ref ref;
> +
> + counters = counters_enter(&ref, pflowcounters);
> + if (counters[pflows_flows] == gcounter)
> + counters[pflows_flows]++;
> + counters_leave(&ref, pflowcounters);
> +}
> +
> int
> pflowvalidsockaddr(const struct sockaddr *sa, int ignore_port)
> {
> @@ -460,7 +487,7 @@ pflowioctl(struct ifnet *ifp, u_long cmd
> case SIOCSIFFLAGS:
> if ((ifp->if_flags & IFF_UP) && sc->so != NULL) {
> ifp->if_flags |= IFF_RUNNING;
> - sc->sc_gcounter=pflowstats.pflow_flows;
> + sc->sc_gcounter = pflowstat_get_flows();
> /* send templates on startup */
> if (sc->sc_version == PFLOW_PROTO_10) {
> /* XXXSMP breaks atomicity */
> @@ -518,7 +545,7 @@ pflowioctl(struct ifnet *ifp, u_long cmd
>
> if ((ifp->if_flags & IFF_UP) && sc->so != NULL) {
> ifp->if_flags |= IFF_RUNNING;
> - sc->sc_gcounter=pflowstats.pflow_flows;
> + sc->sc_gcounter = pflowstat_get_flows();
> if (sc->sc_version == PFLOW_PROTO_10) {
> s = splnet();
> pflow_sendout_ipfix_tmpl(sc);
> @@ -619,14 +646,14 @@ pflow_get_mbuf(struct pflow_softc *sc, u
>
> MGETHDR(m, M_DONTWAIT, MT_DATA);
> if (m == NULL) {
> - pflowstats.pflow_onomem++;
> + pflowstat_inc(pflows_onomem);
> return (NULL);
> }
>
> MCLGET(m, M_DONTWAIT);
> if ((m->m_flags & M_EXT) == 0) {
> m_free(m);
> - pflowstats.pflow_onomem++;
> + pflowstat_inc(pflows_onomem);
> return (NULL);
> }
>
> @@ -865,8 +892,7 @@ copy_flow_to_m(struct pflow_flow *flow,
> (sc->sc_count * sizeof(struct pflow_flow)),
> sizeof(struct pflow_flow), flow, M_NOWAIT);
>
> - if (pflowstats.pflow_flows == sc->sc_gcounter)
> - pflowstats.pflow_flows++;
> + pflowstat_update_flows(sc->sc_gcounter);
> sc->sc_gcounter++;
> sc->sc_count++;
>
> @@ -896,8 +922,7 @@ copy_flow_ipfix_4_to_m(struct pflow_ipfi
> (sc->sc_count4 * sizeof(struct pflow_ipfix_flow4)),
> sizeof(struct pflow_ipfix_flow4), flow, M_NOWAIT);
>
> - if (pflowstats.pflow_flows == sc->sc_gcounter)
> - pflowstats.pflow_flows++;
> + pflowstat_update_flows(sc->sc_gcounter);
> sc->sc_gcounter++;
> sc->sc_count4++;
>
> @@ -926,8 +951,7 @@ copy_flow_ipfix_6_to_m(struct pflow_ipfi
> (sc->sc_count6 * sizeof(struct pflow_ipfix_flow6)),
> sizeof(struct pflow_ipfix_flow6), flow, M_NOWAIT);
>
> - if (pflowstats.pflow_flows == sc->sc_gcounter)
> - pflowstats.pflow_flows++;
> + pflowstat_update_flows(sc->sc_gcounter);
> sc->sc_gcounter++;
> sc->sc_count6++;
>
> @@ -1086,7 +1110,7 @@ pflow_sendout_v5(struct pflow_softc *sc)
> return (0);
> }
>
> - pflowstats.pflow_packets++;
> + pflowstat_inc(pflows_packets);
> h = mtod(m, struct pflow_header *);
> h->count = htons(sc->sc_count);
>
> @@ -1141,14 +1165,14 @@ pflow_sendout_ipfix(struct pflow_softc *
> return (0);
> }
>
> - pflowstats.pflow_packets++;
> + pflowstat_inc(pflows_packets);
> set_hdr = mtod(m, struct pflow_set_header *);
> set_hdr->set_length = htons(set_length);
>
> /* populate pflow_header */
> M_PREPEND(m, sizeof(struct pflow_v10_header), M_DONTWAIT);
> if (m == NULL) {
> - pflowstats.pflow_onomem++;
> + pflowstat_inc(pflows_onomem);
> return (ENOBUFS);
> }
> h10 = mtod(m, struct pflow_v10_header *);
> @@ -1182,12 +1206,12 @@ pflow_sendout_ipfix_tmpl(struct pflow_so
> m_freem(m);
> return (0);
> }
> - pflowstats.pflow_packets++;
> + pflowstat_inc(pflows_packets);
>
> /* populate pflow_header */
> M_PREPEND(m, sizeof(struct pflow_v10_header), M_DONTWAIT);
> if (m == NULL) {
> - pflowstats.pflow_onomem++;
> + pflowstat_inc(pflows_onomem);
> return (ENOBUFS);
> }
> h10 = mtod(m, struct pflow_v10_header *);
> @@ -1216,6 +1240,17 @@ pflow_sendout_mbuf(struct pflow_softc *s
> }
>
> int
> +pflow_sysctl_pflowstat(void *oldp, size_t *oldlenp, void *newp)
> +{
> + struct pflowstats pflowstat;
> +
> + CTASSERT(sizeof(pflowstat) == (pflows_ncounters * sizeof(uint64_t)));
> + counters_read(pflowcounters, (uint64_t *)&pflowstat, pflows_ncounters);
> + return (sysctl_rdstruct(oldp, oldlenp, newp,
> + &pflowstat, sizeof(pflowstat)));
> +}
> +
> +int
> pflow_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> void *newp, size_t newlen)
> {
> @@ -1224,10 +1259,7 @@ pflow_sysctl(int *name, u_int namelen, v
>
> switch (name[0]) {
> case NET_PFLOW_STATS:
> - if (newp != NULL)
> - return (EPERM);
> - return (sysctl_struct(oldp, oldlenp, newp, newlen,
> - &pflowstats, sizeof(pflowstats)));
> + return (pflow_sysctl_pflowstat(oldp, oldlenp, newp));
> default:
> return (EOPNOTSUPP);
> }
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
I'm not entirely sure you are real.
