## Bug #25211

### bug in PerfCounters

Start date:

08/01/2018

Due date:

% Done:

0%

Source:

Tags:

Backport:

Regression:

No

Severity:

3 - minor

Reviewed:

Affected Versions:

ceph-qa-suite:

Component(RADOS):

OSD

Pull request ID:

**Description**

when we call PerfCounters::inc() and read_avg() at the same time, maybe the result is not what we want.

show the code here

```
void PerfCounters::inc(int idx, uint64_t amt)
{
if (!m_cct->_conf->perf)
return;
assert(idx > m_lower_bound);
assert(idx < m_upper_bound);
perf_counter_data_any_d& data(m_data[idx - m_lower_bound - 1]);
if (!(data.type & PERFCOUNTER_U64))
return;
if (data.type & PERFCOUNTER_LONGRUNAVG) {
data.avgcount.inc();
data.u64.add(amt);
data.avgcount2.inc();
} else {
data.u64.add(amt);
}
}
```

```
/// read <sum, count> safely
pair<uint64_t,uint64_t> read_avg() const {
uint64_t sum, count;
do {
count = avgcount.read();
sum = u64.read();
} while (avgcount2.read() != count);
return make_pair(sum, count);
}
```

ok, here is the problem, if we have two thread:

assume data.u64 = s; data.avgcount = data.avgcount2

thread1 : read | thread2: inc(v) | count = (avgcount=n) | | avgcount = (n+1) | u64 = (s+v) sum = (s+v) | count == avgcount2 == n | | avgcount2 = (n+1) \/

we get (s+v, n), but correct result is (s+v, n+1)

same problem:

thread1 : read_avg() thread2: inc(v) | avgcount = (n+1) | count = (avgcount=(n+1)) | sum = (data.u64 = s) | | data.u64 = (s+v) | avgcount2 = (n+1) count == avgcount2 == n+1 | \/

we get (s, n+1), but correct result is (s+v, n+1)

we should swap the position of avgcount and avgcount2:

```
/// read <sum, count> safely
pair<uint64_t,uint64_t> read_avg() const {
uint64_t sum, count;
do {
count = avgcount2.read(); // read avgcount2 first instead of avgcount
sum = u64.read();
} while (avgcount.read() != count);
return make_pair(sum, count);
}
```

### History

#### #1 Updated by hongpeng lu about 1 year ago

#### #2 Updated by Josh Durgin about 1 year ago

**Status**changed from*New*to*Need Review*

#### #3 Updated by Kefu Chai about 1 year ago

**Description**updated (diff)

#### #4 Updated by Kefu Chai about 1 year ago

**Assignee**set to*hongpeng lu*