On Dec 15, 2007 10:17 AM, Sam Leffler wrote:
>
> Robert Watson wrote:
> >
> > On Sat, 15 Dec 2007, Robert Watson wrote:
> >
> >> More comments to follow.

> >
> > Some more, back to tcp_offload.c which I didn't look at last time around:
> >
> > + ifp = rt->rt_ifp;
> > + tdev = TOEDEV(ifp);
> > + if (tdev == NULL)
> > + return (EINVAL);
> > +
> > + if (tdev->tod_can_offload(tdev, so) == 0)
> > + return (EINVAL);
> >
> > I sort of expected to see a if_capenable check for TOE here, but I
> > guess it doesn't make much difference if the flag is going to be
> > checked at the lower level. However, in the case of things like TCP
> > checksum offload, we do do the check at the higher level, so it
> > wouldn't be inconsistent to do that.
> >
> > BTW, could you prepare similar comments for toedev.h?
> >
> > +struct toe_usrreqs tcp_offload_usrreqs = {
> > + .tu_send = tcp_output,
> > + .tu_rcvd = tcp_output,
> > + .tu_disconnect = tcp_output,
> > + .tu_abort = tcp_output,
> > + .tu_detach = tcp_offload_detach_noop,
> > + .tu_syncache_event = tcp_offload_syncache_event_noop,
> > +};
> >
> > This structure seems to introduce quite a bit more indirection for
> > non-offloaded cases than before, especially to do things like this:
> >
> > +static void
> > +tcp_offload_syncache_event_noop(int event, void *toepcb)
> > +{
> > +}
> >
> > The compiler can't compile these out because it likely doesn't do much
> > in the way of function pointer analysis, and probably shouldn't.
> > However, it leaves quite a few code paths heavier weight than before.
> > I think I'd prefer a model in which a TF_OFFLOAD flag is set on the
> > tcpcb and then we conditionally invoke tu_foo as a result. I.e.,:
> >
> > static __inline int
> > tcp_gen_send(struct tcpcb *tp)
> > {
> >
> > #ifndef TCP_OFFLOAD_DISABLE
> > if (tcp->f_flag & TF_OFFLOADED)
> > return (tp->t_tu->tu_send(tp));
> > else
> > #endif
> > return (tcp_output(tp));
> > }
> >
> > This would compile to a straight call to tcp_output() when offloading
> > isn't compiled in, and when it is compiled in and offloading isn't
> > enabled, we do a simple flag check rather than invoking a function via
> > a series of pointers.

>
> I suggested Kip explore this technique as an alternative to having the
> inlines that check whether a socket is marked for offload or not
> (tradeoff indirect function call vs conditionals). My comment to him
> was that I find it can make code more intuitive. But with the common
> case being empty functions it's probably not a great option as the
> compiler cannot optimize it out.


Actually, in the datapath the common case is an indirect call to
tcp_output. The only empty function that is called is detach, and that
is only called once on shutdown.

-Kip
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/lis...reebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"