From: Jim Meyering <meyering@fb.com>
Date: Thu, 15 Jan 2015 03:54:27 +0000 (-0800)
Subject: MPMCQueue.h: fix a bug introduced while accommodating -Wsign-compare
X-Git-Tag: v0.23.0~45
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=1509cebb0407aee1ca0a381af6f737d53aa2325b;p=folly.git

MPMCQueue.h: fix a bug introduced while accommodating -Wsign-compare

Summary:
* folly/MPMCQueue.h (waitForTurn): My changes to accommodate
-Wsign-compare introduced a bug that could cause lots of wasted CPU.
(See @scottf's comments on the blamed patch.)
Subtraction involving an unsigned type will always yield an unsigned
result, and there was at least this one instance where the result
would sometimes have to be a small negative number.  Instead, we'd
get approximately UINT32_MAX.  The fix is to explicitly cast the
difference to a signed type.

Test Plan:
Run this and note there is no error:
(this ensures it remains -Wsign-compare-clean with gcc-4.9)

fbconfig --platform-all=gcc-4.9-glibc-2.20 tao/server && fbmake dbgo

Before this change, running the mpmc_queue_test test would get
timeout errors ~8 in 100 runs.  Confirm that with this patch, now
there is no type of failure. I.e., run these commands and ensure
the grep prints nothing:

fbconfig folly/test:mpmc_queue_test
for i in $(seq 100); do fbmake runtests_dbgo >& makerr-$i; done
grep -E '(TIMEOUT|FATAL|FAIL).* [1-9]' makerr-*

Reviewed By: hans@fb.com

Subscribers: trunkagent, scottf, folly-diffs@

FB internal diff: D1783672

Tasks: 5941250

Signature: t1:1783672:1421350260:d7d306e8cf3ed3ee636d06228e1f24a2c2796b54

Blame Revision: D1766722
---

diff --git a/folly/MPMCQueue.h b/folly/MPMCQueue.h
index 4be94269..9e3b88eb 100644
--- a/folly/MPMCQueue.h
+++ b/folly/MPMCQueue.h
@@ -664,8 +664,9 @@ struct TurnSequencer {
       } else {
         // try once, keep moving if CAS fails.  Exponential moving average
         // with alpha of 7/8
+        // Be careful that the quantity we add to prevThresh is signed.
         spinCutoff.compare_exchange_weak(
-            prevThresh, prevThresh + (target - prevThresh) / 8);
+            prevThresh, prevThresh + int(target - prevThresh) / 8);
       }
     }
   }