Download raw body.
make route rt_locks atomic
On Mon, Jan 20, 2025 at 02:09:03PM +0100, Claudio Jeker wrote:
> On Tue, Jan 14, 2025 at 12:16:59AM +0100, Alexander Bluhm wrote:
> > Hi,
> >
> > rt_locks is accessed lockless. Setting the bits has to be done
> > atomically. Loads should be marked as atomic to avoid wrong compiler
> > optimizations.
> >
> > The correlation between rt_locks and rt_mtu is very weak. When
> > RTV_MTU is set, stop modifying rt_mtu. But is is not really necessary
> > to stop updating rt_mtu immediately when another thread modifies
> > rt_locks. The events in the network stack changing those values
> > are not synchronized anyway. So I came to the conclusion that
> > memory barriers are not necessary.
> >
> > In route message output, I have left setting the flags in exclusive
> > net lock as they are modified together with other route attributes.
> >
> > ok?
>
> I'm probably late to the party but again, how can rt_locks be atomic when
> the value rt_locks protects is changed not in an atomic transaction with
> the rt_locks value?
> This is not truly atomic and while what you do is probably OK it is not
> correct.
Changes of rt_locks are atomic with this diff. Old code is lucky
without atomic_setbits_int() as RTV_MTU is the only flag used in
rt_locks.
Another thing is how rt_locks and rt_mtu are related. Do they need
a common mutex? I say no. Do the need memmory barries? I also
say no, as rt_mtu is only valid if RTV_MTU is clean. If one CPU
misses the RTV_MTU update, there is no rt_mtu update that must be
handled. And if it misses the rt_mtu update, it does not matter
as RTV_MTU is set.
The only problematic path may be clearing RTV_MTU. But I left this
code in exclusive net lock.
Note that the diff makes things only better. If path MTU is really
not correct, we have to backout parallel softnet. Or provide a
diff which fixes it. I cannot see a bug.
The reason I want the volatile around rt_locks is to avoid compiler
misoptmisations. Nothing can go wrong by CPU reordering in this
case.
bluhm
> > Index: net/route.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> > diff -u -p -r1.438 route.c
> > --- net/route.c 3 Jan 2025 21:27:40 -0000 1.438
> > +++ net/route.c 13 Jan 2025 22:50:54 -0000
> > @@ -557,7 +557,7 @@ rt_setgwroute(struct rtentry *rt, const
> > * If the MTU of next hop is 0, this will reset the MTU of the
> > * route to run PMTUD again from scratch.
> > */
> > - if (!ISSET(rt->rt_locks, RTV_MTU)) {
> > + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> > u_int mtu, nhmtu;
> >
> > mtu = atomic_load_int(&rt->rt_mtu);
> > Index: net/route.h
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
> > diff -u -p -r1.211 route.h
> > --- net/route.h 3 Jan 2025 21:27:40 -0000 1.211
> > +++ net/route.h 13 Jan 2025 22:50:54 -0000
> > @@ -38,6 +38,7 @@
> > /*
> > * Locks used to protect struct members in this file:
> > * I immutable after creation
> > + * a atomic operations
> > * N net lock
> > * X exclusive net lock, or shared net lock + kernel lock
> > * R art (rtable) lock
> > @@ -60,7 +61,7 @@
> > struct rt_kmetrics {
> > u_int64_t rmx_pksent; /* packets sent using this route */
> > int64_t rmx_expire; /* lifetime for route, e.g. redirect */
> > - u_int rmx_locks; /* Kernel must leave these values */
> > + u_int rmx_locks; /* [a] Kernel must leave these values */
> > u_int rmx_mtu; /* [a] MTU for this path */
> > };
> > #endif
> > Index: net/rtsock.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v
> > diff -u -p -r1.377 rtsock.c
> > --- net/rtsock.c 9 Jan 2025 18:20:29 -0000 1.377
> > +++ net/rtsock.c 13 Jan 2025 22:50:54 -0000
> > @@ -1198,8 +1198,9 @@ change:
> > }
> > if_group_routechange(info->rti_info[RTAX_DST],
> > info->rti_info[RTAX_NETMASK]);
> > - rt->rt_locks &= ~(rtm->rtm_inits);
> > - rt->rt_locks |= (rtm->rtm_inits & rtm->rtm_rmx.rmx_locks);
> > + atomic_clearbits_int(&rt->rt_locks, rtm->rtm_inits);
> > + atomic_setbits_int(&rt->rt_locks,
> > + rtm->rtm_inits & rtm->rtm_rmx.rmx_locks);
> > NET_UNLOCK();
> > break;
> > case RTM_GET:
> > @@ -1344,7 +1345,7 @@ route_cleargateway(struct rtentry *rt, v
> > struct rtentry *nhrt = arg;
> >
> > if (ISSET(rt->rt_flags, RTF_GATEWAY) && rt->rt_gwroute == nhrt &&
> > - !ISSET(rt->rt_locks, RTV_MTU))
> > + !ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> > atomic_store_int(&rt->rt_mtu, 0);
> >
> > return (0);
> > @@ -1420,7 +1421,7 @@ rtm_getmetrics(const struct rtentry *rt,
> > }
> >
> > bzero(out, sizeof(*out));
> > - out->rmx_locks = in->rmx_locks;
> > + out->rmx_locks = atomic_load_int(&in->rmx_locks);
> > out->rmx_mtu = atomic_load_int(&in->rmx_mtu);
> > out->rmx_expire = expire;
> > out->rmx_pksent = in->rmx_pksent;
> > Index: netinet/ip_icmp.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
> > diff -u -p -r1.198 ip_icmp.c
> > --- netinet/ip_icmp.c 3 Jan 2025 21:27:40 -0000 1.198
> > +++ netinet/ip_icmp.c 13 Jan 2025 22:50:54 -0000
> > @@ -1054,9 +1054,9 @@ icmp_mtudisc(struct icmp *icp, u_int rta
> > * on a route. We should be using a separate flag
> > * for the kernel to indicate this.
> > */
> > - if ((rt->rt_locks & RTV_MTU) == 0) {
> > + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> > if (mtu < 296 || mtu > ifp->if_mtu)
> > - rt->rt_locks |= RTV_MTU;
> > + atomic_setbits_int(&rt->rt_locks, RTV_MTU);
> > else if (rtmtu > mtu || rtmtu == 0)
> > atomic_cas_uint(&rt->rt_mtu, rtmtu, mtu);
> > }
> > @@ -1090,7 +1090,7 @@ icmp_mtudisc_timeout(struct rtentry *rt,
> > (*ctlfunc)(PRC_MTUINC, sintosa(&sin),
> > rtableid, NULL);
> > } else {
> > - if ((rt->rt_locks & RTV_MTU) == 0)
> > + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> > atomic_store_int(&rt->rt_mtu, 0);
> > }
> >
> > Index: netinet/ip_output.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v
> > diff -u -p -r1.402 ip_output.c
> > --- netinet/ip_output.c 3 Jan 2025 21:27:40 -0000 1.402
> > +++ netinet/ip_output.c 13 Jan 2025 22:50:54 -0000
> > @@ -384,7 +384,7 @@ sendit:
> > * the route's MTU is locked.
> > */
> > if ((flags & IP_MTUDISC) && ro && ro->ro_rt &&
> > - (ro->ro_rt->rt_locks & RTV_MTU) == 0)
> > + (!ISSET(atomic_load_int(&ro->ro_rt->rt_locks), RTV_MTU)))
> > ip->ip_off |= htons(IP_DF);
> >
> > #ifdef IPSEC
> > @@ -471,7 +471,7 @@ sendit:
> > */
> > if (rtisvalid(ro->ro_rt) &&
> > ISSET(ro->ro_rt->rt_flags, RTF_HOST) &&
> > - !(ro->ro_rt->rt_locks & RTV_MTU)) {
> > + !ISSET(atomic_load_int(&ro->ro_rt->rt_locks), RTV_MTU)) {
> > u_int rtmtu;
> >
> > rtmtu = atomic_load_int(&ro->ro_rt->rt_mtu);
> > Index: netinet/tcp_subr.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_subr.c,v
> > diff -u -p -r1.204 tcp_subr.c
> > --- netinet/tcp_subr.c 3 Jan 2025 17:23:51 -0000 1.204
> > +++ netinet/tcp_subr.c 13 Jan 2025 22:50:54 -0000
> > @@ -868,7 +868,8 @@ tcp_mtudisc(struct inpcb *inp, int errno
> >
> > rt = in_pcbrtentry(inp);
> > if (rt != NULL) {
> > - unsigned int orig_mtulock = (rt->rt_locks & RTV_MTU);
> > + unsigned int orig_mtulock =
> > + (atomic_load_int(&rt->rt_locks) & RTV_MTU);
> >
> > /*
> > * If this was not a host route, remove and realloc.
> > @@ -878,7 +879,7 @@ tcp_mtudisc(struct inpcb *inp, int errno
> > if ((rt = in_pcbrtentry(inp)) == NULL)
> > return;
> > }
> > - if (orig_mtulock < (rt->rt_locks & RTV_MTU))
> > + if (orig_mtulock < (atomic_load_int(&rt->rt_locks) & RTV_MTU))
> > change = 1;
> > }
> > tcp_mss(tp, -1);
> > Index: netinet/tcp_timer.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_timer.c,v
> > diff -u -p -r1.80 tcp_timer.c
> > --- netinet/tcp_timer.c 5 Jan 2025 12:18:48 -0000 1.80
> > +++ netinet/tcp_timer.c 13 Jan 2025 22:50:54 -0000
> > @@ -282,7 +282,7 @@ tcp_timer_rexmt(void *arg)
> > rt = in_pcbrtentry(inp);
> > /* Check if path MTU discovery is disabled already */
> > if (rt && (rt->rt_flags & RTF_HOST) &&
> > - (rt->rt_locks & RTV_MTU))
> > + ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> > goto leave;
> >
> > rt = NULL;
> > @@ -303,8 +303,8 @@ tcp_timer_rexmt(void *arg)
> > }
> > if (rt != NULL) {
> > /* Disable path MTU discovery */
> > - if ((rt->rt_locks & RTV_MTU) == 0) {
> > - rt->rt_locks |= RTV_MTU;
> > + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> > + atomic_setbits_int(&rt->rt_locks, RTV_MTU);
> > in_rtchange(inp, 0);
> > }
> >
> > Index: netinet6/icmp6.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/icmp6.c,v
> > diff -u -p -r1.256 icmp6.c
> > --- netinet6/icmp6.c 3 Jan 2025 21:27:40 -0000 1.256
> > +++ netinet6/icmp6.c 13 Jan 2025 22:50:54 -0000
> > @@ -1016,7 +1016,7 @@ icmp6_mtudisc_update(struct ip6ctlparam
> > rt = icmp6_mtudisc_clone(&sin6, m->m_pkthdr.ph_rtableid, 0);
> >
> > if (rt != NULL && ISSET(rt->rt_flags, RTF_HOST) &&
> > - !(rt->rt_locks & RTV_MTU)) {
> > + !ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> > u_int rtmtu;
> >
> > rtmtu = atomic_load_int(&rt->rt_mtu);
> > @@ -1851,7 +1851,7 @@ icmp6_mtudisc_timeout(struct rtentry *rt
> > if ((rt->rt_flags & (RTF_DYNAMIC|RTF_HOST)) == (RTF_DYNAMIC|RTF_HOST)) {
> > rtdeletemsg(rt, ifp, rtableid);
> > } else {
> > - if (!(rt->rt_locks & RTV_MTU))
> > + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> > atomic_store_int(&rt->rt_mtu, 0);
> > }
> >
> > Index: netinet6/ip6_output.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_output.c,v
> > diff -u -p -r1.294 ip6_output.c
> > --- netinet6/ip6_output.c 3 Jan 2025 21:27:40 -0000 1.294
> > +++ netinet6/ip6_output.c 13 Jan 2025 22:50:54 -0000
> > @@ -1047,7 +1047,7 @@ ip6_getpmtu(struct rtentry *rt, struct i
> > * field isn't locked).
> > */
> > mtu = ifp->if_mtu;
> > - if (!(rt->rt_locks & RTV_MTU))
> > + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> > atomic_cas_uint(&rt->rt_mtu, rtmtu, mtu);
> > }
> > } else {
> >
>
> --
> :wq Claudio
make route rt_locks atomic