New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common,test: migrate atomic_t to std::atomic (varan) #14866

Merged
merged 4 commits into from Jun 8, 2017

Conversation

Projects
None yet
4 participants
@chardan
Contributor

chardan commented Apr 28, 2017

Refer-to: #14655

while (!stop_threads.read()) {
if(pause_threads.read()) {
while (!stop_threads) {
if(pause_threads) {

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

please add space after if

shardedpool_lock.Lock();
++num_paused;
wait_cond.Signal();
while(pause_threads.read()) {
while(pause_threads) {

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

please add space after while

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

@chardan could you address this comment also?

This comment has been minimized.

@chardan

chardan Apr 28, 2017

Contributor

Yes, it's in the latest commit. (Or so I thought-- fixing.)

static atomic64_t buffer_history_alloc_bytes;
static atomic64_t buffer_history_alloc_num;
static std::atomic<unsigned> buffer_total_alloc { 0 };
static std::atomic<int64_t> buffer_history_alloc_bytes { 0 };

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

could be std::atomic<uint64_t>

This comment has been minimized.

@chardan

chardan Apr 28, 2017

Contributor

Thanks for the feedback! We have several places where there's signed/unsigned confusion-- including a couple of unsigned t signed casts that you'll see elsewhere that fixing would require interfaces changes to. Should I try to address those while we're in here?

I'll move atomic<int64_t> to atomic<uint64_t> for at least the low-hanging fruit in the meantime. (Meanwhile, just trying to get all these things to build...)

static atomic64_t buffer_history_alloc_num;
static std::atomic<unsigned> buffer_total_alloc { 0 };
static std::atomic<int64_t> buffer_history_alloc_bytes { 0 };
static std::atomic<int64_t> buffer_history_alloc_num { 0 };

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

could be std::atomic<uint64_t>

}
}
/// Increase counter for given axis values by one
template <typename... T>
void inc(T... axis) {
auto index = get_raw_index_for_value(axis...);
m_rawData[index].add(1);
m_rawData[index] += 1;

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

could also use ++ operator

@liewegas

This comment has been minimized.

@branch-predictor

This comment has been minimized.

Member

branch-predictor commented Apr 28, 2017

In c029316 you're adding perf_historgam.h and in 3257ba5 you're reverting it. Can you remove these two changes from your pull request?

@branch-predictor

This comment has been minimized.

Member

branch-predictor commented May 9, 2017

Needs rebase.

@chardan chardan changed the title from common: migrate atomic_t to std::atomic (varan) to common,osdc,filestore,test,msg: migrate atomic_t to std::atomic (varan) May 12, 2017

@chardan chardan changed the title from common,osdc,filestore,test,msg: migrate atomic_t to std::atomic (varan) to common,test: migrate atomic_t to std::atomic (varan) May 23, 2017

}
}
return count.read();
return count;
>>>>>>> c857eae805... common: migrate atomic_t to <atomic>

This comment has been minimized.

@tchaikov

tchaikov May 23, 2017

Contributor

@chardan i don't think this compiles.

This comment has been minimized.

@chardan

chardan May 30, 2017

Contributor

Should now be addressed.

@chardan

This comment has been minimized.

Contributor

chardan commented May 23, 2017

Jenkins retest this please.

1 similar comment
@chardan

This comment has been minimized.

Contributor

chardan commented May 29, 2017

Jenkins retest this please.

@chardan

This comment has been minimized.

Contributor

chardan commented May 30, 2017

@@ -3,6 +3,11 @@
#include "Finisher.h"
#include "common/debug.h"
// re-include our assert to clobber the system one; fix dout:

This comment has been minimized.

@tchaikov

tchaikov May 31, 2017

Contributor

do you happen to know who is including cassert or assert.h? so the assert() in ceph is hidden? AFAICT, no headers in libstdc++ but cassert includes assert.h.

This comment has been minimized.

@chardan

chardan May 31, 2017

Contributor

AFAIK that's correct. I'm not super familiar with our dout()/assert() hackery at this time, other than that I get scads of compiler errors without this. Although "common/debug.h" looks like it should be including "include/assert.h", without including it afterwards, the above source file gives me an error when macro ldout is expanded.

This comment has been minimized.

@tchaikov

tchaikov Jun 6, 2017

Contributor

@chardan i admit this is a mess and we need to fix this in long term without the _ASSERT_H hackery .

but i am still just wondering why we need to add this in this change. but don't need before. is there any headers we are introducing break it?

also, you need to avoid the fix-of-a-fix commits in a single PR.

This comment has been minimized.

@chardan

chardan Jun 6, 2017

Contributor

@tchaikov I've removed references to "include/atomic.h", and I'd imagine that's the real cause, though unfortunately it's a bit hard for me to cleanly say if that's the "real reason"(TM), since early on I didn't have this correctly based of off master. More than happy if there are alternatives, but I'm not sure what those would be-- certainly possible that I'm doing something else wrong that's causing the issue, but I don't know what it would be.

This comment has been minimized.

@chardan

chardan Jun 6, 2017

Contributor

Mini-experiment:
I reverted Finisher.cc and Finisher.h to edbd6ed, and rebuilt; this causes dout complilation errors, absent with this patch. Unfortunately, I've had less luck tracking down the dependencies-- Cond.h is included by Finisher.h, which includes common/Mutex.h and via ceph_context.h atomic.h is included, so it doesn't seem like that is the cause after all. Suggestions welcomed.

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 1, 2017

needs rebase

chardan added some commits May 28, 2017

test: migrate atomic_t to atomic<>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
common: migrate atomic_t to <atomic>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
mgr: fixes compiler error because of atomic_t to <atomic>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
compile fixups for dout
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
@chardan

This comment has been minimized.

Contributor

chardan commented Jun 5, 2017

How are things looking? Anything further I can do?

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 7, 2017

@tchaikov ping

@tchaikov

okay. but we should have a better way to fix the assert() without including "common/debug.h" everywhere.

@tchaikov tchaikov merged commit c703459 into ceph:master Jun 8, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment