On 10/11/17(Fri) 10:58, David Gwynne wrote: > this makes ifq_start try to wait for 4 packets before calling > if->if_qstart.
So you're adding back the IFXF_TXREADY mechanism that you removed in 2015 in r1.418. At that time we couldn't clearly see how to make it MP-safe. > this is based on work sephe did in dragonflybsd, and described in > a comment in their sys/net/if.c. there's a link to it here: > https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/net/if.c#L2976-L2987 > > the tests we've done generally show a performance bump, but otherwise > no degradation in performance. > > the mechanism for bundling packets is to but schedule a task to > service the queue later. if 4 packets get accumulated before the > task runs, it's cancelled and the code runs the start routine > directly. > > the most significant difference this implementation has to the dfly > one is that our ifqs dont (currently) track the number of bytes on > the q. dfly sends if it can bundle 4 packets, or up to mtu bytes > worth of packets. this implementation only looks at the number of > packets. IFXF_TXREADY had a different magic. Instead of 4 packets it looked at: min(8, ifp->if_snd.ifq_maxlen) It also checked for ifq_is_oactive()... Why is this logic no longer relevant? > the taskq the ifq uses is one of the softnets, which is assigned > when the ifq is initted and unconditionally used during the ifq's > lifetime. We're currently using net_tq() to distribute load for incoming packets. So I believe you should schedule the task on the current taskq or the first one if coming from userland. > because ifq work could now be pending in a softnet taskq, > ifq_barrier also needs to put a barrier in the taskq. this is > implemented using taskq_barrier, which i wrote ages ago but didn't > have a use case for at the time. > > tests? ok? > > Index: share/man/man9/task_add.9 > =================================================================== > RCS file: /cvs/src/share/man/man9/task_add.9,v > retrieving revision 1.16 > diff -u -p -r1.16 task_add.9 > --- share/man/man9/task_add.9 14 Sep 2015 15:14:55 -0000 1.16 > +++ share/man/man9/task_add.9 10 Nov 2017 00:45:41 -0000 > @@ -20,6 +20,7 @@ > .Sh NAME > .Nm taskq_create , > .Nm taskq_destroy , > +.Nm taskq_barrier , > .Nm task_set , > .Nm task_add , > .Nm task_del , > @@ -37,6 +38,8 @@ > .Ft void > .Fn taskq_destroy "struct taskq *tq" > .Ft void > +.Fn taskq_barrier "struct taskq *tq" > +.Ft void > .Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg" > .Ft int > .Fn task_add "struct taskq *tq" "struct task *t" > @@ -88,6 +91,15 @@ Calling > against the system taskq is an error and will lead to undefined > behaviour or a system fault. > .Pp > +.Fn taskq_barrier > +guarantees that any task that was running on the > +.Fa tq > +taskq when the barrier was called has finished by the time the barrier > +returns. > +.Fn taskq_barrier > +is only supported on taskqs serviced by 1 thread, > +and may not be called by a task running in the specified taskq. > +.Pp > It is the responsibility of the caller to provide the > .Fn task_set , > .Fn task_add , > @@ -163,6 +175,8 @@ argument given in > and > .Fn taskq_destroy > can be called during autoconf, or from process context. > +.Fn taskq_barrier > +can be called from process context. > .Fn task_set , > .Fn task_add , > and > Index: sys/sys/task.h > =================================================================== > RCS file: /cvs/src/sys/sys/task.h,v > retrieving revision 1.11 > diff -u -p -r1.11 task.h > --- sys/sys/task.h 7 Jun 2016 07:53:33 -0000 1.11 > +++ sys/sys/task.h 10 Nov 2017 00:45:41 -0000 > @@ -43,6 +43,7 @@ extern struct taskq *const systqmp; > > struct taskq *taskq_create(const char *, unsigned int, int, unsigned int); > void taskq_destroy(struct taskq *); > +void taskq_barrier(struct taskq *); > > void task_set(struct task *, void (*)(void *), void *); > int task_add(struct taskq *, struct task *); > Index: sys/kern/kern_task.c > =================================================================== > RCS file: /cvs/src/sys/kern/kern_task.c,v > retrieving revision 1.20 > diff -u -p -r1.20 kern_task.c > --- sys/kern/kern_task.c 30 Oct 2017 14:01:42 -0000 1.20 > +++ sys/kern/kern_task.c 10 Nov 2017 00:45:41 -0000 > @@ -22,6 +22,7 @@ > #include <sys/mutex.h> > #include <sys/kthread.h> > #include <sys/task.h> > +#include <sys/proc.h> > > #define TASK_ONQUEUE 1 > > @@ -68,6 +69,7 @@ struct taskq *const systqmp = &taskq_sys > > void taskq_init(void); /* called in init_main.c */ > void taskq_create_thread(void *); > +void taskq_barrier_task(void *); > int taskq_sleep(const volatile void *, struct mutex *, int, > const char *, int); > int taskq_next_work(struct taskq *, struct task *, sleepfn); > @@ -176,6 +178,30 @@ taskq_create_thread(void *arg) > } while (tq->tq_running < tq->tq_nthreads); > > mtx_leave(&tq->tq_mtx); > +} > + > +void > +taskq_barrier(struct taskq *tq) > +{ > + struct sleep_state sls; > + unsigned int notdone = 1; > + struct task t = TASK_INITIALIZER(taskq_barrier_task, ¬done); > + > + task_add(tq, &t); > + > + while (notdone) { > + sleep_setup(&sls, ¬done, PWAIT, "tqbar"); > + sleep_finish(&sls, notdone); > + } > +} > + > +void > +taskq_barrier_task(void *p) > +{ > + unsigned int *notdone = p; > + > + *notdone = 0; > + wakeup_one(notdone); > } > > void > Index: sys/net/ifq.c > =================================================================== > RCS file: /cvs/src/sys/net/ifq.c,v > retrieving revision 1.12 > diff -u -p -r1.12 ifq.c > --- sys/net/ifq.c 2 Jun 2017 00:07:12 -0000 1.12 > +++ sys/net/ifq.c 10 Nov 2017 00:45:41 -0000 > @@ -64,9 +64,16 @@ struct priq { > void ifq_start_task(void *); > void ifq_restart_task(void *); > void ifq_barrier_task(void *); > +void ifq_bundle_task(void *); > > #define TASK_ONQUEUE 0x1 > > +static inline void > +ifq_run_start(struct ifqueue *ifq) > +{ > + ifq_serialize(ifq, &ifq->ifq_start); > +} > + > void > ifq_serialize(struct ifqueue *ifq, struct task *t) > { > @@ -108,6 +115,16 @@ ifq_is_serialized(struct ifqueue *ifq) > } > > void > +ifq_start(struct ifqueue *ifq) > +{ > + if (ifq_len(ifq) >= 4) { > + task_del(ifq->ifq_softnet, &ifq->ifq_bundle); > + ifq_run_start(ifq); > + } else > + task_add(ifq->ifq_softnet, &ifq->ifq_bundle); > +} > + > +void > ifq_start_task(void *p) > { > struct ifqueue *ifq = p; > @@ -131,6 +148,14 @@ ifq_restart_task(void *p) > } > > void > +ifq_bundle_task(void *p) > +{ > + struct ifqueue *ifq = p; > + > + ifq_run_start(ifq); > +} > + > +void > ifq_barrier(struct ifqueue *ifq) > { > struct sleep_state sls; > @@ -140,6 +165,9 @@ ifq_barrier(struct ifqueue *ifq) > /* this should only be called from converted drivers */ > KASSERT(ISSET(ifq->ifq_if->if_xflags, IFXF_MPSAFE)); > > + if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) > + taskq_barrier(ifq->ifq_softnet); > + > if (ifq->ifq_serializer == NULL) > return; > > @@ -168,6 +196,7 @@ void > ifq_init(struct ifqueue *ifq, struct ifnet *ifp, unsigned int idx) > { > ifq->ifq_if = ifp; > + ifq->ifq_softnet = net_tq(ifp->if_index + idx); > ifq->ifq_softc = NULL; > > mtx_init(&ifq->ifq_mtx, IPL_NET); > @@ -189,6 +218,7 @@ ifq_init(struct ifqueue *ifq, struct ifn > mtx_init(&ifq->ifq_task_mtx, IPL_NET); > TAILQ_INIT(&ifq->ifq_task_list); > ifq->ifq_serializer = NULL; > + task_set(&ifq->ifq_bundle, ifq_bundle_task, ifq); > > task_set(&ifq->ifq_start, ifq_start_task, ifq); > task_set(&ifq->ifq_restart, ifq_restart_task, ifq); > @@ -239,6 +269,8 @@ void > ifq_destroy(struct ifqueue *ifq) > { > struct mbuf_list ml = MBUF_LIST_INITIALIZER(); > + > + ifq_barrier(ifq); /* ensure nothing is running with the ifq */ > > /* don't need to lock because this is the last use of the ifq */ > > Index: sys/net/ifq.h > =================================================================== > RCS file: /cvs/src/sys/net/ifq.h,v > retrieving revision 1.13 > diff -u -p -r1.13 ifq.h > --- sys/net/ifq.h 3 May 2017 20:55:29 -0000 1.13 > +++ sys/net/ifq.h 10 Nov 2017 00:45:41 -0000 > @@ -25,6 +25,7 @@ struct ifq_ops; > > struct ifqueue { > struct ifnet *ifq_if; > + struct taskq *ifq_softnet; > union { > void *_ifq_softc; > /* > @@ -57,6 +58,7 @@ struct ifqueue { > struct mutex ifq_task_mtx; > struct task_list ifq_task_list; > void *ifq_serializer; > + struct task ifq_bundle; > > /* work to be serialised */ > struct task ifq_start; > @@ -378,6 +380,7 @@ void ifq_init(struct ifqueue *, struct > void ifq_attach(struct ifqueue *, const struct ifq_ops *, void *); > void ifq_destroy(struct ifqueue *); > int ifq_enqueue(struct ifqueue *, struct mbuf *); > +void ifq_start(struct ifqueue *); > struct mbuf *ifq_deq_begin(struct ifqueue *); > void ifq_deq_commit(struct ifqueue *, struct mbuf *); > void ifq_deq_rollback(struct ifqueue *, struct mbuf *); > @@ -411,12 +414,6 @@ static inline unsigned int > ifq_is_oactive(struct ifqueue *ifq) > { > return (ifq->ifq_oactive); > -} > - > -static inline void > -ifq_start(struct ifqueue *ifq) > -{ > - ifq_serialize(ifq, &ifq->ifq_start); > } > > static inline void >