Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 15 Mar 2015 05:21:06 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Dimitry Andric <dim@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r279981 - in head: contrib/compiler-rt/lib/builtins lib/libcompiler_rt
Message-ID:  <20150315023356.X977@besplex.bde.org>
In-Reply-To: <201503141240.t2ECeKqP045674@svn.freebsd.org>
References:  <201503141240.t2ECeKqP045674@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 14 Mar 2015, Dimitry Andric wrote:

> Log:
>  =EF=BB=BFPull in r231965 from upstream compiler-rt trunk (by J=C3=B6rg S=
onnenberger):
>
>    Refactor float to integer conversion to share the same code.
>    80bit Intel/PPC long double is excluded due to lacking support
>    for the abstraction. Consistently provide saturation logic.
>    Extend to long double on 128bit IEEE extended platforms.

I hoped that this would fix a longstanding conversion bug, but that bug
is actually for integer to float conversion, and the conversion is inline.

clang can't even convert integer 0 to floating point correctly (when the
integer has type uintmax_t and is variable with value 0, and the rounding
mode is downwards):

X #include <fenv.h>
X #include <stdint.h>
X #include <stdio.h>
X=20
X int
X main(void)
X {
X =09volatile uintmax_t er =3D 0;
X=20
X =09fesetround(FE_DOWNWARD);
X =09printf("%.4f\n", (double)er);
X =09return (0);
X }

clang generates broken inline code giving a result of -0.0000.  It does
a magic conversion involving loading the variable as an integer, shuffling
bits, subtracting a double and adding a double.  The subtraction gives
-0.0 when the rounding mode is downwards.

gcc48 generates apparently-correct inline code.  It does a less magic but
slower conversion involving:
- for values not exceeding INT64_MAX, just cvtsi2sdq
- for values exceeding INT64_MAX, modify er to ((er >> 1) | (er & 1)),
   convert that using cvtsi2sdq, then double the result.

Does this commit fix the differences between the runtime calculations
and compile-time calculations for overflowing cases?  Saturation logic
should do this.  My old test programs (previously last tested in 2004)
show the differences.  Compilers produced much the same garbage in
1994, 2004 and 2015.  Before this commit, they do the following:
- gcc48 saturates at compile time.  Its runtime results are inconsistent
   except for some cases of converting negative values to unsigned:
   - generally, the result is 0x8000000000000000 in bits for 64-bit
     values and 0x80000000 for 32-bit values.  This is the corect x86
     value since it is what is generated on overflow by the hardware.
     Call it IOV.  gcc corrupts even this value:
   - overflowing u_long or long -> (u_long)IOV or (long)IOV, OK
   - overflowing u_long, positive value -> 0
   - overflowing u_long, negative value -> (u_long)IOV, OK, but it is weird
       that negative values overflow to a larger value than positive values=
=2E
   - this is with 64-bit longs on amd64.  For conversions to u_int and int,
     the results are the same (with the the 32-bit IOV), except for the
     weird last result.  Now:
   - overflowing u_int, negative value -> 0.  This is the one case for
     unsigned types where the runtime result is consistent with the compile
     time result.
- clang gives identical results.
gcc was much more inconsistent in 1994.  Its typical behaviour was to
handle u_int by storing an int64_t and discarding the top bits.  uint64_t
is harder to handle and was more broken.

The behaviour is undefined on overflow, so these bugs are less serious
than for converting 0.  I prefer traps on overflow.  Everyone is used
to integer division trapping when the result would be infinite.  x86
hardware makes the trap for this impossible to avoid (but the kernel
could handle the trap and produce an in-band error result like IOV).
Converting FP infinity to integer should do the same.  Unfortunately,
x86 hardware make this trap hard to get efficiently (traps for it
would have to be enabled; then the kernel could produce a signal for
the integer case and emulate the hardware trap handling for the FP
case).

Bruce
From owner-svn-src-head@FreeBSD.ORG  Sat Mar 14 21:15:46 2015
Return-Path: <owner-svn-src-head@FreeBSD.ORG>
Delivered-To: svn-src-head@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org
 [IPv6:2001:1900:2254:206a::19:1])
 (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
 (No client certificate requested)
 by hub.freebsd.org (Postfix) with ESMTPS id 6E14B6BB;
 Sat, 14 Mar 2015 21:15:46 +0000 (UTC)
Received: from svn.freebsd.org (svn.freebsd.org
 [IPv6:2001:1900:2254:2068::e6a:0])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (Client did not present a certificate)
 by mx1.freebsd.org (Postfix) with ESMTPS id 5958D1AA;
 Sat, 14 Mar 2015 21:15:46 +0000 (UTC)
Received: from svn.freebsd.org ([127.0.1.70])
 by svn.freebsd.org (8.14.9/8.14.9) with ESMTP id t2ELFk5R092832;
 Sat, 14 Mar 2015 21:15:46 GMT (envelope-from mav@FreeBSD.org)
Received: (from mav@localhost)
 by svn.freebsd.org (8.14.9/8.14.9/Submit) id t2ELFkWP092831;
 Sat, 14 Mar 2015 21:15:46 GMT (envelope-from mav@FreeBSD.org)
Message-Id: <201503142115.t2ELFkWP092831@svn.freebsd.org>
X-Authentication-Warning: svn.freebsd.org: mav set sender to mav@FreeBSD.org
 using -f
From: Alexander Motin <mav@FreeBSD.org>
Date: Sat, 14 Mar 2015 21:15:46 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
 svn-src-head@freebsd.org
Subject: svn commit: r280004 - head/usr.sbin/bhyve
X-SVN-Group: head
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-BeenThere: svn-src-head@freebsd.org
X-Mailman-Version: 2.1.18-1
Precedence: list
List-Id: SVN commit messages for the src tree for head/-current
 <svn-src-head.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/svn-src-head>,
 <mailto:svn-src-head-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-head/>;
List-Post: <mailto:svn-src-head@freebsd.org>
List-Help: <mailto:svn-src-head-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-head>,
 <mailto:svn-src-head-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 14 Mar 2015 21:15:46 -0000

Author: mav
Date: Sat Mar 14 21:15:45 2015
New Revision: 280004
URL: https://svnweb.freebsd.org/changeset/base/280004

Log:
  Give block I/O interface multiple (8) execution threads.
  
  On parallel random I/O this allows better utilize wide storage pools.
  To not confuse prefetcher on linear I/O, consecutive requests are executed
  sequentially, following the same logic as was earlier implemented in CTL.
  
  Benchmarks of virtual AHCI disk, backed by ZVOL on RAID10 pool of 4 HDDs,
  show ~3.5 times random read performance improvements, while no degradation
  on linear I/O.
  
  MFC after:	2 weeks

Modified:
  head/usr.sbin/bhyve/block_if.c

Modified: head/usr.sbin/bhyve/block_if.c
==============================================================================
--- head/usr.sbin/bhyve/block_if.c	Sat Mar 14 21:07:37 2015	(r280003)
+++ head/usr.sbin/bhyve/block_if.c	Sat Mar 14 21:15:45 2015	(r280004)
@@ -55,6 +55,7 @@ __FBSDID("$FreeBSD$");
 #define BLOCKIF_SIG	0xb109b109
 
 #define BLOCKIF_MAXREQ	33
+#define BLOCKIF_NUMTHR	8
 
 enum blockop {
 	BOP_READ,
@@ -65,6 +66,7 @@ enum blockop {
 
 enum blockstat {
 	BST_FREE,
+	BST_BLOCK,
 	BST_PEND,
 	BST_BUSY,
 	BST_DONE
@@ -76,6 +78,7 @@ struct blockif_elem {
 	enum blockop	     be_op;
 	enum blockstat	     be_status;
 	pthread_t            be_tid;
+	off_t		     be_block;
 };
 
 struct blockif_ctxt {
@@ -88,16 +91,15 @@ struct blockif_ctxt {
 	int			bc_sectsz;
 	int			bc_psectsz;
 	int			bc_psectoff;
-	pthread_t		bc_btid;
+	int			bc_closing;
+	pthread_t		bc_btid[BLOCKIF_NUMTHR];
         pthread_mutex_t		bc_mtx;
         pthread_cond_t		bc_cond;
-	int			bc_closing;
 
 	/* Request elements and free/pending/busy queues */
 	TAILQ_HEAD(, blockif_elem) bc_freeq;       
 	TAILQ_HEAD(, blockif_elem) bc_pendq;
 	TAILQ_HEAD(, blockif_elem) bc_busyq;
-	u_int			bc_req_count;
 	struct blockif_elem	bc_reqs[BLOCKIF_MAXREQ];
 };
 
@@ -116,58 +118,83 @@ static int
 blockif_enqueue(struct blockif_ctxt *bc, struct blockif_req *breq,
 		enum blockop op)
 {
-	struct blockif_elem *be;
-
-	assert(bc->bc_req_count < BLOCKIF_MAXREQ);
+	struct blockif_elem *be, *tbe;
+	off_t off;
+	int i;
 
 	be = TAILQ_FIRST(&bc->bc_freeq);
 	assert(be != NULL);
 	assert(be->be_status == BST_FREE);
-
 	TAILQ_REMOVE(&bc->bc_freeq, be, be_link);
-	be->be_status = BST_PEND;
 	be->be_req = breq;
 	be->be_op = op;
+	switch (op) {
+	case BOP_READ:
+	case BOP_WRITE:
+	case BOP_DELETE:
+		off = breq->br_offset;
+		for (i = 0; i < breq->br_iovcnt; i++)
+			off += breq->br_iov[i].iov_len;
+		break;
+	default:
+		off = OFF_MAX;
+	}
+	be->be_block = off;
+	TAILQ_FOREACH(tbe, &bc->bc_pendq, be_link) {
+		if (tbe->be_block == breq->br_offset)
+			break;
+	}
+	if (tbe == NULL) {
+		TAILQ_FOREACH(tbe, &bc->bc_busyq, be_link) {
+			if (tbe->be_block == breq->br_offset)
+				break;
+		}
+	}
+	if (tbe == NULL)
+		be->be_status = BST_PEND;
+	else
+		be->be_status = BST_BLOCK;
 	TAILQ_INSERT_TAIL(&bc->bc_pendq, be, be_link);
-
-	bc->bc_req_count++;
-
-	return (0);
+	return (be->be_status == BST_PEND);
 }
 
 static int
-blockif_dequeue(struct blockif_ctxt *bc, struct blockif_elem **bep)
+blockif_dequeue(struct blockif_ctxt *bc, pthread_t t, struct blockif_elem **bep)
 {
 	struct blockif_elem *be;
 
-	if (bc->bc_req_count == 0)
-		return (ENOENT);
-
-	be = TAILQ_FIRST(&bc->bc_pendq);
-	assert(be != NULL);
-	assert(be->be_status == BST_PEND);
+	TAILQ_FOREACH(be, &bc->bc_pendq, be_link) {
+		if (be->be_status == BST_PEND)
+			break;
+		assert(be->be_status == BST_BLOCK);
+	}
+	if (be == NULL)
+		return (0);
 	TAILQ_REMOVE(&bc->bc_pendq, be, be_link);
 	be->be_status = BST_BUSY;
-	be->be_tid = bc->bc_btid;
+	be->be_tid = t;
 	TAILQ_INSERT_TAIL(&bc->bc_busyq, be, be_link);
-
 	*bep = be;
-
-	return (0);
+	return (1);
 }
 
 static void
 blockif_complete(struct blockif_ctxt *bc, struct blockif_elem *be)
 {
-	assert(be->be_status == BST_DONE);
+	struct blockif_elem *tbe;
 
-	TAILQ_REMOVE(&bc->bc_busyq, be, be_link);
+	if (be->be_status == BST_DONE || be->be_status == BST_BUSY)
+		TAILQ_REMOVE(&bc->bc_busyq, be, be_link);
+	else
+		TAILQ_REMOVE(&bc->bc_pendq, be, be_link);
+	TAILQ_FOREACH(tbe, &bc->bc_pendq, be_link) {
+		if (tbe->be_req->br_offset == be->be_block)
+			tbe->be_status = BST_PEND;
+	}
 	be->be_tid = 0;
 	be->be_status = BST_FREE;
 	be->be_req = NULL;
 	TAILQ_INSERT_TAIL(&bc->bc_freeq, be, be_link);
-
-	bc->bc_req_count--;
 }
 
 static void
@@ -226,28 +253,27 @@ blockif_thr(void *arg)
 {
 	struct blockif_ctxt *bc;
 	struct blockif_elem *be;
+	pthread_t t;
 
 	bc = arg;
+	t = pthread_self();
 
+	pthread_mutex_lock(&bc->bc_mtx);
 	for (;;) {
-		pthread_mutex_lock(&bc->bc_mtx);
-		while (!blockif_dequeue(bc, &be)) {
+		while (blockif_dequeue(bc, t, &be)) {
 			pthread_mutex_unlock(&bc->bc_mtx);
 			blockif_proc(bc, be);
 			pthread_mutex_lock(&bc->bc_mtx);
 			blockif_complete(bc, be);
 		}
-		pthread_cond_wait(&bc->bc_cond, &bc->bc_mtx);
-		pthread_mutex_unlock(&bc->bc_mtx);
-
-		/*
-		 * Check ctxt status here to see if exit requested
-		 */
+		/* Check ctxt status here to see if exit requested */
 		if (bc->bc_closing)
-			pthread_exit(NULL);
+			break;
+		pthread_cond_wait(&bc->bc_cond, &bc->bc_mtx);
 	}
+	pthread_mutex_unlock(&bc->bc_mtx);
 
-	/* Not reached */
+	pthread_exit(NULL);
 	return (NULL);
 }
 
@@ -386,16 +412,16 @@ blockif_open(const char *optstr, const c
 	TAILQ_INIT(&bc->bc_freeq);
 	TAILQ_INIT(&bc->bc_pendq);
 	TAILQ_INIT(&bc->bc_busyq);
-	bc->bc_req_count = 0;
 	for (i = 0; i < BLOCKIF_MAXREQ; i++) {
 		bc->bc_reqs[i].be_status = BST_FREE;
 		TAILQ_INSERT_HEAD(&bc->bc_freeq, &bc->bc_reqs[i], be_link);
 	}
 
-	pthread_create(&bc->bc_btid, NULL, blockif_thr, bc);
-
-	snprintf(tname, sizeof(tname), "blk-%s", ident);
-	pthread_set_name_np(bc->bc_btid, tname);
+	for (i = 0; i < BLOCKIF_NUMTHR; i++) {
+		pthread_create(&bc->bc_btid[i], NULL, blockif_thr, bc);
+		snprintf(tname, sizeof(tname), "blk-%s-%d", ident, i);
+		pthread_set_name_np(bc->bc_btid[i], tname);
+	}
 
 	return (bc);
 }
@@ -409,13 +435,13 @@ blockif_request(struct blockif_ctxt *bc,
 	err = 0;
 
 	pthread_mutex_lock(&bc->bc_mtx);
-	if (bc->bc_req_count < BLOCKIF_MAXREQ) {
+	if (!TAILQ_EMPTY(&bc->bc_freeq)) {
 		/*
 		 * Enqueue and inform the block i/o thread
 		 * that there is work available
 		 */
-		blockif_enqueue(bc, breq, op);
-		pthread_cond_signal(&bc->bc_cond);
+		if (blockif_enqueue(bc, breq, op))
+			pthread_cond_signal(&bc->bc_cond);
 	} else {
 		/*
 		 * Callers are not allowed to enqueue more than
@@ -481,11 +507,7 @@ blockif_cancel(struct blockif_ctxt *bc, 
 		/*
 		 * Found it.
 		 */
-		TAILQ_REMOVE(&bc->bc_pendq, be, be_link);
-		be->be_status = BST_FREE;
-		be->be_req = NULL;
-		TAILQ_INSERT_TAIL(&bc->bc_freeq, be, be_link);
-		bc->bc_req_count--;
+		blockif_complete(bc, be);
 		pthread_mutex_unlock(&bc->bc_mtx);
 
 		return (0);
@@ -546,7 +568,7 @@ int
 blockif_close(struct blockif_ctxt *bc)
 {
 	void *jval;
-	int err;
+	int err, i;
 
 	err = 0;
 
@@ -556,8 +578,9 @@ blockif_close(struct blockif_ctxt *bc)
 	 * Stop the block i/o thread
 	 */
 	bc->bc_closing = 1;
-	pthread_cond_signal(&bc->bc_cond);
-	pthread_join(bc->bc_btid, &jval);
+	pthread_cond_broadcast(&bc->bc_cond);
+	for (i = 0; i < BLOCKIF_NUMTHR; i++)
+		pthread_join(bc->bc_btid[i], &jval);
 
 	/* XXX Cancel queued i/o's ??? */
 



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