Re: struct refcnt for routes
On Mon, Jun 27, 2022 at 03:21:56PM +0200, Alexander Bluhm wrote: > Hi, > > Use ref count API for routes. > > ok? > Some time ago, I posted the diff which removes the logic around negative `refcnt' within rtfree(). This diff was stopped by mpi@, he said we could underflow rt->rt_refcnt and this logic is required to prevent panics. I don't know, is it true, but I didn't find any report about "rtfree: ... not freed (neg refs)" in the dmesg(8) output. I think the logic around negative `refcnt' is much worse then panic, because we trying hide the problem instead of catch and fix it. ok mvs@ unless someone else denies this diff. > bluhm > > Index: net/route.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v > retrieving revision 1.410 > diff -u -p -r1.410 route.c > --- net/route.c 5 May 2022 13:57:40 - 1.410 > +++ net/route.c 27 Jun 2022 13:03:16 - > @@ -488,40 +488,34 @@ rt_putgwroute(struct rtentry *rt) > void > rtref(struct rtentry *rt) > { > - atomic_inc_int(&rt->rt_refcnt); > + refcnt_take(&rt->rt_refcnt); > } > > void > rtfree(struct rtentry *rt) > { > - int refcnt; > - > if (rt == NULL) > return; > > - refcnt = (int)atomic_dec_int_nv(&rt->rt_refcnt); > - if (refcnt <= 0) { > - KASSERT(!ISSET(rt->rt_flags, RTF_UP)); > - KASSERT(!RT_ROOT(rt)); > - atomic_dec_int(&rttrash); > - if (refcnt < 0) { > - printf("rtfree: %p not freed (neg refs)\n", rt); > - return; > - } > + if (refcnt_rele(&rt->rt_refcnt) == 0) > + return; > > - KERNEL_LOCK(); > - rt_timer_remove_all(rt); > - ifafree(rt->rt_ifa); > - rtlabel_unref(rt->rt_labelid); > + KASSERT(!ISSET(rt->rt_flags, RTF_UP)); > + KASSERT(!RT_ROOT(rt)); > + atomic_dec_int(&rttrash); > + > + KERNEL_LOCK(); > + rt_timer_remove_all(rt); > + ifafree(rt->rt_ifa); > + rtlabel_unref(rt->rt_labelid); > #ifdef MPLS > - rt_mpls_clear(rt); > + rt_mpls_clear(rt); > #endif > - free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len)); > - free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len); > - KERNEL_UNLOCK(); > + free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len)); > + free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len); > + KERNEL_UNLOCK(); > > - pool_put(&rtentry_pool, rt); > - } > + pool_put(&rtentry_pool, rt); > } > > void > @@ -877,7 +871,7 @@ rtrequest(int req, struct rt_addrinfo *i > return (ENOBUFS); > } > > - rt->rt_refcnt = 1; > + refcnt_init(&rt->rt_refcnt); > rt->rt_flags = info->rti_flags | RTF_UP; > rt->rt_priority = prio; /* init routing priority */ > LIST_INIT(&rt->rt_timer); > @@ -1864,8 +1858,8 @@ db_show_rtentry(struct rtentry *rt, void > { > db_printf("rtentry=%p", rt); > > - db_printf(" flags=0x%x refcnt=%d use=%llu expire=%lld rtableid=%u\n", > - rt->rt_flags, rt->rt_refcnt, rt->rt_use, rt->rt_expire, id); > + db_printf(" flags=0x%x refcnt=%u use=%llu expire=%lld rtableid=%u\n", > + rt->rt_flags, rt->rt_refcnt.r_refs, rt->rt_use, rt->rt_expire, id); > > db_printf(" key="); db_print_sa(rt_key(rt)); > db_printf(" plen=%d", rt_plen(rt)); > Index: net/route.h > === > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v > retrieving revision 1.194 > diff -u -p -r1.194 route.h > --- net/route.h 5 May 2022 13:57:40 - 1.194 > +++ net/route.h 27 Jun 2022 13:03:16 - > @@ -119,7 +119,7 @@ struct rtentry { > struct rt_kmetrics rt_rmx; /* metrics used by rx'ing protocols */ > unsigned int rt_ifidx; /* the answer: interface to use */ > unsigned int rt_flags; /* up/down?, host/net */ > - int rt_refcnt; /* # held references */ > + struct refcntrt_refcnt; /* # held references */ > int rt_plen; /* prefix length */ > uint16_t rt_labelid;/* route label ID */ > uint8_t rt_priority; /* routing priority to use */ > Index: net/rtable.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtable.c,v > retrieving revision 1.77 > diff -u -p -r1.77 rtable.c > --- net/rtable.c 19 Apr 2022 15:44:56 - 1.77 > +++ net/rtable.c 27 Jun 2022 13:03:16 - > @@ -680,7 +680,7 @@ rtable_delete(unsigned int rtableid, str > npaths++; > > if (npaths > 1) { > - KASSERT(rt->rt_refcnt >= 1); > + KASSERT(refcnt_read(&rt->rt_refcnt) >= 1)
struct refcnt for routes
Hi, Use ref count API for routes. ok? bluhm Index: net/route.c === RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v retrieving revision 1.410 diff -u -p -r1.410 route.c --- net/route.c 5 May 2022 13:57:40 - 1.410 +++ net/route.c 27 Jun 2022 13:03:16 - @@ -488,40 +488,34 @@ rt_putgwroute(struct rtentry *rt) void rtref(struct rtentry *rt) { - atomic_inc_int(&rt->rt_refcnt); + refcnt_take(&rt->rt_refcnt); } void rtfree(struct rtentry *rt) { - int refcnt; - if (rt == NULL) return; - refcnt = (int)atomic_dec_int_nv(&rt->rt_refcnt); - if (refcnt <= 0) { - KASSERT(!ISSET(rt->rt_flags, RTF_UP)); - KASSERT(!RT_ROOT(rt)); - atomic_dec_int(&rttrash); - if (refcnt < 0) { - printf("rtfree: %p not freed (neg refs)\n", rt); - return; - } + if (refcnt_rele(&rt->rt_refcnt) == 0) + return; - KERNEL_LOCK(); - rt_timer_remove_all(rt); - ifafree(rt->rt_ifa); - rtlabel_unref(rt->rt_labelid); + KASSERT(!ISSET(rt->rt_flags, RTF_UP)); + KASSERT(!RT_ROOT(rt)); + atomic_dec_int(&rttrash); + + KERNEL_LOCK(); + rt_timer_remove_all(rt); + ifafree(rt->rt_ifa); + rtlabel_unref(rt->rt_labelid); #ifdef MPLS - rt_mpls_clear(rt); + rt_mpls_clear(rt); #endif - free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len)); - free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len); - KERNEL_UNLOCK(); + free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len)); + free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len); + KERNEL_UNLOCK(); - pool_put(&rtentry_pool, rt); - } + pool_put(&rtentry_pool, rt); } void @@ -877,7 +871,7 @@ rtrequest(int req, struct rt_addrinfo *i return (ENOBUFS); } - rt->rt_refcnt = 1; + refcnt_init(&rt->rt_refcnt); rt->rt_flags = info->rti_flags | RTF_UP; rt->rt_priority = prio; /* init routing priority */ LIST_INIT(&rt->rt_timer); @@ -1864,8 +1858,8 @@ db_show_rtentry(struct rtentry *rt, void { db_printf("rtentry=%p", rt); - db_printf(" flags=0x%x refcnt=%d use=%llu expire=%lld rtableid=%u\n", - rt->rt_flags, rt->rt_refcnt, rt->rt_use, rt->rt_expire, id); + db_printf(" flags=0x%x refcnt=%u use=%llu expire=%lld rtableid=%u\n", + rt->rt_flags, rt->rt_refcnt.r_refs, rt->rt_use, rt->rt_expire, id); db_printf(" key="); db_print_sa(rt_key(rt)); db_printf(" plen=%d", rt_plen(rt)); Index: net/route.h === RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v retrieving revision 1.194 diff -u -p -r1.194 route.h --- net/route.h 5 May 2022 13:57:40 - 1.194 +++ net/route.h 27 Jun 2022 13:03:16 - @@ -119,7 +119,7 @@ struct rtentry { struct rt_kmetrics rt_rmx; /* metrics used by rx'ing protocols */ unsigned int rt_ifidx; /* the answer: interface to use */ unsigned int rt_flags; /* up/down?, host/net */ - int rt_refcnt; /* # held references */ + struct refcntrt_refcnt; /* # held references */ int rt_plen; /* prefix length */ uint16_t rt_labelid;/* route label ID */ uint8_t rt_priority; /* routing priority to use */ Index: net/rtable.c === RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtable.c,v retrieving revision 1.77 diff -u -p -r1.77 rtable.c --- net/rtable.c19 Apr 2022 15:44:56 - 1.77 +++ net/rtable.c27 Jun 2022 13:03:16 - @@ -680,7 +680,7 @@ rtable_delete(unsigned int rtableid, str npaths++; if (npaths > 1) { - KASSERT(rt->rt_refcnt >= 1); + KASSERT(refcnt_read(&rt->rt_refcnt) >= 1); SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist, rt, rtentry, rt_next); @@ -694,7 +694,7 @@ rtable_delete(unsigned int rtableid, str if (art_delete(ar, an, addr, plen) == NULL) panic("art_delete failed to find node %p", an); - KASSERT(rt->rt_refcnt >= 1); + KASSERT(refcnt_read(&rt->rt_refcnt) >= 1); SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist, rt, rtentry, rt_next); art_put(an); Index: net/rtsock.c === RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v retrieving revision 1.330 diff -u -p -r1.330 rtsock.c --- net/rtsock.c26 Jun 2022 16:07:00 -000