Skip to content
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.

Sign up for GitHub

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

Jump to bottom

common: add thread names #5882

Merged
merged 1 commit into from Jan 11, 2016
Merged

common: add thread names #5882

merged 1 commit into from Jan 11, 2016

Conversation

aiicore
Copy link
Contributor

@aiicore aiicore commented Sep 11, 2015

Adding names to threads simplifies cpu usage realtime tracking e.g. top -H -p <OSD_PID>

This commit changes Thread.create() method forcing to pass thread name.

Example:

> pgrep -lf ceph-osd
2300236 /usr/bin/ceph-osd -i 0 --pid-file /var/run/ceph/osd.0.pid -c /etc/ceph/ceph.conf --cluster ceph
...

> top -H -p 2300236 -b -n 1 
...
    PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
2300295 root      20   0 1409m 513m  46m S  5.9  0.4   0:00.18 file journal
2300299 root      20   0 1409m 513m  46m S  3.9  0.4   0:01.76 thread pool
2645280 root      20   0 1409m 513m  46m S  3.9  0.4   0:00.02 pipe rader
2300296 root      20   0 1409m 513m  46m S  2.0  0.4   0:00.03 ceph-osd

Signed-off-by: Igor Podoski igor.podoski@ts.fujitsu.com

@XinzeChi
Copy link
Contributor

👍 a good tools

@liewegas
Copy link
Member

Note that there is a limit of 15 or 16 characters for this name. I believe set_name will return an error if it's exceeded. I tried to do a patch that populated thread names with, e.g., workqueue names, but they were too long :(

I think it should assert if the length is too long so we catch those bugs immediately instead of it silently failing...

@liewegas liewegas self-assigned this Sep 11, 2015
@aiicore
Copy link
Contributor Author

aiicore commented Sep 11, 2015

Yep you're right, missed in manual. I'll fix this on Monday.

EDIT_1: fixed
I allowed empty name "thread stealth mode ;)", so there is only one strlen() in assert and no additional variable for handling name length. I think it would be ok, because there is not so many Thread.create() in the code, and I assume that everyone who adds new thread will name it.

According to manual pthread_setname_np() can fail with error like open(2) at /proc/self/task/[tid]/comm, but I don't check this. If it fails, you will see duplicated process name in thread list.

EDIT_2: added missing name + squashed

@loic-bot
Copy link

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/7749/

@loic-bot
Copy link

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/7759/

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/7775/

@aiicore
Copy link
Contributor Author

aiicore commented Sep 21, 2015

Another bonus using thread names can be seen in core dumps:

/etc/sysctl:
kernel.core_pattern = core.%p.%s.%t.%e

ls core*
core.3376021.6.1442393620.ceph-osd   
core.3372297.6.1442393171.thread pool
core.3213193.11.1442824968.reaper

Now I see, that maybe i should concatenate names with '_' instead of spaces.

@XinzeChi
Copy link
Contributor

@aiicore , How are you getting on with this PR? I think this is every useful for debugging performance.

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9257/

@aiicore
Copy link
Contributor Author

aiicore commented Nov 13, 2015

@XinzeChi, Done.

liewegas
liewegas reviewed Nov 24, 2015
View reviewed changes
src/mds/MDLog.cc

submit_thread.create();
submit_thread.create("md_sumbit");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

md_submit

@liewegas
Copy link
Member

some cosmetic nits, but otherwise this looks great!

@liewegas liewegas assigned liewegas and unassigned liewegas Nov 24, 2015
@aiicore aiicore force-pushed the add_thread_names branch 2 times, most recently from 1b2beeb to c504052 Compare November 25, 2015 08:07
@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9542/

@aiicore aiicore mentioned this pull request Nov 25, 2015
Open
@aiicore aiicore force-pushed the add_thread_names branch 2 times, most recently from f2820a6 to eb35d4f Compare December 2, 2015 09:45
@loic-bot
Copy link

loic-bot commented Dec 2, 2015

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9753/

@loic-bot
Copy link

loic-bot commented Dec 2, 2015

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9756/

@loic-bot
Copy link

loic-bot commented Dec 2, 2015

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9764/

@loic-bot
Copy link

loic-bot commented Dec 2, 2015

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9768/

@loic-bot
Copy link

loic-bot commented Dec 3, 2015

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9804/

aiicore
aiicore reviewed Dec 3, 2015
View reviewed changes
src/osd/OSD.cc
&osd->recovery_tp),
op_gen_wq("op_gen_wq", cct->_conf->osd_recovery_thread_timeout, &osd->osd_tp),
op_gen_wq("op_gen_wq", "wq_gen_op", cct->_conf->osd_recovery_thread_timeout, &osd->osd_tp),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes It looks funny, but I wanted to keep thread naming convention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which convention? Matching the other name seems convenient..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread name looks just like a mirror of wq name ;)

@aiicore
Copy link
Contributor Author

aiicore commented Dec 3, 2015

@Sage could you have a look in the meantime on my comments and general wq_/fn_ naming like you did before with ms_?

@liewegas
Copy link
Member

liewegas commented Dec 3, 2015

fn_ and tp_ prefixes seem okay to me. (not wq_, since the thread belongs to the thread pool, and we don't want to change the thread name for every work item).

@liewegas
Copy link
Member

liewegas commented Jan 4, 2016

@aiicore can you rebase please?

threads: add thread names
4a4b447 
Adding names to threads simplifies cpu usage realtime tracking
e.g. top -H -p <OSD_PID>

This commit changes Thread.create() method forcing to pass thread name.

Signed-off-by: Igor Podoski <igor.podoski@ts.fujitsu.com>
liewegas added a commit that referenced this pull request Jan 11, 2016
@liewegas
Merge pull request #5882 from aiicore/add_thread_names
b6e9902 
common: add thread names

Reviewed-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit b6e9902 into ceph:master Jan 11, 2016
@aiicore aiicore deleted the add_thread_names branch January 11, 2016 14:06
@ghost ghost changed the title threads: add thread names common: add thread names Feb 10, 2016
branch-predictor referenced this pull request in adamemerson/ceph Feb 16, 2016
@adamemerson
finisher: Update to C++11 concurrency
81530ad 
The only visible effect of this change is that the constructor no longer takes a
name for the finisher thread.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liewegas liewegas

Labels
core feature needs-qa wip-sage-testing
Projects
None yet
Milestone
No milestone
5 participants
@aiicore @XinzeChi @liewegas @loic-bot @yuyuyu101