svn commit: r285051 - head/sys/netinet

[Ermal Luçi]

On Tue, Jul 28, 2015 at 2:42 PM, Gleb Smirnoff <glebius at freebsd.org> wrote:

>   Ermal,
>
>   see comments inlined,
>
> On Thu, Jul 02, 2015 at 06:10:42PM +0000, Ermal Luçi wrote:
> E> Author: eri
> E> Date: Thu Jul  2 18:10:41 2015
> E> New Revision: 285051
> E> URL: https://svnweb.freebsd.org/changeset/base/285051
> E>
> E> Log:
> E>   Avoid doing multiple route lookups for the same destination IP during
> forwarding
> E>
> E>   ip_forward() does a route lookup for testing this packet can be sent
> to a known destination,
> E>   it also can do another route lookup if it detects that an ICMP
> redirect is needed,
> E>   it forgets all of this and handovers to ip_output() to do the same
> lookup yet again.
> E>
> E>   This optimisation just does one route lookup during the forwarding
> path and handovers that to be considered by ip_output().
> E>
> E>   Differential Revision:     https://reviews.freebsd.org/D2964
> E>   Approved by:       ae, gnn(mentor)
> E>   MFC after: 1 week
> E>
> E> Modified:
> E>   head/sys/netinet/ip_input.c
> E>
> E> Modified: head/sys/netinet/ip_input.c
> E>
> ==============================================================================
> E> --- head/sys/netinet/ip_input.c      Thu Jul  2 17:30:59 2015
> (r285050)
> E> +++ head/sys/netinet/ip_input.c      Thu Jul  2 18:10:41 2015
> (r285051)
> E> @@ -897,6 +897,7 @@ ip_forward(struct mbuf *m, int srcrt)
> E>      struct ip *ip = mtod(m, struct ip *);
> E>      struct in_ifaddr *ia;
> E>      struct mbuf *mcopy;
> E> +    struct sockaddr_in *sin;
> E>      struct in_addr dest;
> E>      struct route ro;
> E>      int error, type = 0, code = 0, mtu = 0;
> E> @@ -925,7 +926,22 @@ ip_forward(struct mbuf *m, int srcrt)
> E>      }
> E>  #endif
> E>
> E> -    ia = ip_rtaddr(ip->ip_dst, M_GETFIB(m));
> E> +    bzero(&ro, sizeof(ro));
> E> +    sin = (struct sockaddr_in *)&ro.ro_dst;
> E> +    sin->sin_family = AF_INET;
> E> +    sin->sin_len = sizeof(*sin);
> E> +    sin->sin_addr = ip->ip_dst;
> E> +#ifdef RADIX_MPATH
> E> +    rtalloc_mpath_fib(&ro,
> E> +        ntohl(ip->ip_src.s_addr ^ ip->ip_dst.s_addr),
> E> +        M_GETFIB(m));
> E> +#else
> E> +    in_rtalloc_ign(&ro, 0, M_GETFIB(m));
> E> +#endif
> E> +    if (ro.ro_rt != NULL) {
> E> +            ia = ifatoia(ro.ro_rt->rt_ifa);
> E> +            ifa_ref(&ia->ia_ifa);
> E> +    }
> E>  #ifndef IPSEC
> E>      /*
> E>       * 'ia' may be NULL if there is no route for this destination.
> E> @@ -934,6 +950,7 @@ ip_forward(struct mbuf *m, int srcrt)
> E>       */
> E>      if (!srcrt && ia == NULL) {
> E>              icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_HOST, 0, 0);
> E> +            RO_RTFREE(&ro);
> E>              return;
> E>      }
>
> Here the ifa reference is leaked upon return.
>
>
Gleb,

the improvement on the ifa_ref not needed is something to look at but the
ifa_ref here is not lost since ia == NULL, no?

Maybe i am missing something else.
Also can we put this on a review?


>
> But don't hurry with fixing that :) Actually you don't need to ifa_ref()
> in this function. You acquired a reference on rtentry in in_rtalloc_ign()
> and hold it until RO_RTFREE(). And the rtentry itself always holds a
> reference on the ifa. So, there is no reason to put extra reference on
> the ifa.
>
> The ip_output() was already improved in r262747. And ip_forward() can
> also be. The only place that touches ia after RO_RTFREE() is EMSGSIZE
> handling, this can be moved up before RO_RTFREE().
>
> Here is suggested patch. Ermal and Oliver, can you please test/benchmark
> it?
>
> --
> Totus tuus, Glebius.
>



-- 
Ermal