From owner-svn-src-all@freebsd.org Fri Jun 24 11:26:49 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B79E8B73CEF; Fri, 24 Jun 2016 11:26:49 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 4320B1720; Fri, 24 Jun 2016 11:26:48 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 390901044DF1; Fri, 24 Jun 2016 21:26:39 +1000 (AEST) Date: Fri, 24 Jun 2016 21:26:39 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r301932 - head/sys/dev/cxgbe/tom In-Reply-To: <10550858.Wqzuf5sXBE@ralph.baldwin.cx> Message-ID: <20160624210041.J1013@besplex.bde.org> References: <201606152108.u5FL8pcO012174@repo.freebsd.org> <10550858.Wqzuf5sXBE@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=OtmysHLt c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=WQYTAzPHRSsGqI3LFlkA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jun 2016 11:26:49 -0000 On Wed, 15 Jun 2016, John Baldwin wrote: > On Wednesday, June 15, 2016 09:08:51 PM John Baldwin wrote: >> Author: jhb >> Date: Wed Jun 15 21:08:51 2016 >> New Revision: 301932 >> URL: https://svnweb.freebsd.org/changeset/base/301932 >> >> Log: >> Use sbused() instead of sbspace() to avoid signed issues. This description is sort of backwards. sbused() is unsigned (u_int), so using it gives (un)sign extension bugs. sbspace() has complications to return signed (long) to avoid (un)sign extension bugs. It has to be careful to avoid (un)sign extension bugs internally (intermediate versions of it were broken by removing this care when converting it from a macro to an inline function). The changed code also mostly uses signed types, but ... >> Inserting a full mbuf with an external cluster into the socket buffer >> resulted in sbspace() returning -MLEN. However, since sb_hiwat is >> unsigned, the -MLEN value was converted to unsigned in comparisons. As a >> result, the socket buffer was never autosized. Note that sb_lowat is signed >> to permit direct comparisons with sbspace(), but sb_hiwat is unsigned. >> Follow suit with what tcp_output() does and compare the value of sbused() >> with sb_hiwat instead. ... direct comparisons with sb_hiwat gives the (un)sign extension bugs since its foor-shooting type is exposed. > Amusingly (or not), sb_lowat used to be signed as well. Mike Karels > changed it to signed in this commit to BSD: > > https://svnweb.freebsd.org/csrg/sys/sys/socketvar.h?revision=43896 > > The log reads: > > add SB_ASYNC in sockbuf, add SB_NOTIFY, SB_NOINTR; > make lowat signed for comparison with sbspace (should probably give up > and make all fields signed They shoyld all have been signed to begin with. X Modified: head/sys/dev/cxgbe/tom/t4_cpl_io.c X ============================================================================== X --- head/sys/dev/cxgbe/tom/t4_cpl_io.c Wed Jun 15 21:01:53 2016 (r301931) X +++ head/sys/dev/cxgbe/tom/t4_cpl_io.c Wed Jun 15 21:08:51 2016 (r301932) X @@ -577,7 +577,7 @@ t4_push_frames(struct adapter *sc, struc X struct tcpcb *tp = intotcpcb(inp); X struct socket *so = inp->inp_socket; X struct sockbuf *sb = &so->so_snd; X - int tx_credits, shove, compl, space, sowwakeup; X + int tx_credits, shove, compl, sowwakeup; X struct ofld_tx_sdesc *txsd = &toep->txsd[toep->txsd_pidx]; X X INP_WLOCK_ASSERT(inp); X @@ -652,9 +652,7 @@ t4_push_frames(struct adapter *sc, struc X } X } X X - space = sbspace(sb); Here 'space' was signed (int) to hold sbspace(), but this was still a type mismatch since sbspace() returns signed long. X - X - if (space <= sb->sb_hiwat * 3 / 8 && Then this broke because the unsigned sb_hiwat is too hard to use. X + if (sbused(sb) > sb->sb_hiwat * 5 / 8 && Is this conversion really correct? sbspace(sb) is only sb->sb_hiwat - sbused(sb) in the usual case. For a direct translation, convert sb_hiwat * 3 / 8 to int (it is sure to fit). Bruce