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.
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.
Index: ip_input.c
===================================================================
--- ip_input.c (revision 285945)
+++ ip_input.c (working copy)
@@ -938,10 +938,9 @@ ip_forward(struct mbuf *m, int srcrt)
#else
in_rtalloc_ign(&ro, 0, M_GETFIB(m));
#endif
- if (ro.ro_rt != NULL) {
+ if (ro.ro_rt != NULL)
ia = ifatoia(ro.ro_rt->rt_ifa);
- ifa_ref(&ia->ia_ifa);
- } else
+ else
ia = NULL;
#ifndef IPSEC
/*
@@ -1031,9 +1030,33 @@ ip_forward(struct mbuf *m, int srcrt)
}
error = ip_output(m, NULL, &ro, IP_FORWARDING, NULL, NULL);
-
- if (error == EMSGSIZE && ro.ro_rt)
- mtu = ro.ro_rt->rt_mtu;
+ if (error == EMSGSIZE) {
+ if (ro.ro_rt != NULL)
+ mtu = ro.ro_rt->rt_mtu;
+#ifdef IPSEC
+ /*
+ * If IPsec is configured for this path,
+ * override any possibly mtu value set by ip_output.
+ */
+ mtu = ip_ipsec_mtu(mcopy, mtu);
+#endif /* IPSEC */
+ /*
+ * If the MTU was set before make sure we are below the
+ * interface MTU.
+ * If the MTU wasn't set before use the interface mtu or
+ * fall back to the next smaller mtu step compared to the
+ * current packet size.
+ */
+ if (mtu != 0) {
+ if (ia != NULL)
+ mtu = min(mtu, ia->ia_ifp->if_mtu);
+ } else {
+ if (ia != NULL)
+ mtu = ia->ia_ifp->if_mtu;
+ else
+ mtu = ip_next_mtu(ntohs(ip->ip_len), 0);
+ }
+ }
RO_RTFREE(&ro);
if (error)
@@ -1045,16 +1068,11 @@ ip_forward(struct mbuf *m, int srcrt)
else {
if (mcopy)
m_freem(mcopy);
- if (ia != NULL)
- ifa_free(&ia->ia_ifa);
return;
}
}
- if (mcopy == NULL) {
- if (ia != NULL)
- ifa_free(&ia->ia_ifa);
+ if (mcopy == NULL)
return;
- }
switch (error) {
@@ -1074,30 +1092,6 @@ ip_forward(struct mbuf *m, int srcrt)
case EMSGSIZE:
type = ICMP_UNREACH;
code = ICMP_UNREACH_NEEDFRAG;
-
-#ifdef IPSEC
- /*
- * If IPsec is configured for this path,
- * override any possibly mtu value set by ip_output.
- */
- mtu = ip_ipsec_mtu(mcopy, mtu);
-#endif /* IPSEC */
- /*
- * If the MTU was set before make sure we are below the
- * interface MTU.
- * If the MTU wasn't set before use the interface mtu or
- * fall back to the next smaller mtu step compared to the
- * current packet size.
- */
- if (mtu != 0) {
- if (ia != NULL)
- mtu = min(mtu, ia->ia_ifp->if_mtu);
- } else {
- if (ia != NULL)
- mtu = ia->ia_ifp->if_mtu;
- else
- mtu = ip_next_mtu(ntohs(ip->ip_len), 0);
- }
IPSTAT_INC(ips_cantfrag);
break;
@@ -1104,12 +1098,8 @@ ip_forward(struct mbuf *m, int srcrt)
case ENOBUFS:
case EACCES: /* ipfw denied packet */
m_freem(mcopy);
- if (ia != NULL)
- ifa_free(&ia->ia_ifa);
return;
}
- if (ia != NULL)
- ifa_free(&ia->ia_ifa);
icmp_error(mcopy, type, code, dest.s_addr, mtu);
}
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
