osd: replace ceph:atomic_t with std::atomic in osd module. #9138
Conversation
| @@ -1024,18 +1024,18 @@ class OSDService { | |||
| NOT_STOPPING, | |||
| PREPARING_TO_STOP, | |||
| STOPPING }; | |||
| atomic_t state; | |||
| atomic_uint state; | |||
|
@tchaikov please review again, thanks . |
|
I think @adamemerson or somebody in his group was working on a more comprehensive set of changes to our atomics. Maybe he has thoughts about how incrementally we want to handle this transition. |
|
I didn't have a plan to do a big replacement all at once, though I can if people like. It would have a portability advantage since all we'd depend upon is C++ support, rather than pulling in libatomic_ops (or falling back on the spinlock.) It would also open up the opportunity to use looser memory models where appropriate (though I would want to consult with the people more knowledgeable in any given subsystem before relaxing things.) |
|
@adamemerson @gregsfortytwo so what's the meaning ? is this PR meaningful ? I am also trying to stdanlize ceph code. |
|
@cxwshawn It means this is a step in the right direction, and nobody was working on it yet. :) |
|
retest this please |
| @@ -1241,32 +1241,32 @@ class OSD : public Dispatcher, | |||
| } | |||
|
|
|||
| private: | |||
| atomic_t state; | |||
| atomic_uint state; | |||
There was a problem hiding this comment.
should use atomic_int, as the state here is int actually .
|
@tchaikov 👍 , thanks for the review, fixed |
same work as ceph#9138 Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
same work as ceph#9138 Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
…c_atomic_replacement. same work as ceph#9138 Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
| @@ -796,7 +796,7 @@ class OSDService { | |||
|
|
|||
| private: | |||
| /// throttle promotion attempts | |||
| atomic_t promote_probability_millis; ///< probability thousands. one word. | |||
| atomic_uint promote_probability_millis; ///< probability thousands. one word. | |||
There was a problem hiding this comment.
i'd prefer if we were explicit about the std:: part, especially in headers - we shouldn't assume that other headers are using namespace std;
…c_atomic_replacement. same work as ceph#9138 Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
liewegas
changed the title
|
lgtm |
| @@ -796,7 +796,7 @@ class OSDService { | |||
|
|
|||
| private: | |||
| /// throttle promotion attempts | |||
| atomic_t promote_probability_millis; ///< probability thousands. one word. | |||
| std::atomic_uint promote_probability_millis; ///< probability thousands. one word. | |||
There was a problem hiding this comment.
Should be initialized here.
|
You seem to have missed several more initializations. We'll re-add needs-qa once those are fixed. |
|
@athanatos since the missed you said has been initialized by using constructor initialize list, so is there any need to reinitialize using xxx a{0} ? |
|
You are claiming that they are already initialized in the constructor list? I'd actually prefer you moved the initialization to the declaration like you did with the other variables. Are those not also initialized in the constructor? |
|
@athanatos yes, exactly, the former variables has been initialized in the constructor list, so I think there is no need to reinitialize, but since declaration initialization is preferred, I will change all the atomic variables to this form. |
…c_atomic_replacement. same work as ceph#9138 Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
|
@athanatos all modified atomic var has been initialized when declaration, please help merge, thanks! |
…c_atomic_replacement. same work as ceph#9138 Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
|
make check seems to be failing |
|
@athanatos it's a little weired, does this failure relate with this change ? |
|
@cxwshawn you might want to rebase your changeset against master for a green build. |
trying to clean up using std c++11 other than current project library. Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
|
@tchaikov @athanatos rebased, but still failed, is there any hint for this failure ? I cannot see any information related with this change. |
|
tested in http://pulpito.ceph.com/kchai-2016-07-10_07:48:29-rados-wip-kefu-testing---basic-mira/ the non-environmental failures are addressed by #10234 . |
trying to use std more, and atomic_t easier to use and no need
t implement more interfaces, like operator++,++ etc.
Signed-off-by: Xiaowei Chen chen.xiaowei@h3c.com