On Thu, Apr 28, 2022 at 07:24:22PM +0200, Alexander Bluhm wrote:
> I still need an ok for this diff. It is the final step before we
> can run IP forwaring in parallel.
Fine with me. If it holds you back put it in OK claudio@
I will rip the rttimer code appart in the next days and make that API a
lot simpler.
> bluhm
>
> On Thu, Apr 21, 2022 at 05:44:17PM +0200, Alexander Bluhm wrote:
> > On Wed, Apr 20, 2022 at 08:12:51PM +0200, Alexander Bluhm wrote:
> > > mvs@ reminded me of a crash I have seen in December. Route timers
> > > are not MP safe, but I think this can be fixed with a mutex. The
> > > idea is to protect the global lists with a mutex and move the rttimer
> > > into a temporary list. Then the callback and pool put can be called
> > > later without mutex.
> >
> > I have a tiny update to the diff.
> >
> > - Global locks are documented with capital letter, so use [T].
> >
> > - rt_timer_add() grabbed the mutex twice, first remove then add.
> > Better exchange in one critical section. pool_get before and
> > pool_put after.
> >
> > ok?
> >
> > Index: net/route.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> > retrieving revision 1.406
> > diff -u -p -r1.406 route.c
> > --- net/route.c 20 Apr 2022 17:58:22 -0000 1.406
> > +++ net/route.c 21 Apr 2022 13:31:52 -0000
> > @@ -1361,7 +1361,8 @@ rt_ifa_purge_walker(struct rtentry *rt,
> > * for multiple queues for efficiency's sake...
> > */
> >
> > -LIST_HEAD(, rttimer_queue) rttimer_queue_head;
> > +struct mutex rttimer_mtx;
> > +LIST_HEAD(, rttimer_queue) rttimer_queue_head; /* [T] */
> >
> > #define RTTIMER_CALLOUT(r) { \
> > if (r->rtt_func != NULL) { \
> > @@ -1393,6 +1394,7 @@ rt_timer_init(void)
> > pool_init(&rttimer_queue_pool, sizeof(struct rttimer_queue), 0,
> > IPL_MPFLOOR, 0, "rttmrq", NULL);
> >
> > + mtx_init(&rttimer_mtx, IPL_MPFLOOR);
> > LIST_INIT(&rttimer_queue_head);
> > timeout_set_proc(&rt_timer_timeout, rt_timer_timer, &rt_timer_timeout);
> > timeout_add_sec(&rt_timer_timeout, 1);
> > @@ -1408,7 +1410,10 @@ rt_timer_queue_create(int timeout)
> > rtq->rtq_timeout = timeout;
> > rtq->rtq_count = 0;
> > TAILQ_INIT(&rtq->rtq_head);
> > +
> > + mtx_enter(&rttimer_mtx);
> > LIST_INSERT_HEAD(&rttimer_queue_head, rtq, rtq_link);
> > + mtx_leave(&rttimer_mtx);
> >
> > return (rtq);
> > }
> > @@ -1416,28 +1421,36 @@ rt_timer_queue_create(int timeout)
> > void
> > rt_timer_queue_change(struct rttimer_queue *rtq, int timeout)
> > {
> > + mtx_enter(&rttimer_mtx);
> > rtq->rtq_timeout = timeout;
> > + mtx_leave(&rttimer_mtx);
> > }
> >
> > void
> > rt_timer_queue_destroy(struct rttimer_queue *rtq)
> > {
> > - struct rttimer *r;
> > + struct rttimer *r;
> > + TAILQ_HEAD(, rttimer) rttlist;
> >
> > NET_ASSERT_LOCKED();
> >
> > + TAILQ_INIT(&rttlist);
> > + mtx_enter(&rttimer_mtx);
> > while ((r = TAILQ_FIRST(&rtq->rtq_head)) != NULL) {
> > LIST_REMOVE(r, rtt_link);
> > TAILQ_REMOVE(&rtq->rtq_head, r, rtt_next);
> > + TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
> > + KASSERT(rtq->rtq_count > 0);
> > + rtq->rtq_count--;
> > + }
> > + LIST_REMOVE(rtq, rtq_link);
> > + mtx_leave(&rttimer_mtx);
> > +
> > + while ((r = TAILQ_FIRST(&rttlist)) != NULL) {
> > + TAILQ_REMOVE(&rttlist, r, rtt_next);
> > RTTIMER_CALLOUT(r);
> > pool_put(&rttimer_pool, r);
> > - if (rtq->rtq_count > 0)
> > - rtq->rtq_count--;
> > - else
> > - printf("rt_timer_queue_destroy: rtq_count reached 0\n");
> > }
> > -
> > - LIST_REMOVE(rtq, rtq_link);
> > pool_put(&rttimer_queue_pool, rtq);
> > }
> >
> > @@ -1450,15 +1463,22 @@ rt_timer_queue_count(struct rttimer_queu
> > void
> > rt_timer_remove_all(struct rtentry *rt)
> > {
> > - struct rttimer *r;
> > + struct rttimer *r;
> > + TAILQ_HEAD(, rttimer) rttlist;
> >
> > + TAILQ_INIT(&rttlist);
> > + mtx_enter(&rttimer_mtx);
> > while ((r = LIST_FIRST(&rt->rt_timer)) != NULL) {
> > LIST_REMOVE(r, rtt_link);
> > TAILQ_REMOVE(&r->rtt_queue->rtq_head, r, rtt_next);
> > - if (r->rtt_queue->rtq_count > 0)
> > - r->rtt_queue->rtq_count--;
> > - else
> > - printf("rt_timer_remove_all: rtq_count reached 0\n");
> > + TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
> > + KASSERT(r->rtt_queue->rtq_count > 0);
> > + r->rtt_queue->rtq_count--;
> > + }
> > + mtx_leave(&rttimer_mtx);
> > +
> > + while ((r = TAILQ_FIRST(&rttlist)) != NULL) {
> > + TAILQ_REMOVE(&rttlist, r, rtt_next);
> > pool_put(&rttimer_pool, r);
> > }
> > }
> > @@ -1467,12 +1487,23 @@ int
> > rt_timer_add(struct rtentry *rt, void (*func)(struct rtentry *,
> > struct rttimer *), struct rttimer_queue *queue, u_int rtableid)
> > {
> > - struct rttimer *r;
> > + struct rttimer *r, *rnew;
> > time_t current_time;
> >
> > + rnew = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO);
> > + if (rnew == NULL)
> > + return (ENOBUFS);
> > +
> > current_time = getuptime();
> > - rt->rt_expire = current_time + queue->rtq_timeout;
> >
> > + rnew->rtt_rt = rt;
> > + rnew->rtt_time = current_time;
> > + rnew->rtt_func = func;
> > + rnew->rtt_queue = queue;
> > + rnew->rtt_tableid = rtableid;
> > +
> > + mtx_enter(&rttimer_mtx);
> > + rt->rt_expire = current_time + queue->rtq_timeout;
> > /*
> > * If there's already a timer with this action, destroy it before
> > * we add a new one.
> > @@ -1481,27 +1512,19 @@ rt_timer_add(struct rtentry *rt, void (*
> > if (r->rtt_func == func) {
> > LIST_REMOVE(r, rtt_link);
> > TAILQ_REMOVE(&r->rtt_queue->rtq_head, r, rtt_next);
> > - if (r->rtt_queue->rtq_count > 0)
> > - r->rtt_queue->rtq_count--;
> > - else
> > - printf("rt_timer_add: rtq_count reached 0\n");
> > - pool_put(&rttimer_pool, r);
> > + KASSERT(r->rtt_queue->rtq_count > 0);
> > + r->rtt_queue->rtq_count--;
> > break; /* only one per list, so we can quit... */
> > }
> > }
> >
> > - r = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO);
> > - if (r == NULL)
> > - return (ENOBUFS);
> > + LIST_INSERT_HEAD(&rt->rt_timer, rnew, rtt_link);
> > + TAILQ_INSERT_TAIL(&queue->rtq_head, rnew, rtt_next);
> > + rnew->rtt_queue->rtq_count++;
> > + mtx_leave(&rttimer_mtx);
> >
> > - r->rtt_rt = rt;
> > - r->rtt_time = current_time;
> > - r->rtt_func = func;
> > - r->rtt_queue = queue;
> > - r->rtt_tableid = rtableid;
> > - LIST_INSERT_HEAD(&rt->rt_timer, r, rtt_link);
> > - TAILQ_INSERT_TAIL(&queue->rtq_head, r, rtt_next);
> > - r->rtt_queue->rtq_count++;
> > + if (r != NULL)
> > + pool_put(&rttimer_pool, r);
> >
> > return (0);
> > }
> > @@ -1512,23 +1535,30 @@ rt_timer_timer(void *arg)
> > struct timeout *to = (struct timeout *)arg;
> > struct rttimer_queue *rtq;
> > struct rttimer *r;
> > + TAILQ_HEAD(, rttimer) rttlist;
> > time_t current_time;
> >
> > current_time = getuptime();
> > + TAILQ_INIT(&rttlist);
> >
> > NET_LOCK();
> > + mtx_enter(&rttimer_mtx);
> > LIST_FOREACH(rtq, &rttimer_queue_head, rtq_link) {
> > while ((r = TAILQ_FIRST(&rtq->rtq_head)) != NULL &&
> > (r->rtt_time + rtq->rtq_timeout) < current_time) {
> > LIST_REMOVE(r, rtt_link);
> > TAILQ_REMOVE(&rtq->rtq_head, r, rtt_next);
> > - RTTIMER_CALLOUT(r);
> > - pool_put(&rttimer_pool, r);
> > - if (rtq->rtq_count > 0)
> > - rtq->rtq_count--;
> > - else
> > - printf("rt_timer_timer: rtq_count reached 0\n");
> > + TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
> > + KASSERT(rtq->rtq_count > 0);
> > + rtq->rtq_count--;
> > }
> > + }
> > + mtx_leave(&rttimer_mtx);
> > +
> > + while ((r = TAILQ_FIRST(&rttlist)) != NULL) {
> > + TAILQ_REMOVE(&rttlist, r, rtt_next);
> > + RTTIMER_CALLOUT(r);
> > + pool_put(&rttimer_pool, r);
> > }
> > NET_UNLOCK();
> >
> > Index: net/route.h
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
> > retrieving revision 1.190
> > diff -u -p -r1.190 route.h
> > --- net/route.h 20 Apr 2022 17:58:22 -0000 1.190
> > +++ net/route.h 21 Apr 2022 13:26:18 -0000
> > @@ -36,6 +36,12 @@
> > #define _NET_ROUTE_H_
> >
> > /*
> > + * Locks used to protect struct members in this file:
> > + * I immutable after creation
> > + * T rttimer_mtx route timer lists
> > + */
> > +
> > +/*
> > * Kernel resident routing tables.
> > *
> > * The routing tables are initialized when interface addresses
> > @@ -400,21 +406,21 @@ rtstat_inc(enum rtstat_counters c)
> > * These allow functions to be called for routes at specific times.
> > */
> > struct rttimer {
> > - TAILQ_ENTRY(rttimer) rtt_next; /* entry on timer queue */
> > - LIST_ENTRY(rttimer) rtt_link; /* multiple timers per rtentry */
> > - struct rttimer_queue *rtt_queue;/* back pointer to queue */
> > - struct rtentry *rtt_rt; /* Back pointer to the route */
> > - void (*rtt_func)(struct rtentry *,
> > - struct rttimer *);
> > - time_t rtt_time; /* When this timer was registered */
> > - u_int rtt_tableid; /* routing table id of rtt_rt */
> > + TAILQ_ENTRY(rttimer) rtt_next; /* [T] entry on timer queue */
> > + LIST_ENTRY(rttimer) rtt_link; /* [T] timers per rtentry */
> > + struct rttimer_queue *rtt_queue; /* [T] back pointer to queue */
> > + struct rtentry *rtt_rt; /* [I] back pointer to route */
> > + void (*rtt_func) /* [I] callback */
> > + (struct rtentry *, struct rttimer *);
> > + time_t rtt_time; /* [I] when timer registered */
> > + u_int rtt_tableid; /* [I] rtable id of rtt_rt */
> > };
> >
> > struct rttimer_queue {
> > - TAILQ_HEAD(, rttimer) rtq_head;
> > - LIST_ENTRY(rttimer_queue) rtq_link;
> > - unsigned long rtq_count;
> > - int rtq_timeout;
> > + TAILQ_HEAD(, rttimer) rtq_head; /* [T] */
> > + LIST_ENTRY(rttimer_queue) rtq_link; /* [T] */
> > + unsigned long rtq_count; /* [T] */
> > + int rtq_timeout; /* [T] */
> > };
> >
> > const char *rtlabel_id2name(u_int16_t);
>
--
:wq Claudio
