[Xorp-hackers] Patch to get rid of two system calls per asyncio send.

[Pavlin Radoslavov]

Ben Greear <greearb at candelatech.com> wrote:

> Asyncio was disabling and enabling SIGPIPE for each send.  At least on Linux
> (and probably BSD), we can use MSG_NOSIGNAL in most cases.  Attached is a patch
> that implements this.  Not specifically benchmarked, but it's always good to
> get rid of
> extra system calls...

I agree that we should get rid of extra system calls.
However, this part of the code is very critical and we want to be
very careful with it (e.g., it has been changed by a number of
people in the past and it might be quite fragile).
Said that, please add it to Bugzilla.

Thanks,
Pavlin

> Thanks,
> Ben
> 
> -- 
> Ben Greear <greearb at candelatech.com> Candela Technologies Inc
> http://www.candelatech.com
> 
> 
> Index: asyncio.cc
> ===================================================================
> RCS file: /cvs/xorp/libxorp/asyncio.cc,v
> retrieving revision 1.40
> diff -u -r1.40 asyncio.cc
> --- asyncio.cc	4 Jan 2008 03:16:32 -0000	1.40
> +++ asyncio.cc	20 Mar 2008 07:23:56 -0000
> @@ -205,8 +205,12 @@
>      _last_error = 0;
>      done = ::read(_fd, head->buffer() + head->offset(),
>  		  head->buffer_bytes() - head->offset());
> -    if (done < 0)
> +    if (done < 0) {
>  	_last_error = errno;
> +	XLOG_WARNING("read error: _fd: %i  offset: %i  total-len: %i error: %s\n",
> +		     (int)(_fd), head->offset(), head->buffer_bytes(),
> +		     strerror(errno));
> +    }
>      errno = 0;
>  #endif // ! HOST_OS_WINDOWS
>  
> @@ -571,8 +575,10 @@
>  	XLOG_ASSERT(! dst_addr.is_zero());
>  
>  #ifndef HOST_OS_WINDOWS
> +#ifndef MSG_NOSIGNAL // save two system calls if MSG_NOSIGNAL is supported.
>  	sig_t saved_sigpipe = signal(SIGPIPE, SIG_IGN);
>  #endif
> +#endif
>  
>  	switch (dst_addr.af()) {
>  	case AF_INET:
> @@ -584,7 +590,11 @@
>  
>  	    done = ::sendto(_fd, XORP_CONST_BUF_CAST(_iov[0].iov_base),
>  			    _iov[0].iov_len,
> +#ifdef MSG_NOSIGNAL
> +			    MSG_NOSIGNAL,
> +#else
>  			    0,
> +#endif
>  			    reinterpret_cast<const sockaddr*>(&sin),
>  			    sizeof(sin));
>  	    break;
> @@ -599,7 +609,11 @@
>  
>  	    done = ::sendto(_fd, XORP_CONST_BUF_CAST(_iov[0].iov_base),
>  			    _iov[0].iov_len,
> +#ifdef MSG_NOSIGNAL
> +			    MSG_NOSIGNAL,
> +#else
>  			    0,
> +#endif
>  			    reinterpret_cast<const sockaddr*>(&sin6),
>  			    sizeof(sin6));
>  	    break;
> @@ -620,8 +634,10 @@
>  	}
>  
>  #ifndef HOST_OS_WINDOWS
> +#ifndef MSG_NOSIGNAL
>  	signal(SIGPIPE, saved_sigpipe);
>  #endif
> +#endif
>  
>      } else {
>  	//
> @@ -654,16 +670,34 @@
>  	    _last_error = (result == FALSE) ? GetLastError() : 0;
>  	}
>  #else // ! HOST_OS_WINDOWS
> -	sig_t saved_sigpipe = signal(SIGPIPE, SIG_IGN);
>  
> -	errno = 0;
> -	_last_error = 0;
> -	done = ::writev(_fd, _iov, (int)iov_cnt);
> -	if (done < 0)
> -	    _last_error = errno;
> -	errno = 0;
> +#ifdef MSG_NOSIGNAL
> +	if (iov_cnt == 1) {
> +	    // No need for coelesce, so use send.  This saves us the two
> +	    // sigaction calls since we can pass the MSG_NOSIGNAL flag.
> +	    errno = 0;
> +	    _last_error = 0;
> +	    done = ::send(_fd, XORP_CONST_BUF_CAST(_iov[0].iov_base),
> +			  _iov[0].iov_len, MSG_NOSIGNAL);
> +	    if (done < 0)
> +		_last_error = errno;
> +	    errno = 0;
> +	}
> +	else {
> +#endif
> +	    sig_t saved_sigpipe = signal(SIGPIPE, SIG_IGN);
>  
> -	signal(SIGPIPE, saved_sigpipe);
> +	    errno = 0;
> +	    _last_error = 0;
> +	    done = ::writev(_fd, _iov, (int)iov_cnt);
> +	    if (done < 0)
> +		_last_error = errno;
> +	    errno = 0;
> +
> +	    signal(SIGPIPE, saved_sigpipe);
> +#ifdef HOST_OS_LINUX
> +	}
> +#endif
>  #endif // ! HOST_OS_WINDOWS
>      }
>  
> _______________________________________________
> Xorp-hackers mailing list
> Xorp-hackers at icir.org
> http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers