On 07/12/15(Mon) 19:37, David Gwynne wrote:
> On Sun, Dec 06, 2015 at 03:51:26PM +0100, Martin Pieuchot wrote:
> > On 06/12/15(Sun) 14:00, David Gwynne wrote:
> > > [...]
> > > the idea is you have a taskctx, which represents a serialising
> > > context for tasks. tasks are submitted to the taskctx, and the code
> > > will try to run the tasks immediately rather than defer them to a
> > > thread. if there is contention on the context, the contending cpu
> > > yields after queueing the task because the other cpu is responsible
> > > for running all pending tasks to completion.
> > >
> > > it also simplifies the barrier operations a lot.
> > >
> > > the diff below implements a generic taskctx framework, and cuts the
> > > mpsafe if_start() implementation over to it.
> >
> > I'm not sure this should be implemented as a generic framework. I'd
> > suggest to keep it specific to if_start(). As you say above the least
> > worst mechanism we have is currently taskqs, but maybe this could/will
> > change?
>
> im in two minds about where the ctx code should sit. i obviously
> lean toward keeping it with taskqs, but i also cant come up with
> another use case for it. i also havent tried very hard to think of
> one.
Even if you have another use case for it I find this generic framework
confusing. It is build on top of taskq, but it's not part of the taskq
API. So putting the documentation in the same manual just confuse me.
Your task API is simple and people use it easily. I think it should
stay like that.
Now we need a way to serialize start and txeof and you came up with a
solution...
> are you asking if taskqs will get better, or if something better
> than taskqs will come along?
...I'm just saying that the "implementation" of your serialisation
mechanism should not matter. I'm fine with having a context for tasks
but I'd suggest that this should be abstracted in a simple API instead
of offering a framework to build upon.
Could you make it such that you don't need a task in myx_softc?
> > I cannot understand what's happening by reading the myx(4) chunk itself.
> > So I believe the interface needs to be polished. Would it be possible
> > to implement this serialization without introducing if_restart()?
>
> that is by far my least favourite bit of this diff. in my defense,
> i wrote it while my mother was trying to have a conversation with
> me, so it didn't get my full attention. ive torn if_restart out and
> implemented it completely in myx below.
>
> > > myx is also changed to only clr oactive from within the taskctx
> > > serialiser, thereby avoiding the race, but keeps the bulk of txeof
> > > outside the serialiser so it can run concurrently with start.
> > >
> > > other nics are free to serialise start and txeof within the
> > > ifq_serializer if they want, or not, it is up to them.
> > >
> > > thoughts? tests? opinions on messy .h files?
> >
> > It appears to me that you have unrelated changes: if_enter/if_leave.
>
> oops, yes. i suck at juggling diffs. maybe we should call if.c
> full and split it up ;)
I think we should start with that because the I'm afraid we're already
cooking spaghetti.
My question is the barrier and mpsafe code is related to a queue or to
an interface? In other words does it make sense to serialize on a
context in the sending queue? What about multiple queues?
> Index: sys/net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.422
> diff -u -p -r1.422 if.c
> --- sys/net/if.c 5 Dec 2015 10:07:55 -0000 1.422
> +++ sys/net/if.c 7 Dec 2015 06:30:42 -0000
> @@ -153,8 +153,9 @@ void if_input_process(void *);
> void ifa_print_all(void);
> #endif
>
> -void if_start_mpsafe(struct ifnet *ifp);
> -void if_start_locked(struct ifnet *ifp);
> +void if_start_mpsafe(struct ifnet *);
> +void if_start_locked(struct ifnet *);
> +void if_start_task(void *);
>
> /*
> * interface index map
> @@ -513,6 +514,7 @@ if_attach_common(struct ifnet *ifp)
> TAILQ_INIT(&ifp->if_maddrlist);
>
> ifq_init(&ifp->if_snd);
> + task_set(&ifp->if_start_ctx, if_start_task, ifp);
>
> ifp->if_addrhooks = malloc(sizeof(*ifp->if_addrhooks),
> M_TEMP, M_WAITOK);
> @@ -557,66 +559,29 @@ if_start_locked(struct ifnet *ifp)
> KERNEL_UNLOCK();
> }
>
> -static inline unsigned int
> -ifq_enter(struct ifqueue *ifq)
> -{
> - return (atomic_inc_int_nv(&ifq->ifq_serializer) == 1);
> -}
> -
> -static inline unsigned int
> -ifq_leave(struct ifqueue *ifq)
> +void
> +if_start_mpsafe(struct ifnet *ifp)
> {
> - if (atomic_cas_uint(&ifq->ifq_serializer, 1, 0) == 1)
> - return (1);
> -
> - ifq->ifq_serializer = 1;
> -
> - return (0);
> + task_dispatch(&ifp->if_snd.ifq_serializer, &ifp->if_start_ctx);
> }
>
> void
> -if_start_mpsafe(struct ifnet *ifp)
> +if_start_task(void *p)
> {
> - struct ifqueue *ifq = &ifp->if_snd;
> + struct ifnet *ifp = p;
>
> - if (!ifq_enter(ifq))
> + if (!ISSET(ifp->if_flags, IFF_RUNNING) ||
> + ifq_empty(&ifp->if_snd) ||
> + ifq_is_oactive(&ifp->if_snd))
> return;
>
> - do {
> - if (__predict_false(!ISSET(ifp->if_flags, IFF_RUNNING))) {
> - ifq->ifq_serializer = 0;
> - wakeup_one(&ifq->ifq_serializer);
> - return;
> - }
> -
> - if (ifq_empty(ifq) || ifq_is_oactive(ifq))
> - continue;
> -
> - ifp->if_start(ifp);
> -
> - } while (!ifq_leave(ifq));
> + ifp->if_start(ifp);
> }
>
> void
> if_start_barrier(struct ifnet *ifp)
> {
> - struct sleep_state sls;
> - struct ifqueue *ifq = &ifp->if_snd;
> -
> - /* this should only be called from converted drivers */
> - KASSERT(ISSET(ifp->if_xflags, IFXF_MPSAFE));
> -
> - /* drivers should only call this on the way down */
> - KASSERT(!ISSET(ifp->if_flags, IFF_RUNNING));
> -
> - if (ifq->ifq_serializer == 0)
> - return;
> -
> - if_start_mpsafe(ifp); /* spin the wheel to guarantee a wakeup */
> - do {
> - sleep_setup(&sls, &ifq->ifq_serializer, PWAIT, "ifbar");
> - sleep_finish(&sls, ifq->ifq_serializer != 0);
> - } while (ifq->ifq_serializer != 0);
> + taskctx_barrier(&ifp->if_snd.ifq_serializer);
> }
>
> int
> @@ -2992,6 +2957,7 @@ ifq_purge(struct ifqueue *ifq)
> void
> ifq_init(struct ifqueue *ifq)
> {
> + taskctx_init(&ifq->ifq_serializer, IPL_NET);
> mtx_init(&ifq->ifq_mtx, IPL_NET);
> ifq->ifq_drops = 0;
>
> @@ -2999,7 +2965,6 @@ ifq_init(struct ifqueue *ifq)
> ifq->ifq_ops = &priq_ops;
> ifq->ifq_q = priq_ops.ifqop_alloc(NULL);
>
> - ifq->ifq_serializer = 0;
> ifq->ifq_len = 0;
>
> if (ifq->ifq_maxlen == 0)
> Index: sys/net/if_var.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if_var.h,v
> retrieving revision 1.64
> diff -u -p -r1.64 if_var.h
> --- sys/net/if_var.h 5 Dec 2015 16:24:59 -0000 1.64
> +++ sys/net/if_var.h 7 Dec 2015 06:30:42 -0000
> @@ -42,6 +42,7 @@
> #include <sys/mbuf.h>
> #include <sys/srp.h>
> #include <sys/refcnt.h>
> +#include <sys/task.h>
> #include <sys/time.h>
>
> /*
> @@ -73,7 +74,6 @@ struct rtentry;
> struct timeout;
> struct arpcom;
> struct ifnet;
> -struct task;
>
> /*
> * Structure describing a `cloning' interface.
> @@ -108,12 +108,13 @@ struct ifq_ops {
> };
>
> struct ifqueue {
> + struct taskctx ifq_serializer;
> +
> struct mutex ifq_mtx;
> uint64_t ifq_drops;
> const struct ifq_ops *ifq_ops;
> void *ifq_q;
> unsigned int ifq_len;
> - unsigned int ifq_serializer;
> unsigned int ifq_oactive;
>
> unsigned int ifq_maxlen;
> @@ -173,6 +174,7 @@ struct ifnet { /* and the
> entries */
> int (*if_ll_output)(struct ifnet *, struct mbuf *,
> struct sockaddr *, struct rtentry *);
> /* initiate output routine */
> + struct task if_start_ctx;
> void (*if_start)(struct ifnet *);
> /* ioctl routine */
> int (*if_ioctl)(struct ifnet *, u_long, caddr_t);
> @@ -299,6 +301,12 @@ static inline unsigned int
> ifq_is_oactive(struct ifqueue *ifq)
> {
> return (ifq->ifq_oactive);
> +}
> +
> +static inline void
> +ifq_serialize(struct ifqueue *ifq, struct task *t)
> +{
> + task_dispatch(&ifq->ifq_serializer, t);
> }
>
> extern const struct ifq_ops * const ifq_priq_ops;
> Index: sys/sys/task.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/task.h,v
> retrieving revision 1.8
> diff -u -p -r1.8 task.h
> --- sys/sys/task.h 9 Feb 2015 03:15:41 -0000 1.8
> +++ sys/sys/task.h 7 Dec 2015 06:30:42 -0000
> @@ -20,6 +20,7 @@
> #define _SYS_TASKQ_H_
>
> #include <sys/queue.h>
> +#include <sys/mutex.h>
>
> struct taskq;
>
> @@ -30,6 +31,12 @@ struct task {
> unsigned int t_flags;
> };
>
> +struct taskctx {
> + struct mutex tc_mtx;
> + TAILQ_HEAD(, task) tc_worklist;
> + unsigned int tc_serializer;
> +};
> +
> #define TASKQ_MPSAFE (1 << 0)
> #define TASKQ_CANTSLEEP (1 << 1)
>
> @@ -39,6 +46,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 *);
> @@ -46,6 +54,14 @@ int task_del(struct taskq *, struct ta
>
> #define TASK_INITIALIZER(_f, _a) \
> { { NULL, NULL }, (_f), (_a), 0 }
> +
> +void taskctx_init(struct taskctx *, int);
> +void taskctx_barrier(struct taskctx *);
> +
> +void task_dispatch(struct taskctx *, struct task *);
> +
> +#define TASKCTX_INITIALIZER(_tc, _i) \
> + { MUTEX_INITIALIZER(_i), TAILQ_HEAD_INITIALIZER(_tc.tc_worklist), 0 }
>
> #endif /* _KERNEL */
>
>
