On 13/11/17(Mon) 10:56, David Gwynne wrote: > On Sun, Nov 12, 2017 at 02:45:05PM +0100, Martin Pieuchot wrote: > > [...] > > 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. > > /em shrug. > > i dont know how to tell what the current softnet tq is.
That's something we need to know. This will allow to have per taskq data structure. > would it > be enough to simply not mix if the ifq_idx into the argument to > net_tq? Maybe. If a CPU processing packets from myx0 in softnet0 and want to send forward via myx1, using net_tq() like you do it know will pick softnet1. My suggestion was to keep softnet1 busy with processing packets coming from myx1 and enqueue the task on softnet0. This way no inter CPU communication is needed. But without testing we can't tell if it's more efficient the other way. > > > 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. > > both hrvoje and i hit a deadlock when downing an interface. if > softnet (or softnets) are waiting on the netlock that and ioctl > path holds, then an ifq_barrier in that ioctl path will sleep forever > waiting for a task to run in softnets that are waiting for the > netlock. > > to mitigate this ive added code like what uvm has. thoughts? That's only called in ioctl(2) path, right? Which commands end up there? The problem with driver *_ioctl() routines is that pseudo-driver need the NET_LOCK() while the others should not. So I'd rather see the NET_LOCK() released before calling ifp->if_ioctl(). > 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 13 Nov 2017 00:46:17 -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 13 Nov 2017 00:46:17 -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 13 Nov 2017 00:46:17 -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 13 Nov 2017 00:46:17 -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,18 @@ 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)) { > + int netlocked = (rw_status(&netlock) == RW_WRITE); > + > + if (netlocked) > + NET_UNLOCK(); > + > + taskq_barrier(ifq->ifq_softnet); > + > + if (netlocked) > + NET_LOCK(); > + } > + > if (ifq->ifq_serializer == NULL) > return; > > @@ -168,6 +205,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); > ifq->ifq_softc = NULL; > > mtx_init(&ifq->ifq_mtx, IPL_NET); > @@ -189,6 +227,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 +278,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 13 Nov 2017 00:46:17 -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 >