log: save/fetch thread name infra by mchangir · Pull Request #60058 · ceph/ceph

mchangir · 2024-09-30T12:22:50Z

pthread name is saved against the pthread ID in a class-wide static unordered_map as part of the Thread class

save/fetch methods are implemented to manage the thread names
thread names are never removed from the map

Fixes: https://tracker.ceph.com/issues/50743
Signed-off-by: Milind Changire mchangir@redhat.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

src/common/Thread.cc

mchangir · 2024-10-01T11:42:16Z

jenkins test make check

mchangir · 2024-10-01T11:42:29Z

jenkins test api

mchangir · 2024-10-01T11:42:56Z

jenkins test make check arm64

rzarzynski

The global registry might be worrisome from the performance point of view. I'm not sure we really need it. Please see my comments.

src/common/Thread.cc

src/log/Entry.h

rzarzynski · 2024-10-01T23:18:04Z

src/common/Thread.h

  int cpuid;
  std::string thread_name;
+  static std::mutex threadid_name_mutex;
+  static std::unordered_map<pthread_t, std::string> threadid_name;


Doesn't this look like reinventing the thread local storage?

It does look like it ... but I'm trying to help Log::dump_recent() find the thread name after a thread ceases to exist.
I'm also worried that a pthread_t may be recycled by the time Log::dump_recent() eventually calls pthread_getname_np().

I think I understand what's your intention. My point is recording in TLS would happen when the thread is still alive, not at the dump_recent() stage.

src/log/Entry.h

mchangir · 2024-10-04T12:05:06Z

now storing global thread IDs in the thread_id --> thread_name lookup table
thread names are std::array<char, 16> in the lookup table instead of std::string

mchangir · 2024-10-04T18:38:13Z

jenkins test api

mchangir · 2024-10-04T18:38:34Z

jenkins test make check arm64

badone · 2024-10-06T01:10:28Z

@markhpc so it is on your radar

mchangir · 2024-10-06T11:56:30Z

jenkins test api

mchangir · 2024-10-06T11:56:53Z

jenkins test make check arm64

mchangir · 2024-10-06T12:03:39Z

thread names are never removed from the map

vshankar · 2024-10-07T04:22:10Z

thread names are never removed from the map

Practically this map should not grow too huge to cause problems, but still I'm asking this for brevity - are we okay with this?

rzarzynski

Conceptually seems fine. Hopefully we could use C++'s thread_local for delegating the TLS interaction out.

rzarzynski · 2024-10-07T14:38:39Z

src/common/Thread.h

+     * we do not allocate extra memory to store the 32bit global ID
+     * we just type cast the 32bit value to a (void*) and save it in the TLS
+     */
+    pthread_once(&global_thread_id_key_once, global_thread_id_key_alloc);


C++ has the thread_local that takes from you the responsibility for this low level stuff.

rzarzynski · 2024-10-07T14:40:14Z

src/log/Log.cc

    for (const auto& e : t) {
-      recent_pthread_ids.emplace(e.m_thread);
+      recent_pthread_ids.emplace(std::make_pair(e.m_thread,
+						Thread::fetch_thread_name(e.m_global_thread_id)));


mchangir · 2024-10-08T03:17:57Z

now using thread_local to save the unique thread ID

mchangir · 2024-10-08T07:20:01Z

jenkins test api

mchangir · 2024-10-08T07:22:05Z

jenkins test windows

mchangir · 2024-10-08T10:49:57Z

class static members have now been declared as static inline to do away with the class static member declaration and definition confusion

batrick · 2024-10-04T13:59:05Z

src/common/Thread.cc

 #endif

+// STATIC
+std::unordered_map<unsigned int, std::array<char, 16>> Thread::threadid_name;


Suggested change

std::unordered_map<unsigned int, std::array<char, 16>> Thread::threadid_name;

std::unordered_map<unsigned int, boost::container::small_vector<char, 16>> Thread::threadid_name;

batrick · 2024-10-08T12:46:18Z

src/log/Entry.h

  time m_stamp;
  pthread_t m_thread;
  short m_prio, m_subsys;
+  unsigned int m_global_thread_id; // for Log::dump_recent()


I feel like it would be much simpler to:

save the thread's name in thread_local storage (once)

copy the 128 (16 byte) value in this Entry

But, when the thread is gone during dump_recent(), where does the thread name gets fetched?

It's in the Entry itself. It can be copied out into the recent_pthread_ids set.

I want to add that since there is strong interest to get this fix into older releases, I would strongly recommend this simple solution. I suspect the performance and memory cost to be inconsequential. It can be explored afterwards whether optimizations are worthwhile.

I had indeed started this PR with copying the thread name into the Entry object ... but was discouraged by Radek to do so due to memory and time consumption in the hot path

Nope, it wasn't just copying of 16 bytes; it was a call to the lock-protected Thread::fetch_thread_name() (#60058 (comment)) which spurs the concern.

IIUC Patrick is arguing for the simpler approach where the thread name is copied from TLS by the Entry constructor:

Maybe instead of this complex, lock-protected container every Thread could cache its name in a thread_local variable from which the Entry constructor would simply copy?

Okay, then we wouldn't require any thread local storage at all.
We could simply call ceph_pthread_getname(pthread_self(), ...) in the Entry constructor and save it for future reference.
Does this sound reasonable ?

This is what I understand:

Save the thread name (once) in TLS during thread creation (one call to pthread_getname_np(...))

In the ctor for Entry, fetch the thread name from TLS (no locking required)

copy the thread name in the Entry object

So, repeated calls to pthread_getname_np(...) is avoided and the overhead is the 16byte copy.

Discussed this with @mchangir a bit and it's boiling down to deciding if we want to store the thread name in TLS vs calling pthread_getname_np() when creating Entry object. As @rzarzynski mentioned, the pthread call might have a more optimized implementation. So, I'm really on the fence hence to choose between the two.

rzarzynski · 2024-10-08T16:56:38Z

src/common/Thread.cc

 #endif

+// Thread Local Storage
+thread_local unsigned int global_thread_id = 0;


C++ allows for thread_local static class member while the ID (or cached thread name) conceptually belongs to Thread.

The global in the name is bit misleading IMHO.

vshankar · 2024-10-10T02:10:07Z

jenkins test api

vshankar · 2024-10-10T02:10:13Z

jenkins test windows

mchangir · 2024-10-14T02:56:15Z

jenkins test api

mchangir · 2024-10-14T02:56:25Z

jenkins test windows

mchangir · 2024-10-14T02:56:43Z

jenkins test make check arm64

mchangir · 2024-10-14T11:02:08Z

rebased
PR update with revised thread name fetch procedure

mchangir · 2024-10-14T12:01:08Z

jenkins test api

mchangir · 2024-10-15T10:53:13Z

jenkins test make check arm64

* pthread name is saved in a thread_local storage * the thread_local name is copied into Entry object's ctor * Log::dump_recent() reads the thread name from the Entry object's data member when dumping logs Fixes: https://tracker.ceph.com/issues/50743 Signed-off-by: Milind Changire <mchangir@redhat.com>

mchangir · 2024-10-15T12:00:43Z

minor change to set m_thread_name[15] = '\0'; in class Entry ctor

mchangir · 2024-10-15T16:22:12Z

jenkins test api

mchangir · 2024-10-15T16:22:41Z

jenkins test make check

ljflores · 2024-10-16T20:26:56Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues68533

batrick · 2024-10-24T03:22:49Z

Bug: https://tracker.ceph.com/issues/68691

batrick · 2024-10-24T03:25:25Z

src/common/Thread.h

  int detach();
  int set_affinity(int cpuid);
+  static const std::string get_thread_name() {
+    return Thread::thread_name;


There are many threads in Ceph (and the MDS) which do not use this class Thread. So the "name" will wrongly be empty every time this is called.

It might be a good idea to make those threads use Thread class then.

I think the idea behind helpers like make_named_thread() was the opposite -- slowly move off of Thread and just use std::thread.

github-actions bot added the common label Sep 30, 2024

vshankar requested review from a team, badone, ljflores and rzarzynski September 30, 2024 12:46

mchangir force-pushed the log-save-thread-name-in-log-entries branch from 8d71897 to 2b08e2c Compare October 1, 2024 03:15

mchangir added the needs-review label Oct 1, 2024

vshankar reviewed Oct 1, 2024

View reviewed changes

src/common/Thread.cc Outdated Show resolved Hide resolved

rzarzynski reviewed Oct 1, 2024

View reviewed changes

mchangir force-pushed the log-save-thread-name-in-log-entries branch from 2b08e2c to 00386c1 Compare October 2, 2024 09:38

rzarzynski reviewed Oct 2, 2024

View reviewed changes

src/log/Entry.h Outdated Show resolved Hide resolved

mchangir force-pushed the log-save-thread-name-in-log-entries branch 2 times, most recently from eebb5a5 to 67d2532 Compare October 4, 2024 12:02

badone requested a review from markhpc October 6, 2024 01:08

mchangir changed the title ~~log: save/fetch/remove thread name infra~~ log: save/fetch thread name infra Oct 6, 2024

mchangir force-pushed the log-save-thread-name-in-log-entries branch from 67d2532 to 87ac83c Compare October 6, 2024 12:02

rzarzynski approved these changes Oct 7, 2024

View reviewed changes

mchangir force-pushed the log-save-thread-name-in-log-entries branch from 87ac83c to 3e7bdfa Compare October 8, 2024 03:14

vshankar added the needs-qa label Oct 8, 2024

mchangir force-pushed the log-save-thread-name-in-log-entries branch from 3e7bdfa to 8f7d3f6 Compare October 8, 2024 10:47

batrick reviewed Oct 8, 2024

View reviewed changes

rzarzynski reviewed Oct 8, 2024

View reviewed changes

ljflores added the wip-yuri2-testing label Oct 8, 2024

mchangir force-pushed the log-save-thread-name-in-log-entries branch from 30d4875 to f385c43 Compare October 14, 2024 10:26

mchangir force-pushed the log-save-thread-name-in-log-entries branch from f385c43 to 1837811 Compare October 14, 2024 11:16

yuriw added wip-yuri8-testing and removed wip-yuri2-testing labels Oct 14, 2024

mchangir force-pushed the log-save-thread-name-in-log-entries branch from 1837811 to 0be8d01 Compare October 15, 2024 11:57

yuriw merged commit 6e72881 into ceph:main Oct 16, 2024

batrick reviewed Oct 24, 2024

View reviewed changes

mchangir mentioned this pull request Nov 14, 2024

reef: log: save/fetch thread name infra #60728

Merged

	std::unordered_map<unsigned int, std::array<char, 16>> Thread::threadid_name;
	std::unordered_map<unsigned int, boost::container::small_vector<char, 16>> Thread::threadid_name;

Conversation

mchangir commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

mchangir commented Oct 1, 2024

Uh oh!

mchangir commented Oct 1, 2024

Uh oh!

mchangir commented Oct 1, 2024

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mchangir commented Oct 4, 2024

Uh oh!

mchangir commented Oct 4, 2024

Uh oh!

mchangir commented Oct 4, 2024

Uh oh!

badone commented Oct 6, 2024

Uh oh!

mchangir commented Oct 6, 2024

Uh oh!

mchangir commented Oct 6, 2024

Uh oh!

mchangir commented Oct 6, 2024

Uh oh!

vshankar commented Oct 7, 2024

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchangir commented Oct 8, 2024

Uh oh!

mchangir commented Oct 8, 2024

Uh oh!

mchangir commented Oct 8, 2024

Uh oh!

mchangir commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchangir commented Sep 30, 2024 •

edited

Loading