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

src/common: change last_work_queue to next_work_queue. #14738

Merged
merged 1 commit into from Apr 25, 2017

Conversation

Projects
None yet
4 participants
@liupan1111
Contributor

liupan1111 commented Apr 23, 2017

In current ceph code, last_work_queue(0) in construction list of ThreadPool, so that wq is always started from work_queue[1] instead of work_queue[0] when multiple queues:

   last_work_queue++;
   last_work_queue %= work_queues.size();
   wq = work_queues[last_work_queue];

Signed-off-by: Pan Liu liupan1111@gmail.com

@liupan1111 liupan1111 requested a review from liewegas Apr 23, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 23, 2017

@liewegas , please help take a look, thanks.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 24, 2017

Retest it please.

@@ -39,7 +39,7 @@ ThreadPool::ThreadPool(CephContext *cct_, string nm, string tn, int n, const cha
ioprio_class(-1),
ioprio_priority(-1),
_num_threads(n),
last_work_queue(0),
last_work_queue(-1),

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

instead of making the first iteration an exception, i'd suggest following change:

diff --git a/src/common/WorkQueue.cc b/src/common/WorkQueue.cc
index b74763901b..7c91dda618 100644
--- a/src/common/WorkQueue.cc
+++ b/src/common/WorkQueue.cc
@@ -117,9 +117,8 @@ void ThreadPool::worker(WorkThread *wt)
       int tries = work_queues.size();
       bool did = false;
       while (tries--) {
-       last_work_queue++;
+       wq = work_queues[last_work_queue++];
        last_work_queue %= work_queues.size();
-       wq = work_queues[last_work_queue];

        void *item = wq->_void_dequeue();
        if (item) {
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

retest this please

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 24, 2017

@tchaikov cool, I will modify it.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 24, 2017

retest this please

1 similar comment
@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 24, 2017

retest this please

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

retest this please.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 24, 2017

@tchaikov jenkins makes me mad ...

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 24, 2017

retest this please

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/run-rbd-unit-tests.sh: line 9: 19464 Segmentation fault      unittest_librbd

retest this please.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 24, 2017

If we make this change, the variable should probably be called "next_work_queue"?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

agreed, and we need to update the commit message message accordingly.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 24, 2017

agree

@liupan1111 liupan1111 changed the title from src/common: initialize last_work_queue from 0 to -1. to src/common: change last_work_queue to next_work_queue. Apr 24, 2017

@@ -434,7 +434,7 @@ class ThreadPool : public md_config_obs_t {
};
private:
vector<WorkQueue_*> work_queues;
int last_work_queue;
int next_work_queue;

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

could take this chance to initialize this member variable in place.

This comment has been minimized.

@tchaikov

This comment has been minimized.

@liupan1111

liupan1111 Apr 25, 2017

Contributor

sorry not catch your idea clearly... this var is initialized in construction list, so what is your suggestion?

This comment has been minimized.

@tchaikov

tchaikov Apr 25, 2017

Contributor

@liupan1111 just put

int next_work_queue = 0;

here. and drop it from the initializer list.

This comment has been minimized.

@liupan1111

liupan1111 Apr 25, 2017

Contributor

er... Since all other vars are initialized in initializer list yet, I think it may makes the reader weird: only this variable is initialized in its declare place. If you prefer this style, I suggest we create a new pr to clean this.

This comment has been minimized.

@tchaikov

tchaikov Apr 25, 2017

Contributor

that's why i'd suggest do this in this very commit to avoid extra merge commit, and code churn. personally, i don't really like to give the source an overhaul to obscure the history just to use the new syntax, but we can always update the existing code base if we happen to touch a part of it: it's safer and it introduces less code churn.

This comment has been minimized.

@liupan1111

liupan1111 Apr 25, 2017

Contributor

@tchaikov reasonable to me, let me change it.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/run-rbd-unit-tests.sh: line 9: 27366 Segmentation fault      (core dumped) unittest_librbd

https://jenkins.ceph.com/job/ceph-pull-requests/22799/
@trociny and @dillaman is this a known issue or just a transient one?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

retest this please.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 24, 2017

@tchaikov have never seen that before -- perhaps related to this change?

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 24, 2017

Looks like it seg faulted again -- let me try to run w/ this PR locally to see what's going on.

last_work_queue++;
last_work_queue %= work_queues.size();
wq = work_queues[last_work_queue];
wq = work_queues[next_work_queue++];

This comment has been minimized.

@dillaman

dillaman Apr 24, 2017

Contributor

Potential for out-of-bounds access of the array when work queues are removed

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 24, 2017

@tchaikov Confirmed -- it's a bug introduced in this PR

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 25, 2017

@dillaman @tchaikov yes... Jason is right...

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 25, 2017

@tchaikov All checks have passed.

@dillaman

lgtm

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 25, 2017

@dillaman sorry for the false alarm (again) and thanks for confirming this.

@tchaikov tchaikov added the needs-qa label Apr 25, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Apr 25, 2017

src/common: change last_work_queue to next_work_queue.
Signed-off-by: Pan Liu <liupan1111@gmail.com>

@liewegas liewegas merged commit 5af0944 into ceph:master Apr 25, 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

@liupan1111 liupan1111 deleted the liupan1111:wip-fix-initial branch Apr 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment