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_refcnt);
> + refcnt_take(>rt_refcnt);
> }
>
> void
> rtfree(struct rtentry *rt)
> {
> - int refcnt;
> -
> if (rt == NULL)
> return;
>
> - refcnt = (int)atomic_dec_int_nv(>rt_refcnt);
> - if (refcnt <= 0) {
> - KASSERT(!ISSET(rt->rt_flags, RTF_UP));
> - KASSERT(!RT_ROOT(rt));
> - atomic_dec_int();
> - if (refcnt < 0) {
> - printf("rtfree: %p not freed (neg refs)\n", rt);
> - return;
> - }
> + if (refcnt_rele(>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();
> +
> + 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(_pool, rt);
> - }
> + pool_put(_pool, rt);
> }
>
> void
> @@ -877,7 +871,7 @@ rtrequest(int req, struct rt_addrinfo *i
> return (ENOBUFS);
> }
>
> - rt->rt_refcnt = 1;
> + refcnt_init(>rt_refcnt);
> rt->rt_flags = info->rti_flags | RTF_UP;
> rt->rt_priority = prio; /* init routing priority */
> LIST_INIT(>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_refcnt) >= 1);
> SRPL_REMOVE_LOCKED(_rc, >an_rtlist, rt,
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_refcnt);
+ refcnt_take(>rt_refcnt);
}
void
rtfree(struct rtentry *rt)
{
- int refcnt;
-
if (rt == NULL)
return;
- refcnt = (int)atomic_dec_int_nv(>rt_refcnt);
- if (refcnt <= 0) {
- KASSERT(!ISSET(rt->rt_flags, RTF_UP));
- KASSERT(!RT_ROOT(rt));
- atomic_dec_int();
- if (refcnt < 0) {
- printf("rtfree: %p not freed (neg refs)\n", rt);
- return;
- }
+ if (refcnt_rele(>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();
+
+ 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(_pool, rt);
- }
+ pool_put(_pool, rt);
}
void
@@ -877,7 +871,7 @@ rtrequest(int req, struct rt_addrinfo *i
return (ENOBUFS);
}
- rt->rt_refcnt = 1;
+ refcnt_init(>rt_refcnt);
rt->rt_flags = info->rti_flags | RTF_UP;
rt->rt_priority = prio; /* init routing priority */
LIST_INIT(>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_refcnt) >= 1);
SRPL_REMOVE_LOCKED(_rc, >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_refcnt) >= 1);
SRPL_REMOVE_LOCKED(_rc, >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 - 1.330
+++ net/rtsock.c27 Jun 2022 13:03:16 -
@@ -1992,7
