From owner-cvs-all@FreeBSD.ORG Tue Jul 3 20:11:31 2007 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 67DA416A475; Tue, 3 Jul 2007 20:11:31 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [209.31.154.42]) by mx1.freebsd.org (Postfix) with ESMTP id 33FAE13C46C; Tue, 3 Jul 2007 20:11:31 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [209.31.154.41]) by cyrus.watson.org (Postfix) with ESMTP id 79F5846B27; Tue, 3 Jul 2007 16:11:30 -0400 (EDT) Date: Tue, 3 Jul 2007 21:11:30 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Peter Jeremy In-Reply-To: <20070703195228.GP15680@turion.vk2pj.dyndns.org> Message-ID: <20070703210648.J29272@fledge.watson.org> References: <200707031014.l63AEE9Y026819@repoman.freebsd.org> <20070703195228.GP15680@turion.vk2pj.dyndns.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/sys socketvar.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Jul 2007 20:11:31 -0000 On Wed, 4 Jul 2007, Peter Jeremy wrote: > On 2007-Jul-03 10:14:13 +0000, Robert Watson wrote: >> Fix a bug in sblock() that has existed since revision 1.1 from BSD: >> correctly return an error if M_NOWAIT is passed to sblock() and the >> operation might block. This remarkably subtle macro bug appears to >> be responsible for quite a few undiagnosed socket buffer corruption >> and mbuf-related kernel panics. > > This bug would appear to be a classic example of the dangers of trying to > force force multiple C statements into a single statement. IMHO, it (and > many of the other macros in socketvar.h) should be inline functions, rather > than macros. Yes, I agree. In 7.x, the I/O serialization lock for socket buffers (sblock/sbunlock) is implemented via functions in uipc_sockbuf.c, and now wraps around the sx(9) primitive rather than being a custom lock implementation. This required adding signal interruption support to sx(9) and optimizing it heavily (thanks Attilio!). As a result of this change, not only is the code easier to read, but it's also quite a bit faster when contended, as hand-hacked msleep(9) locks tend to be pretty inefficient. We also no longer need to interlock the I/O serialization lock using the socket buffer mutex, so I/O serialization is now handled independently from maintaining data integrity for socket buffers via their mutex. I went with the less intrusive change in 6.x as this was the patch submitted by Isilon and tested by Yahoo!. It's pretty minimalist, avoids spreading type and function declaration dependencies all over the place (inlines are fully processed C code, unlike macros, even if unused), and is appropriate for an errata patch should that be desirable. Robert N M Watson Computer Laboratory University of Cambridge > > IMHO, the following is far more legible: > > static __inline int > sblock(struct sockbuf *sb, int wf) > { > if (sb->sb_flags & SB_LOCK) > return ((wf == M_WAITOK) ? sb_lock(sb) : EWOULDBLOCK); > else { > sb->sb_flags |= SB_LOCK; > return (0); > } > } > > -- > Peter Jeremy >