On Tue, May 24, 2016 at 09:27:54AM +0200, Martin Pieuchot wrote:
> On 18/05/16(Wed) 22:56, Martin Pieuchot wrote:
> > Back in the old cool days you could simply call ether_output() with a
> > stuffed sockaddr and it will do the L2 resolution for you... Well as
> > long as you do not care about IPv6 it's true.
> >
> > During the last years I tried to reduce the number of places where
> > ether_output() was called without passing it a "struct rtentry" as
> > last argument. The reason for this move is to remove a potential
> > route insertion in arpresolve(), done in read-only context (hot path).
>
> New diff with ND bits. Please test and report back.
Running this on my laptop, no issues so far. OK bluhm@
>
> Index: net/if_ethersubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.236
> diff -u -p -r1.236 if_ethersubr.c
> --- net/if_ethersubr.c 18 May 2016 20:15:14 -0000 1.236
> +++ net/if_ethersubr.c 23 May 2016 12:21:45 -0000
> @@ -193,8 +193,12 @@ ether_output(struct ifnet *ifp, struct m
> struct mbuf *mcopy = NULL;
> struct ether_header *eh;
> struct arpcom *ac = (struct arpcom *)ifp;
> + sa_family_t af = dst->sa_family;
> int error = 0;
>
> + KASSERT(rt != NULL || ISSET(m->m_flags, M_MCAST|M_BCAST) ||
> + af == AF_UNSPEC || af == pseudo_AF_HDRCMPLT);
> +
> #ifdef DIAGNOSTIC
> if (ifp->if_rdomain != rtable_l2(m->m_pkthdr.ph_rtableid)) {
> printf("%s: trying to send packet on wrong domain. "
> @@ -208,7 +212,7 @@ ether_output(struct ifnet *ifp, struct m
> if ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) != (IFF_UP|IFF_RUNNING))
> senderr(ENETDOWN);
>
> - switch (dst->sa_family) {
> + switch (af) {
> case AF_INET:
> error = arpresolve(ifp, rt, m, dst, edst);
> if (error)
> Index: netinet/if_ether.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 if_ether.c
> --- netinet/if_ether.c 23 May 2016 09:23:43 -0000 1.209
> +++ netinet/if_ether.c 23 May 2016 12:21:45 -0000
> @@ -281,9 +281,8 @@ arpresolve(struct ifnet *ifp, struct rte
> struct llinfo_arp *la = NULL;
> struct sockaddr_dl *sdl;
> struct rtentry *rt = NULL;
> - struct mbuf *mh;
> char addr[INET_ADDRSTRLEN];
> - int error, created = 0;
> + int error;
>
> if (m->m_flags & M_BCAST) { /* broadcast */
> memcpy(desten, etherbroadcastaddr, sizeof(etherbroadcastaddr));
> @@ -294,41 +293,20 @@ arpresolve(struct ifnet *ifp, struct rte
> return (0);
> }
>
> - if (rt0 != NULL) {
> - error = rt_checkgate(rt0, &rt);
> - if (error) {
> - m_freem(m);
> - return (error);
> - }
> -
> - if ((rt->rt_flags & RTF_LLINFO) == 0) {
> - log(LOG_DEBUG, "%s: %s: route contains no arp"
> - " information\n", __func__, inet_ntop(AF_INET,
> - &satosin(rt_key(rt))->sin_addr, addr,
> - sizeof(addr)));
> - m_freem(m);
> - return (EINVAL);
> - }
> + error = rt_checkgate(rt0, &rt);
> + if (error) {
> + m_freem(m);
> + return (error);
> + }
>
> - la = (struct llinfo_arp *)rt->rt_llinfo;
> - if (la == NULL)
> - log(LOG_DEBUG, "%s: %s: route without link "
> - "local address\n", __func__, inet_ntop(AF_INET,
> - &satosin(dst)->sin_addr, addr, sizeof(addr)));
> - } else {
> - rt = arplookup(&satosin(dst)->sin_addr, 1, 0, ifp->if_rdomain);
> - if (rt != NULL) {
> - created = 1;
> - la = ((struct llinfo_arp *)rt->rt_llinfo);
> - }
> - if (la == NULL)
> - log(LOG_DEBUG, "%s: %s: can't allocate llinfo\n",
> - __func__,
> - inet_ntop(AF_INET, &satosin(dst)->sin_addr,
> - addr, sizeof(addr)));
> + if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
> + log(LOG_DEBUG, "%s: %s: route contains no arp information\n",
> + __func__, inet_ntop(AF_INET, &satosin(rt_key(rt))->sin_addr,
> + addr, sizeof(addr)));
> + m_freem(m);
> + return (EINVAL);
> }
> - if (la == NULL || rt == NULL)
> - goto bad;
> +
> sdl = satosdl(rt->rt_gateway);
> if (sdl->sdl_alen > 0 && sdl->sdl_alen != ETHER_ADDR_LEN) {
> log(LOG_DEBUG, "%s: %s: incorrect arp information\n", __func__,
> @@ -336,6 +314,7 @@ arpresolve(struct ifnet *ifp, struct rte
> addr, sizeof(addr)));
> goto bad;
> }
> +
> /*
> * Check the address family and length is valid, the address
> * is resolved; otherwise, try to resolve.
> @@ -343,10 +322,9 @@ arpresolve(struct ifnet *ifp, struct rte
> if ((rt->rt_expire == 0 || rt->rt_expire > time_second) &&
> sdl->sdl_family == AF_LINK && sdl->sdl_alen != 0) {
> memcpy(desten, LLADDR(sdl), sdl->sdl_alen);
> - if (created)
> - rtfree(rt);
> return (0);
> }
> +
> if (ifp->if_flags & IFF_NOARP)
> goto bad;
>
> @@ -355,7 +333,11 @@ arpresolve(struct ifnet *ifp, struct rte
> * response yet. Insert mbuf in hold queue if below limit
> * if above the limit free the queue without queuing the new packet.
> */
> + la = (struct llinfo_arp *)rt->rt_llinfo;
> + KASSERT(la != NULL);
> if (la_hold_total < LA_HOLD_TOTAL && la_hold_total < nmbclust / 64) {
> + struct mbuf *mh;
> +
> if (ml_len(&la->la_ml) >= LA_HOLD_QUEUE) {
> mh = ml_dequeue(&la->la_ml);
> la_hold_total--;
> @@ -396,14 +378,11 @@ arpresolve(struct ifnet *ifp, struct rte
> }
> }
> }
> - if (created)
> - rtfree(rt);
> +
> return (EAGAIN);
>
> bad:
> m_freem(m);
> - if (created)
> - rtfree(rt);
> return (EINVAL);
> }
>
> Index: netinet6/nd6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.179
> diff -u -p -r1.179 nd6.c
> --- netinet6/nd6.c 17 May 2016 08:29:14 -0000 1.179
> +++ netinet6/nd6.c 23 May 2016 12:38:56 -0000
> @@ -1507,20 +1507,15 @@ nd6_output(struct ifnet *ifp, struct mbu
> struct mbuf *m = m0;
> struct rtentry *rt = rt0;
> struct llinfo_nd6 *ln = NULL;
> - int created = 0, error = 0;
> + int error = 0;
>
> if (IN6_IS_ADDR_MULTICAST(&dst->sin6_addr))
> goto sendpkt;
>
> - /*
> - * next hop determination.
> - */
> - if (rt0 != NULL) {
> - error = rt_checkgate(rt0, &rt);
> - if (error) {
> - m_freem(m);
> - return (error);
> - }
> + error = rt_checkgate(rt0, &rt);
> + if (error) {
> + m_freem(m);
> + return (error);
> }
>
> if (nd6_need_cache(ifp) == 0)
> @@ -1532,42 +1527,17 @@ nd6_output(struct ifnet *ifp, struct mbu
> * At this point, the destination of the packet must be a unicast
> * or an anycast address(i.e. not a multicast).
> */
> -
> - /* Look up the neighbor cache for the nexthop */
> - if (rt != NULL && (rt->rt_flags & RTF_LLINFO) != 0)
> - ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> - else {
> - /*
> - * Since nd6_is_addr_neighbor() internally calls nd6_lookup(),
> - * the condition below is not very efficient. But we believe
> - * it is tolerable, because this should be a rare case.
> - */
> - if (nd6_is_addr_neighbor(dst, ifp)) {
> - rt = nd6_lookup(&dst->sin6_addr, 1, ifp,
> - ifp->if_rdomain);
> - if (rt != NULL) {
> - created = 1;
> - ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> - }
> - }
> + if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
> + char addr[INET6_ADDRSTRLEN];
> + log(LOG_DEBUG, "%s: %s: route contains no ND information\n",
> + __func__, inet_ntop(AF_INET6,
> + &satosin6(rt_key(rt))->sin6_addr, addr, sizeof(addr)));
> + m_freem(m);
> + return (EINVAL);
> }
> - if (ln == NULL || rt == NULL) {
> - if ((ND_IFINFO(ifp)->flags & ND6_IFF_PERFORMNUD) == 0) {
> - char addr[INET6_ADDRSTRLEN];
> -
> - log(LOG_DEBUG, "%s: can't allocate llinfo for %s "
> - "(ln=%p, rt=%p)\n", __func__,
> - inet_ntop(AF_INET6, &dst->sin6_addr,
> - addr, sizeof(addr)),
> - ln, rt);
> - m_freem(m);
> - if (created)
> - rtfree(rt);
> - return (EIO); /* XXX: good error? */
> - }
>
> - goto sendpkt; /* send anyway */
> - }
> + ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> + KASSERT(ln != NULL);
>
> /*
> * Move this entry to the head of the queue so that it is less likely
> @@ -1617,14 +1587,10 @@ nd6_output(struct ifnet *ifp, struct mbu
> (long)ND_IFINFO(ifp)->retrans * hz / 1000);
> nd6_ns_output(ifp, NULL, &dst->sin6_addr, ln, 0);
> }
> - if (created)
> - rtfree(rt);
> return (0);
>
> sendpkt:
> error = ifp->if_output(ifp, m, sin6tosa(dst), rt);
> - if (created)
> - rtfree(rt);
> return (error);
> }
>
> @@ -1665,12 +1631,6 @@ nd6_storelladdr(struct ifnet *ifp, struc
> m_freem(m);
> return (EINVAL);
> }
> - }
> -
> - if (rt0 == NULL) {
> - /* this could happen, if we could not allocate memory */
> - m_freem(m);
> - return (ENOMEM);
> }
>
> error = rt_checkgate(rt0, &rt);
