Date: Sat, 15 Dec 2007 10:19:45 -0800 From: "Kip Macy" <kip.macy@gmail.com> To: "Sam Leffler" <sam@errno.com> Cc: FreeBSD Current <freebsd-current@freebsd.org>, Robert Watson <rwatson@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: pending changes for TOE support Message-ID: <b1fa29170712151019k1bf93aa3j4b36c4eb95348a57@mail.gmail.com> In-Reply-To: <47641A4E.5050106@errno.com> References: <b1fa29170712121303x537fd11fj4b8827bb353ad8e4@mail.gmail.com> <b1fa29170712150057m690bd36bm7a1910969e92293b@mail.gmail.com> <20071215100351.Q70617@fledge.watson.org> <20071215152959.V85668@fledge.watson.org> <47641A4E.5050106@errno.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 15, 2007 10:17 AM, Sam Leffler <sam@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b1fa29170712151019k1bf93aa3j4b36c4eb95348a57>