From owner-svn-src-all@freebsd.org Mon Aug 24 20:23:36 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 013C53CC895; Mon, 24 Aug 2020 20:23:36 +0000 (UTC) (envelope-from melifaro@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Bb3VH6BXMz4DJd; Mon, 24 Aug 2020 20:23:35 +0000 (UTC) (envelope-from melifaro@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id B84E11DDFC; Mon, 24 Aug 2020 20:23:35 +0000 (UTC) (envelope-from melifaro@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 07OKNZo2023418; Mon, 24 Aug 2020 20:23:35 GMT (envelope-from melifaro@FreeBSD.org) Received: (from melifaro@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 07OKNYGI023412; Mon, 24 Aug 2020 20:23:34 GMT (envelope-from melifaro@FreeBSD.org) Message-Id: <202008242023.07OKNYGI023412@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: melifaro set sender to melifaro@FreeBSD.org using -f From: "Alexander V. Chernikov" Date: Mon, 24 Aug 2020 20:23:34 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r364730 - in head/sys: kern net net/route X-SVN-Group: head X-SVN-Commit-Author: melifaro X-SVN-Commit-Paths: in head/sys: kern net net/route X-SVN-Commit-Revision: 364730 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 Aug 2020 20:23:36 -0000 Author: melifaro Date: Mon Aug 24 20:23:34 2020 New Revision: 364730 URL: https://svnweb.freebsd.org/changeset/base/364730 Log: Remove RT_LOCK mutex from rte. rtentry lock traditionally served 2 purposed: first was protecting refcounts, the second was assuring consistent field access/changes. Since route nexthop introduction, the need for the former disappeared and the need for the latter reduced. To be more precise, the following rte field are mutable: rt_nhop (nexthop pointer, updated with RIB_WLOCK, passed in rib_cmd_info) rte_flags (only RTF_HOST and RTF_UP, where RTF_UP gets changed at rte removal) rt_weight (relative weight, updated with RIB_WLOCK, passed in rib_cmd_info) rt_expire (time when rte deletion is scheduled, updated with RIB_WLOCK) rt_chain (deletion chain pointer, updated with RIB_WLOCK) All of them are updated under RIB_WLOCK, so the only remaining concern is the reading. rt_nhop and rt_weight (addressed in this review) are read under rib lock and stored in the rib_cmd_info, so the caller has no problem with consitency. rte_flags is currently read unlocked in rtsock reporting (however the scope is only RTF_UP flag, which is pretty static). rt_expire is currently read unlocked in rtsock reporting. rt_chain accesses are safe, as this is only used at route deletion. rt_expire and rte_flags reads will be dealt in a separate reviews soon. Differential Revision: https://reviews.freebsd.org/D26162 Modified: head/sys/kern/subr_witness.c head/sys/net/route.c head/sys/net/route/route_ctl.c head/sys/net/route/route_var.h head/sys/net/rtsock.c Modified: head/sys/kern/subr_witness.c ============================================================================== --- head/sys/kern/subr_witness.c Mon Aug 24 20:02:36 2020 (r364729) +++ head/sys/kern/subr_witness.c Mon Aug 24 20:23:34 2020 (r364730) @@ -531,7 +531,6 @@ static struct witness_order_list_entry order_lists[] = */ { "so_rcv", &lock_class_mtx_sleep }, { "radix node head", &lock_class_rm }, - { "rtentry", &lock_class_mtx_sleep }, { "ifaddr", &lock_class_mtx_sleep }, { NULL, NULL }, /* Modified: head/sys/net/route.c ============================================================================== --- head/sys/net/route.c Mon Aug 24 20:02:36 2020 (r364729) +++ head/sys/net/route.c Mon Aug 24 20:23:34 2020 (r364730) @@ -214,18 +214,19 @@ rib_add_redirect(u_int fibnum, struct sockaddr *dst, s /* Verify the allowed flag mask. */ KASSERT(((flags & ~(RTF_GATEWAY)) == 0), ("invalid redirect flags: %x", flags)); + flags |= RTF_HOST | RTF_DYNAMIC; /* Get the best ifa for the given interface and gateway. */ if ((ifa = ifaof_ifpforaddr(gateway, ifp)) == NULL) return (ENETUNREACH); ifa_ref(ifa); - + bzero(&info, sizeof(info)); info.rti_info[RTAX_DST] = dst; info.rti_info[RTAX_GATEWAY] = gateway; info.rti_ifa = ifa; info.rti_ifp = ifp; - info.rti_flags = flags | RTF_HOST | RTF_DYNAMIC; + info.rti_flags = flags; /* Setup route metrics to define expire time. */ bzero(&rti_rmx, sizeof(rti_rmx)); @@ -242,10 +243,6 @@ rib_add_redirect(u_int fibnum, struct sockaddr *dst, s return (error); } - RT_LOCK(rc.rc_rt); - flags = rc.rc_rt->rte_flags; - RT_UNLOCK(rc.rc_rt); - RTSTAT_INC(rts_dynamic); /* Send notification of a route addition to userland. */ @@ -253,7 +250,7 @@ rib_add_redirect(u_int fibnum, struct sockaddr *dst, s info.rti_info[RTAX_DST] = dst; info.rti_info[RTAX_GATEWAY] = gateway; info.rti_info[RTAX_AUTHOR] = author; - rt_missmsg_fib(RTM_REDIRECT, &info, flags, error, fibnum); + rt_missmsg_fib(RTM_REDIRECT, &info, flags | RTF_UP, error, fibnum); return (0); } @@ -811,9 +808,7 @@ rt_mpath_unlink(struct rib_head *rnh, struct rt_addrin */ if (rn) { rto = RNTORT(rn); - RT_LOCK(rto); rto->rte_flags |= RTF_UP; - RT_UNLOCK(rto); } else if (rt->rte_flags & RTF_GATEWAY) { /* * For gateway routes, we need to Modified: head/sys/net/route/route_ctl.c ============================================================================== --- head/sys/net/route/route_ctl.c Mon Aug 24 20:02:36 2020 (r364729) +++ head/sys/net/route/route_ctl.c Mon Aug 24 20:23:34 2020 (r364730) @@ -149,9 +149,6 @@ rtfree(struct rtentry *rt) KASSERT(rt != NULL, ("%s: NULL rt", __func__)); - RT_LOCK_ASSERT(rt); - - RT_UNLOCK(rt); epoch_call(net_epoch_preempt, destroy_rtentry_epoch, &rt->rt_epoch_ctx); } @@ -250,7 +247,6 @@ add_route(struct rib_head *rnh, struct rt_addrinfo *in nhop_free(nh); return (ENOBUFS); } - RT_LOCK_INIT(rt); rt->rte_flags = RTF_UP | flags; rt->rt_nhop = nh; @@ -283,7 +279,6 @@ add_route(struct rib_head *rnh, struct rt_addrinfo *in rt_old = NULL; RIB_WLOCK(rnh); - RT_LOCK(rt); #ifdef RADIX_MPATH /* do not permit exactly the same dst/mask/gw pair */ if (rt_mpath_capable(rnh) && @@ -291,7 +286,6 @@ add_route(struct rib_head *rnh, struct rt_addrinfo *in RIB_WUNLOCK(rnh); nhop_free(nh); - RT_LOCK_DESTROY(rt); uma_zfree(V_rtzone, rt); return (EEXIST); } @@ -307,8 +301,9 @@ add_route(struct rib_head *rnh, struct rt_addrinfo *in /* Finalize notification */ rnh->rnh_gen++; - rc->rc_rt = RNTORT(rn); + rc->rc_rt = rt; rc->rc_nh_new = nh; + rc->rc_nh_weight = rt->rt_weight; rib_notify(rnh, RIB_NOTIFY_IMMEDIATE, rc); } else if ((info->rti_flags & RTF_PINNED) != 0) { @@ -333,14 +328,15 @@ add_route(struct rib_head *rnh, struct rt_addrinfo *in if (rn != NULL) { rc->rc_cmd = RTM_CHANGE; - rc->rc_rt = RNTORT(rn); + rc->rc_rt = rt; rc->rc_nh_old = rt_old->rt_nhop; rc->rc_nh_new = nh; + rc->rc_nh_weight = rt->rt_weight; } else { rc->rc_cmd = RTM_DELETE; - rc->rc_rt = RNTORT(rn); + rc->rc_rt = rt_old; rc->rc_nh_old = rt_old->rt_nhop; - rc->rc_nh_new = nh; + rc->rc_nh_weight = rt_old->rt_weight; } rib_notify(rnh, RIB_NOTIFY_IMMEDIATE, rc); } @@ -359,13 +355,10 @@ add_route(struct rib_head *rnh, struct rt_addrinfo *in */ if (rn == NULL) { nhop_free(nh); - RT_LOCK_DESTROY(rt); uma_zfree(V_rtzone, rt); return (EEXIST); } - RT_UNLOCK(rt); - return (0); } @@ -461,7 +454,6 @@ rt_unlinkrte(struct rib_head *rnh, struct rt_addrinfo panic ("rtrequest delete"); rt = RNTORT(rn); - RT_LOCK(rt); rt->rte_flags &= ~RTF_UP; *perror = 0; @@ -620,21 +612,19 @@ change_route_one(struct rib_head *rnh, struct rt_addri } /* Proceed with the update */ - RT_LOCK(rt); /* Provide notification to the protocols.*/ rt->rt_nhop = nh; rt_setmetrics(info, rt); /* Finalize notification */ + rnh->rnh_gen++; + rc->rc_rt = rt; rc->rc_nh_old = nh_orig; rc->rc_nh_new = rt->rt_nhop; + rc->rc_nh_weight = rt->rt_weight; - RT_UNLOCK(rt); - - /* Update generation id to reflect rtable change */ - rnh->rnh_gen++; rib_notify(rnh, RIB_NOTIFY_IMMEDIATE, rc); RIB_WUNLOCK(rnh); Modified: head/sys/net/route/route_var.h ============================================================================== --- head/sys/net/route/route_var.h Mon Aug 24 20:02:36 2020 (r364729) +++ head/sys/net/route/route_var.h Mon Aug 24 20:23:34 2020 (r364730) @@ -168,22 +168,9 @@ struct rtentry { int rte_flags; /* up/down?, host/net */ u_long rt_weight; /* absolute weight */ u_long rt_expire; /* lifetime for route, e.g. redirect */ -#define rt_endzero rt_mtx - struct mtx rt_mtx; /* mutex for routing entry */ struct rtentry *rt_chain; /* pointer to next rtentry to delete */ struct epoch_context rt_epoch_ctx; /* net epoch tracker */ }; - -#define RT_LOCK_INIT(_rt) \ - mtx_init(&(_rt)->rt_mtx, "rtentry", NULL, MTX_DEF | MTX_DUPOK | MTX_NEW) -#define RT_LOCK(_rt) mtx_lock(&(_rt)->rt_mtx) -#define RT_UNLOCK(_rt) mtx_unlock(&(_rt)->rt_mtx) -#define RT_LOCK_DESTROY(_rt) mtx_destroy(&(_rt)->rt_mtx) -#define RT_LOCK_ASSERT(_rt) mtx_assert(&(_rt)->rt_mtx, MA_OWNED) -#define RT_UNLOCK_COND(_rt) do { \ - if (mtx_owned(&(_rt)->rt_mtx)) \ - mtx_unlock(&(_rt)->rt_mtx); \ -} while (0) /* * With the split between the routing entry and the nexthop, Modified: head/sys/net/rtsock.c ============================================================================== --- head/sys/net/rtsock.c Mon Aug 24 20:02:36 2020 (r364729) +++ head/sys/net/rtsock.c Mon Aug 24 20:23:34 2020 (r364730) @@ -184,9 +184,9 @@ static void rt_getmetrics(const struct rtentry *rt, static void rt_dispatch(struct mbuf *, sa_family_t); static int handle_rtm_get(struct rt_addrinfo *info, u_int fibnum, struct rt_msghdr *rtm, struct rib_cmd_info *rc); -static int update_rtm_from_rte(struct rt_addrinfo *info, +static int update_rtm_from_rc(struct rt_addrinfo *info, struct rt_msghdr **prtm, int alloc_len, - struct rtentry *rt, struct nhop_object *nh); + struct rib_cmd_info *rc, struct nhop_object *nh); static void send_rtm_reply(struct socket *so, struct rt_msghdr *rtm, struct mbuf *m, sa_family_t saf, u_int fibnum, int rtm_errno); @@ -747,7 +747,7 @@ handle_rtm_get(struct rt_addrinfo *info, u_int fibnum, } /* - * Update sockaddrs, flags, etc in @prtm based on @rt data. + * Update sockaddrs, flags, etc in @prtm based on @rc data. * rtm can be reallocated. * * Returns 0 on success, along with pointer to (potentially reallocated) @@ -755,8 +755,8 @@ handle_rtm_get(struct rt_addrinfo *info, u_int fibnum, * */ static int -update_rtm_from_rte(struct rt_addrinfo *info, struct rt_msghdr **prtm, - int alloc_len, struct rtentry *rt, struct nhop_object *nh) +update_rtm_from_rc(struct rt_addrinfo *info, struct rt_msghdr **prtm, + int alloc_len, struct rib_cmd_info *rc, struct nhop_object *nh) { struct sockaddr_storage netmask_ss; struct walkarg w; @@ -767,10 +767,10 @@ update_rtm_from_rte(struct rt_addrinfo *info, struct r rtm = *prtm; - info->rti_info[RTAX_DST] = rt_key(rt); + info->rti_info[RTAX_DST] = rt_key(rc->rc_rt); info->rti_info[RTAX_GATEWAY] = &nh->gw_sa; - info->rti_info[RTAX_NETMASK] = rtsock_fix_netmask(rt_key(rt), - rt_mask(rt), &netmask_ss); + info->rti_info[RTAX_NETMASK] = rtsock_fix_netmask(rt_key(rc->rc_rt), + rt_mask(rc->rc_rt), &netmask_ss); info->rti_info[RTAX_GENMASK] = 0; ifp = nh->nh_ifp; if (rtm->rtm_addrs & (RTA_IFP | RTA_IFA)) { @@ -815,12 +815,12 @@ update_rtm_from_rte(struct rt_addrinfo *info, struct r w.w_tmemsize = alloc_len; rtsock_msg_buffer(rtm->rtm_type, info, &w, &len); - if (rt->rte_flags & RTF_GWFLAG_COMPAT) + rtm->rtm_flags = rc->rc_rt->rte_flags | nhop_get_rtflags(nh); + if (rtm->rtm_flags & RTF_GWFLAG_COMPAT) rtm->rtm_flags = RTF_GATEWAY | - (rt->rte_flags & ~RTF_GWFLAG_COMPAT); - else - rtm->rtm_flags = rt->rte_flags; - rt_getmetrics(rt, nh, &rtm->rtm_rmx); + (rtm->rtm_flags & ~RTF_GWFLAG_COMPAT); + rt_getmetrics(rc->rc_rt, nh, &rtm->rtm_rmx); + rtm->rtm_rmx.rmx_weight = rc->rc_nh_weight; rtm->rtm_addrs = info->rti_addrs; if (orig_rtm != NULL) @@ -918,8 +918,8 @@ route_output(struct mbuf *m, struct socket *so, ...) #ifdef INET6 rti_need_deembed = 1; #endif - rtm->rtm_index = rc.rc_nh_new->nh_ifp->if_index; nh = rc.rc_nh_new; + rtm->rtm_index = nh->nh_ifp->if_index; } break; @@ -946,7 +946,7 @@ report: senderr(ESRCH); } - error = update_rtm_from_rte(&info, &rtm, alloc_len, rc.rc_rt, nh); + error = update_rtm_from_rc(&info, &rtm, alloc_len, &rc, nh); /* * Note that some sockaddr pointers may have changed to * point to memory outsize @rtm. Some may be pointing