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