Re: Time to retire RTM_LOSING

2018-07-11 Thread Claudio Jeker
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

2018-07-11 Thread Martin Pieuchot
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);
>