On Wed, Apr 20, 2022 at 08:12:51PM +0200, Alexander Bluhm wrote: > Hi, > > 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. > > It survived a full regress with witness. > > Hrvoje: Can you put this on your test machine together with parallel > IP forwarding? > > ok?
I would have prefer if the code was refactored first. The removal of a rt_timer is copied over in 4 or 5 places with almost no difference. In the rt_timer_queue_destroy() case you can use TAILQ_CONCAT and kill one loop. Also I would use TAILQ_HEAD_INITIALIZER() instead of TAILQ_INIT(). To be honest most of this code should be directly replaced with the timeout API. I bet the timeout API is more efficent. > Index: net/route.c > =================================================================== > RCS file: /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 20 Apr 2022 18:00:39 -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); > } > } > @@ -1471,8 +1491,9 @@ rt_timer_add(struct rtentry *rt, void (* > time_t current_time; > > current_time = getuptime(); > - rt->rt_expire = current_time + queue->rtq_timeout; > > + 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 +1502,31 @@ 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... */ > } > } > + mtx_leave(&rttimer_mtx); > > - r = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO); > - if (r == NULL) > - return (ENOBUFS); > + if (r == NULL) { > + r = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO); > + if (r == NULL) > + return (ENOBUFS); > + } else > + memset(r, 0, sizeof(*r)); > > r->rtt_rt = rt; > r->rtt_time = current_time; > r->rtt_func = func; > r->rtt_queue = queue; > r->rtt_tableid = rtableid; > + > + mtx_enter(&rttimer_mtx); > LIST_INSERT_HEAD(&rt->rt_timer, r, rtt_link); > TAILQ_INSERT_TAIL(&queue->rtq_head, r, rtt_next); > r->rtt_queue->rtq_count++; > + mtx_leave(&rttimer_mtx); > > return (0); > } > @@ -1512,23 +1537,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: /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 20 Apr 2022 18:00:39 -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