util/timeutil: fix stopwatch for concurrent usage

Previously, it was possible to run into a race when manipulating the
stop watch from one goroutine but accessing the elapsed time from
another goroutine. Thus, the promise of "concurrency safe" stop watch
was broken. Previously only calling Start/Stop was concurrency-safe, and
now Elapsed is safe too. I'll add a unit test in a follow-up commit.

Release note: None

pkg/util/timeutil/stopwatch.go

@@ -11,24 +11,28 @@
 package timeutil
 
 import (
-	"sync/atomic"
 	"time"
+
+	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
 
 // StopWatch is a utility stop watch that can be safely started and stopped
 // multiple times and can be used concurrently.
 type StopWatch struct {
-	// started is used as boolean where 0 indicates false and 1 indicates true.
-	// started is "true" if the stop watch has been started.
-	started int32
-	// startedAt is the time when the stop watch was started.
-	startedAt time.Time
-	// elapsed is the total time measured by the stop watch (i.e. between all
-	// Starts and Stops).
-	elapsed time.Duration
-	// timeSource is the source of time used by the stop watch. It is always
-	// timeutil.Now except for tests.
-	timeSource func() time.Time
+	mu struct {
+		syncutil.Mutex
+		// started is true if the stop watch has been started and haven't been
+		// stopped after that.
+		started bool
+		// startedAt is the time when the stop watch was started.
+		startedAt time.Time
+		// elapsed is the total time measured by the stop watch (i.e. between
+		// all Starts and Stops).
+		elapsed time.Duration
+		// timeSource is the source of time used by the stop watch. It is always
+		// timeutil.Now except for tests.
+		timeSource func() time.Time
+	}
 }
 
 // NewStopWatch creates a new StopWatch.
@@ -43,28 +47,38 @@ func NewTestStopWatch(timeSource func() time.Time) *StopWatch {
 }
 
 func newStopWatch(timeSource func() time.Time) *StopWatch {
-	return &StopWatch{timeSource: timeSource}
+	w := &StopWatch{}
+	w.mu.timeSource = timeSource
+	return w
 }
 
 // Start starts the stop watch if it hasn't already been started.
 func (w *StopWatch) Start() {
-	if atomic.CompareAndSwapInt32(&w.started, 0, 1) {
-		w.startedAt = w.timeSource()
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	if !w.mu.started {
+		w.mu.started = true
+		w.mu.startedAt = w.mu.timeSource()
 	}
 }
 
 // Stop stops the stop watch if it hasn't already been stopped and accumulates
 // the duration that elapsed since it was started. If the stop watch has
 // already been stopped, it is a noop.
 func (w *StopWatch) Stop() {
-	if atomic.CompareAndSwapInt32(&w.started, 1, 0) {
-		w.elapsed += w.timeSource().Sub(w.startedAt)
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	if w.mu.started {
+		w.mu.started = false
+		w.mu.elapsed += w.mu.timeSource().Sub(w.mu.startedAt)
 	}
 }
 
 // Elapsed returns the total time measured by the stop watch so far.
 func (w *StopWatch) Elapsed() time.Duration {
-	return w.elapsed
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	return w.mu.elapsed
 }
 
 // TestTimeSource is a source of time that remembers when it was created (in