On 27/05/17(Sat) 11:33, Sebastian Benoit wrote:
> (benno_pflow_try3_1_task.diff)
>
> move sending of pflow packet into a task, seperated from the data
> collection by a mbuf queue.
Comments inline.
> diff --git sys/net/if_pflow.c sys/net/if_pflow.c
> index a40fe23862b..8cfffa1e4e7 100644
> --- sys/net/if_pflow.c
> +++ sys/net/if_pflow.c
> @@ -67,6 +67,7 @@ struct pflowstats pflowstats;
> void pflowattach(int);
> int pflow_output(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst,
> struct rtentry *rt);
> +void pflow_output_process(void *);
> int pflow_clone_create(struct if_clone *, int);
> int pflow_clone_destroy(struct ifnet *);
> int pflow_set(struct pflow_softc *, struct pflowreq *);
> @@ -124,11 +125,33 @@ pflow_output(struct ifnet *ifp, struct mbuf *m, struct
> sockaddr *dst,
> return (EAFNOSUPPORT);
> }
>
> +void
> +pflow_output_process(void *xifidx)
> +{
You can pass the softc pointer as argument because the task has the same
lifetime as its softc.
What protects you is the (missing) task_del() in clone_destroy().
> + unsigned int ifidx = (unsigned long)xifidx;
> + struct mbuf *m;
> + struct ifnet *ifp;
> + struct pflow_softc *sc;
> +
> + ifp = if_get(ifidx);
> + if (ifp == NULL)
> + return;
> + sc = ifp->if_softc;
> +
> + KERNEL_LOCK();
> + while ((m = mq_dequeue(&sc->sc_outputqueue)) != NULL) {
You want to use a mbuf_list, alas, ml_dequeue() because you don't need a
mutex protection.
> + pflow_sendout_mbuf(sc, m);
> + }
> + KERNEL_UNLOCK();
> + if_put(ifp);
> +}
> +
> int
> pflow_clone_create(struct if_clone *ifc, int unit)
> {
> struct ifnet *ifp;
> struct pflow_softc *pflowif;
> + unsigned long ifidx;
>
> if ((pflowif = malloc(sizeof(*pflowif),
> M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL)
> @@ -241,11 +264,15 @@ pflow_clone_create(struct if_clone *ifc, int unit)
> ifp->if_hdrlen = PFLOW_HDRLEN;
> ifp->if_flags = IFF_UP;
> ifp->if_flags &= ~IFF_RUNNING; /* not running, need receiver */
> + mq_init(&pflowif->sc_outputqueue, IFQ_MAXLEN, IPL_NET);
ml_init(9) :)
> pflow_setmtu(pflowif, ETHERMTU);
> pflow_init_timeouts(pflowif);
> if_attach(ifp);
> if_alloc_sadl(ifp);
>
> + ifidx = ifp->if_index;
> + task_set(&pflowif->sc_outputtask, pflow_output_process, (void *)ifidx);
> +
> /* Insert into list of pflows */
> SLIST_INSERT_HEAD(&pflowif_list, pflowif, sc_next);
Insertion in pflowif_list needs the NET_LOCK().
> return (0);
> @@ -255,11 +282,10 @@ int
> pflow_clone_destroy(struct ifnet *ifp)
> {
> struct pflow_softc *sc = ifp->if_softc;
> - int s, error;
> + int error;
>
> error = 0;
>
> - s = splnet();
> if (timeout_initialized(&sc->sc_tmo))
> timeout_del(&sc->sc_tmo);
> if (timeout_initialized(&sc->sc_tmo6))
> @@ -279,7 +305,6 @@ pflow_clone_destroy(struct ifnet *ifp)
> if_detach(ifp);
> SLIST_REMOVE(&pflowif_list, sc, pflow_softc, sc_next);
Deletion as well.
> free(sc, M_DEVBUF, sizeof(*sc));
> - splx(s);
> return (error);
> }
>
> @@ -311,7 +336,7 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
> struct proc *p = curproc;
> struct socket *so;
> struct sockaddr *sa;
> - int error = 0;
> + int s, error = 0;
>
> if (pflowr->addrmask & PFLOW_MASK_VERSION) {
> switch(pflowr->version) {
> @@ -407,6 +432,8 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
> }
>
> if (sc->so == NULL) {
Please leave the unlock/lock dance around pflow_set(). It is not a
problem.
The rest if fine! Thanks for taking care of this.
