tech-net archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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



On Sat, Feb 02, 2013 at 01:17:48AM +0000, Mindaugas Rasiukevicius wrote:
> 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.

Yeah, I don't think we should any new macros like this.  This one came
with the code I was refactoring.

> /*
>  * KNF. :)
>  */

Heh.  I like to save the vertical space, but KNF comment form has a
pleasing symmetry. :-)

> > +   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?

What guarantees that the loop completes---or the recursion, for that
matter? :-) I hope that I can just delete this code: if at first we get
an rtentry that's !RTF_UP, what good is it to try again?

> > +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.

Thanks, I missed that.  I copied & pasted & edited nd6_need_cache().

> +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;

Good point.  I'd always_inline'd it before it five lines long.  It's
unbelievable what GCC will neglect to inline.

Dave

-- 
David Young
dyoung%pobox.com@localhost    Urbana, IL    (217) 721-9981


Home | Main Index | Thread Index | Old Index