From owner-freebsd-arch@FreeBSD.ORG Sat Dec 15 18:19:46 2007 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8672D16A5BB for ; Sat, 15 Dec 2007 18:19:46 +0000 (UTC) (envelope-from kip.macy@gmail.com) Received: from wa-out-1112.google.com (wa-out-1112.google.com [209.85.146.180]) by mx1.freebsd.org (Postfix) with ESMTP id 39F1413C46B for ; Sat, 15 Dec 2007 18:19:46 +0000 (UTC) (envelope-from kip.macy@gmail.com) Received: by wa-out-1112.google.com with SMTP id k17so2248973waf.3 for ; Sat, 15 Dec 2007 10:19:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; bh=m9RV1o99QgVaGtoRcEeT0SezTSp5AN5N7JH5VNAJPY4=; b=rq1E3UODOuy2Gr+0EkvZVKHblSVb6g+nTHhtUhsNOZ5IEVIgpK+xwcN9HkvMsrchyNVynnBDsQqMsdBkpp+1Xz5ocR9Yy24Qycikkt1Tku5vjUsBHSOgtxi8esDTAXpZai+/tBrpthilFWOPYh5DPACFeunFYOopv8bW4nG7xSU= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=lmTscsRcgdIJvX4uMX0qMvV5ZbFVkp8OGdrE7qu761PATOVEFQ0u4PevGt5lo0JuaREPvNgA+Y0gPY7uiKeUXrZ5dpIWZxuTK+gDLQduJOBGay+LBXssbMaLRK8idoaTiSdbjge91ltW1wOafb2cBGytTj3uUqUiB2WAfNVNkYc= Received: by 10.115.92.2 with SMTP id u2mr930461wal.139.1197742785662; Sat, 15 Dec 2007 10:19:45 -0800 (PST) Received: by 10.114.255.11 with HTTP; Sat, 15 Dec 2007 10:19:45 -0800 (PST) Message-ID: Date: Sat, 15 Dec 2007 10:19:45 -0800 From: "Kip Macy" To: "Sam Leffler" In-Reply-To: <47641A4E.5050106@errno.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20071215100351.Q70617@fledge.watson.org> <20071215152959.V85668@fledge.watson.org> <47641A4E.5050106@errno.com> Cc: FreeBSD Current , Robert Watson , freebsd-arch@freebsd.org Subject: Re: pending changes for TOE support X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 15 Dec 2007 18:19:46 -0000 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