Re: refactoring ip_output() and the L2 _output()

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

David Young <dyoung%pobox.com@localhost> wrote:
> <...>
> 
> I'd like ether_output never, ever to deal with the routing table,
> but arpresolve() deals some in routes, so eventually I will have
> to factor that out.  I'm aiming for a streamlined IP transmission
> path.  Today we have something comparatively baroque,
> 
>         # enter ip_output
>         dst <- destination(packet)
>         nexthop <- lookup(ip routing table, dst)
>         # enter ether_output
>         if (nexthop is gateway)
>                 if (gateway is cached)
>                         nexthop <- gwcache(nexthop)
>                 else
>                         gwcache(nexthop) <- lookup(ip routing table,
>                                                    gwaddr(nexthop))
>                         nexthop <- gwcache(nexthop)
>         if (nexthop l2 info incomplete)
>                 defer_until_arp_complete(nexthop, packet)
>         else
>                 l2hdr <- create_l2header(interface(nexthop), gwaddr
> (nexthop)) transmit(interface(nexthop), l2hdr | packet)
> 
> I'm aiming for something more like this:
> 
>         # enter ip_output
>         dst <- destination(packet)
>         nexthop <- lookup(ip routing table, dst)
>         if (nexthop l2 info incomplete)
>                 defer_until_arp_complete(nexthop, packet)
>         else
>                 # enter ether_output
>                 transmit(interface(nexthop), l2header(nexthop), packet)
> 
> It will take several steps to get there.  Here is one.  A step in the
> wrong direction?  Your feedback, please. :-)

Seems reasonable direction to me.

Few small comments on the patch:

> +/*
> + * Send an IP packet to a host.
> + *
> + * If necessary, resolve the arbitrary IP route, rt0, to an IP host route 
> before
> + * calling ifp's output routine.
> + */
> +int
> +ip_hresolv_output(struct ifnet * const ifp, struct mbuf * const m,
> +    const struct sockaddr * const dst, struct rtentry *rt0)
> +{
> +#define senderr(e) { error = (e); goto bad;}

I do not think it is worth to macrify such error path; most of the code in
the kernel does not.  Actually, it might be worth to just leave the error
handling for the caller here.

> +     int error = 0;
> +     struct rtentry *rt;
> +
> +     if (!ip_hresolv_needed(ifp))
> +             return klock_if_output(ifp, m, dst, rt0);
> +
> +     if ((rt = rt0) == NULL)
> +             return klock_if_output(ifp, m, dst, NULL);
> +
> +     /* The following block is highly questionable.  How did we get
> here
> +      * with a !RTF_UP route?  Does rtalloc1() always return an RTF_UP
> +      * route?
> +      */

/*
 * KNF. :)
 */

> +     if ((rt->rt_flags & RTF_UP) == 0) {
> +             if ((rt0 = rt = rtalloc1(dst, 1)) == NULL)
> +                     senderr(EHOSTUNREACH);
> +             rt->rt_refcnt--;
> +             if (rt->rt_ifp != ifp)
> +                     return ip_hresolv_output(rt->rt_ifp, m, dst, rt);
> +     }

Recursive?  How about handling this in a loop?

> +static bool
> +ip_hresolv_needed(const struct ifnet * const ifp)
> +{
> +     switch (ifp->if_type) {
> +     case IFT_ARCNET:
> +     case IFT_ETHER:
> +     case IFT_FDDI:
> +     case IFT_IEEE1394:
> +             return 1;
> +     default:
> +             return 0;
> +     }
> +}

There are standard "true" and "false" for bool.

+static __attribute__((always_inline)) inline int
+klock_if_output(struct ifnet * const ifp, struct mbuf * const m,
+    const struct sockaddr * const dst, struct rtentry *rt)
+{
+       int error;
+
+       if (ifp->if_intern_txenqueue == NULL)
+               KERNEL_LOCK(1, NULL);
+
+       error = (*ifp->if_output)(ifp, m, dst, rt);

I do not think always-inline is worth here, seems like nano-optimising,
especially when this routine performs call on a function pointer.  Better
code would probably be generated by having:

const bool mpsafe = ifp->if_intern_txenqueue != NULL;

-- 
Mindaugas