Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 3 Jul 2007 21:11:30 +0100 (BST)
From:      Robert Watson <rwatson@FreeBSD.org>
To:        Peter Jeremy <peterjeremy@optushome.com.au>
Cc:        cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org
Subject:   Re: cvs commit: src/sys/sys socketvar.h
Message-ID:  <20070703210648.J29272@fledge.watson.org>
In-Reply-To: <20070703195228.GP15680@turion.vk2pj.dyndns.org>
References:  <200707031014.l63AEE9Y026819@repoman.freebsd.org> <20070703195228.GP15680@turion.vk2pj.dyndns.org>

next in thread | previous in thread | raw e-mail | index | archive | help

On Wed, 4 Jul 2007, Peter Jeremy wrote:

> On 2007-Jul-03 10:14:13 +0000, Robert Watson <rwatson@freebsd.org> 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
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070703210648.J29272>