Re: Time to retire RTM_LOSING
On Wed, Jul 11, 2018 at 10:10:50AM +0200, Martin Pieuchot wrote:
> On 11/07/18(Wed) 09:55, Claudio Jeker wrote:
> > On busy servers I seen multiple RTM_LOSING message per second being
> > generated. This is not helpful (especially since nothing is doing
> > something with it). This diff removes the part where RTM_LOSING is
> > generated
>
> I'm fine with that. However what about adding a tcp_trace() call
> instead? This could still be useful for debugging.
tcp_timer_rexmt() which is the only caller of in_losing() is already doing
a tcp_trace call at the end which is hit in that case.
tcp_trace(TA_TIMER, ostate, tp, otp, NULL, TCPT_REXMT, 0);
There is some other cases in tcp_timer_rexmt() which skip the tcp_trace()
especially the block that calls into icmp_mtudisc().
Will talk to bluhm@ about this once he shows up.
> > but at the same time adds some RTM_ADD / RTM_DELETE messages for
> > the dynamic routes added by the PMTU handlers. In my opinion adding even
> > dynamic routes to the routing table should be communicated on the routing
> > socket (also the removes because of the timeout will already show up).
> > Only lightly tested since I'm not having access to the box I have seen
> > this anymore.
>
> I like it. I'd only call rtm_send(9) if rtrequest() didn't returned an
> error, like it is done in rt_ifa_add/del().
Right, it does not make sense to report that error up.
Adapted the diff to do that.
--
:wq Claudio
Index: netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.239
diff -u -p -r1.239 in_pcb.c
--- netinet/in_pcb.c14 Jun 2018 17:16:03 - 1.239
+++ netinet/in_pcb.c10 Jul 2018 21:31:59 -
@@ -709,21 +709,11 @@ in_pcbnotifyall(struct inpcbtable *table
void
in_losing(struct inpcb *inp)
{
- struct rtentry *rt;
- struct rt_addrinfo info;
- struct sockaddr_in6 sa_mask;
+ struct rtentry *rt = inp->inp_route.ro_rt;
- if ((rt = inp->inp_route.ro_rt)) {
- inp->inp_route.ro_rt = 0;
+ if (rt) {
+ inp->inp_route.ro_rt = NULL;
- memset(, 0, sizeof(info));
- info.rti_flags = rt->rt_flags;
- info.rti_info[RTAX_DST] = >inp_route.ro_dst;
- info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
- info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
-
- rtm_miss(RTM_LOSING, , rt->rt_flags, rt->rt_priority,
- rt->rt_ifidx, 0, inp->inp_rtableid);
if (rt->rt_flags & RTF_DYNAMIC) {
struct ifnet *ifp;
@@ -734,10 +724,8 @@ in_losing(struct inpcb *inp)
* so we're dealing with a stale cache and have
* nothing to do.
*/
- if (ifp != NULL) {
- rtrequest_delete(, rt->rt_priority, ifp,
- NULL, inp->inp_rtableid);
- }
+ if (ifp != NULL)
+ rtdeletemsg(rt, ifp, inp->inp_rtableid);
if_put(ifp);
}
/*
Index: netinet/ip_icmp.c
===
RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.175
diff -u -p -r1.175 ip_icmp.c
--- netinet/ip_icmp.c 21 May 2018 15:52:22 - 1.175
+++ netinet/ip_icmp.c 11 Jul 2018 08:18:04 -
@@ -990,6 +990,7 @@ icmp_mtudisc_clone(struct in_addr dst, u
nrt->rt_rmx = rt->rt_rmx;
rtfree(rt);
rt = nrt;
+ rtm_send(rt, RTM_ADD, error, rtableid);
}
error = rt_timer_add(rt, icmp_mtudisc_timeout, ip_mtudisc_timeout_q,
rtableid);
Index: netinet6/icmp6.c
===
RCS file: /cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.224
diff -u -p -r1.224 icmp6.c
--- netinet6/icmp6.c2 Jun 2018 16:38:21 - 1.224
+++ netinet6/icmp6.c11 Jul 2018 08:17:24 -
@@ -1806,6 +1806,7 @@ icmp6_mtudisc_clone(struct sockaddr *dst
nrt->rt_rmx = rt->rt_rmx;
rtfree(rt);
rt = nrt;
+ rtm_send(rt, RTM_ADD, error, rtableid);
}
error = rt_timer_add(rt, icmp6_mtudisc_timeout, icmp6_mtudisc_timeout_q,
rtableid);
Re: Time to retire RTM_LOSING
On 11/07/18(Wed) 09:55, Claudio Jeker wrote:
> On busy servers I seen multiple RTM_LOSING message per second being
> generated. This is not helpful (especially since nothing is doing
> something with it). This diff removes the part where RTM_LOSING is
> generated
I'm fine with that. However what about adding a tcp_trace() call
instead? This could still be useful for debugging.
> but at the same time adds some RTM_ADD / RTM_DELETE messages for
> the dynamic routes added by the PMTU handlers. In my opinion adding even
> dynamic routes to the routing table should be communicated on the routing
> socket (also the removes because of the timeout will already show up).
> Only lightly tested since I'm not having access to the box I have seen
> this anymore.
I like it. I'd only call rtm_send(9) if rtrequest() didn't returned an
error, like it is done in rt_ifa_add/del().
> Index: netinet/in_pcb.c
> ===
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.239
> diff -u -p -r1.239 in_pcb.c
> --- netinet/in_pcb.c 14 Jun 2018 17:16:03 - 1.239
> +++ netinet/in_pcb.c 10 Jul 2018 21:19:09 -
> @@ -709,21 +709,11 @@ in_pcbnotifyall(struct inpcbtable *table
> void
> in_losing(struct inpcb *inp)
> {
> - struct rtentry *rt;
> - struct rt_addrinfo info;
> - struct sockaddr_in6 sa_mask;
> + struct rtentry *rt = inp->inp_route.ro_rt;
>
> - if ((rt = inp->inp_route.ro_rt)) {
> - inp->inp_route.ro_rt = 0;
> + if (rt) {
> + inp->inp_route.ro_rt = NULL;
>
> - memset(, 0, sizeof(info));
> - info.rti_flags = rt->rt_flags;
> - info.rti_info[RTAX_DST] = >inp_route.ro_dst;
> - info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
> - info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
> -
> - rtm_miss(RTM_LOSING, , rt->rt_flags, rt->rt_priority,
> - rt->rt_ifidx, 0, inp->inp_rtableid);
> if (rt->rt_flags & RTF_DYNAMIC) {
> struct ifnet *ifp;
>
> @@ -734,10 +724,8 @@ in_losing(struct inpcb *inp)
>* so we're dealing with a stale cache and have
>* nothing to do.
>*/
> - if (ifp != NULL) {
> - rtrequest_delete(, rt->rt_priority, ifp,
> - NULL, inp->inp_rtableid);
> - }
> + if (ifp != NULL)
> + rtdeletemsg(rt, ifp, inp->inp_rtableid);
> if_put(ifp);
> }
> /*
> Index: netinet/ip_icmp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.175
> diff -u -p -r1.175 ip_icmp.c
> --- netinet/ip_icmp.c 21 May 2018 15:52:22 - 1.175
> +++ netinet/ip_icmp.c 10 Jul 2018 21:09:13 -
> @@ -983,6 +983,7 @@ icmp_mtudisc_clone(struct in_addr dst, u
>
> error = rtrequest(RTM_ADD, , rt->rt_priority, ,
> rtableid);
> + rtm_send(nrt, RTM_ADD, error, rtableid);
> if (error) {
> rtfree(rt);
> return (NULL);
> Index: netinet6/icmp6.c
> ===
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.224
> diff -u -p -r1.224 icmp6.c
> --- netinet6/icmp6.c 2 Jun 2018 16:38:21 - 1.224
> +++ netinet6/icmp6.c 10 Jul 2018 21:18:13 -
> @@ -1799,6 +1799,7 @@ icmp6_mtudisc_clone(struct sockaddr *dst
>
> error = rtrequest(RTM_ADD, , rt->rt_priority, ,
> rtableid);
> + rtm_send(nrt, RTM_ADD, error, rtableid);
> if (error) {
> rtfree(rt);
> return (NULL);
>
Time to retire RTM_LOSING
On busy servers I seen multiple RTM_LOSING message per second being
generated. This is not helpful (especially since nothing is doing
something with it). This diff removes the part where RTM_LOSING is
generated but at the same time adds some RTM_ADD / RTM_DELETE messages for
the dynamic routes added by the PMTU handlers. In my opinion adding even
dynamic routes to the routing table should be communicated on the routing
socket (also the removes because of the timeout will already show up).
Only lightly tested since I'm not having access to the box I have seen
this anymore.
--
:wq Claudio
Index: netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.239
diff -u -p -r1.239 in_pcb.c
--- netinet/in_pcb.c14 Jun 2018 17:16:03 - 1.239
+++ netinet/in_pcb.c10 Jul 2018 21:19:09 -
@@ -709,21 +709,11 @@ in_pcbnotifyall(struct inpcbtable *table
void
in_losing(struct inpcb *inp)
{
- struct rtentry *rt;
- struct rt_addrinfo info;
- struct sockaddr_in6 sa_mask;
+ struct rtentry *rt = inp->inp_route.ro_rt;
- if ((rt = inp->inp_route.ro_rt)) {
- inp->inp_route.ro_rt = 0;
+ if (rt) {
+ inp->inp_route.ro_rt = NULL;
- memset(, 0, sizeof(info));
- info.rti_flags = rt->rt_flags;
- info.rti_info[RTAX_DST] = >inp_route.ro_dst;
- info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
- info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
-
- rtm_miss(RTM_LOSING, , rt->rt_flags, rt->rt_priority,
- rt->rt_ifidx, 0, inp->inp_rtableid);
if (rt->rt_flags & RTF_DYNAMIC) {
struct ifnet *ifp;
@@ -734,10 +724,8 @@ in_losing(struct inpcb *inp)
* so we're dealing with a stale cache and have
* nothing to do.
*/
- if (ifp != NULL) {
- rtrequest_delete(, rt->rt_priority, ifp,
- NULL, inp->inp_rtableid);
- }
+ if (ifp != NULL)
+ rtdeletemsg(rt, ifp, inp->inp_rtableid);
if_put(ifp);
}
/*
Index: netinet/ip_icmp.c
===
RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.175
diff -u -p -r1.175 ip_icmp.c
--- netinet/ip_icmp.c 21 May 2018 15:52:22 - 1.175
+++ netinet/ip_icmp.c 10 Jul 2018 21:09:13 -
@@ -983,6 +983,7 @@ icmp_mtudisc_clone(struct in_addr dst, u
error = rtrequest(RTM_ADD, , rt->rt_priority, ,
rtableid);
+ rtm_send(nrt, RTM_ADD, error, rtableid);
if (error) {
rtfree(rt);
return (NULL);
Index: netinet6/icmp6.c
===
RCS file: /cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.224
diff -u -p -r1.224 icmp6.c
--- netinet6/icmp6.c2 Jun 2018 16:38:21 - 1.224
+++ netinet6/icmp6.c10 Jul 2018 21:18:13 -
@@ -1799,6 +1799,7 @@ icmp6_mtudisc_clone(struct sockaddr *dst
error = rtrequest(RTM_ADD, , rt->rt_priority, ,
rtableid);
+ rtm_send(nrt, RTM_ADD, error, rtableid);
if (error) {
rtfree(rt);
return (NULL);
