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
