Re: pending changes for TOE support

On Dec 15, 2007 10:17 AM, Sam Leffler <sam_at_errno.com> 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