thead pool debugging support proposal using native capabilities if po… #3628

devnexen · 2023-07-15T06:35:41Z

…ssible.

coveralls · 2023-07-15T08:13:10Z

coverage: 91.753% (+0.01%) from 91.743% when pulling 313e439 on thread_pool_debug into ae37d65 on master.

randombit · 2023-07-15T11:34:24Z

src/lib/utils/os_utils.cpp

+ #if defined(BOTAN_TARGET_OS_IS_LINUX)
+ r = pthread_setname_np(hdl, name);
+ #elif defined(BOTAN_TARGET_OS_IS_FREEBSD)
+ r = pthread_set_name_np(hdl, name);


FreeBSD also seems to support pthead_setname_np (https://man.freebsd.org/cgi/man.cgi?query=pthread_set_name_np) which would reduce the number of conditionals.

If we consider windows the only exception in term of supporting EOL releases then yes.

randombit · 2023-07-15T11:35:15Z

src/lib/utils/os_utils.h

@@ -148,6 +152,10 @@ void page_allow_access(void* page);
 */
 void page_named(void* page, size_t size);

+#if defined(BOTAN_TARGET_OS_HAS_THREADS)
+void set_thread_name(std::thread& thread, const char* const name);


Is it required (by any OS) that this point to static storage? If so that should be documented here. Otherwise, name should be a const std::string& instead of a C style string.

(I would say a std::string_view but you can't convert that to a NULL terminated string)

randombit · 2023-07-15T11:37:49Z

src/lib/utils/os_utils.cpp

+ #elif defined(BOTAN_TARGET_OS_IS_FREEBSD)
+ r = pthread_set_name_np(hdl, name);
+ #elif defined(BOTAN_TARGET_OS_IS_OPENBSD)
+ r = pthread_set_name_np(hdl, const_cast<char*>(name));


This cast doesn't seem to be necessary? (https://man.openbsd.org/pthread_set_name_np)

randombit · 2023-07-15T11:39:06Z

src/lib/utils/os_utils.cpp

+ #elif defined(BOTAN_TARGET_OS_IS_OPENBSD)
+ r = pthread_set_name_np(hdl, const_cast<char*>(name));
+ #elif defined(BOTAN_TARGET_OS_IS_NETBSD)
+ r = pthread_set_name_np(hdl, "%s", const_cast<char*>(name));


What a weird interface

fair point, you end up 99% of the time with a string anyway..

randombit · 2023-07-15T11:40:39Z

src/lib/utils/os_utils.cpp

+ // also gives a chance to be in coredumps.
+ wchar_t val[16];
+ if(MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, name, -1, val, 16) > 0) {
+ r = SetThreadDescription(hdl, val);


IIUC https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreaddescription this function is only supported on Windows 10 or higher, which I think would be problematic

In general the policy is that we don't support operating systems that are no longer supported by the OS vendor, but for Windows we are basically forced to be more lax than that simply due to the huge installed base of out of date systems.

I think practically speaking we are limited to using features that are available in the oldest version of Windows that is supported by the oldest version of VC++ we support. For VC that's VC2022 now. But I can't find anything definite about the oldest OS that VC2022 will allow you to target. I know we reverted #365 because people were still building XP-compatible binaries in 2016.

randombit · 2023-07-15T11:45:39Z

src/lib/utils/os_utils.cpp

+ int r;
+ auto hdl = thread.native_handle();
+ #if defined(BOTAN_TARGET_OS_IS_LINUX)
+ r = pthread_setname_np(hdl, name);


Instead of assigning to r and then using BOTAN_UNUSED later on in the function just static_cast<void>(pthread_set...) within each block.

randombit · 2023-07-15T11:55:00Z

src/lib/utils/os_utils.cpp

+#if defined(BOTAN_TARGET_OS_HAS_THREADS)
+void OS::set_thread_name(std::thread& thread, const char* const name) {
+ int r;
+ auto hdl = thread.native_handle();


std::thread::native_handle isn't necessarily guaranteed to exist, which would cause this to fail to compile on a (probably weird) OS that supported threads but didn't let you access native handles.

Instead call thread.native_handle() within each block which ensures we only call it if we have some idea what OS we are on.

ah right I thought HAS_THREADS meant "all the way", fair enough then.

randombit · 2023-09-13T15:18:52Z

src/lib/utils/os_utils.cpp

+ #elif defined(BOTAN_TARGET_OS_IS_NETBSD)
+ static_cast<void>(pthread_set_name_np(thread.native_handle(), "%s", const_cast<char*>(name.c_str())));
+ #elif defined(BOTAN_TARGET_OS_HAS_WIN32) && defined(BOTAN_BUILD_COMPILER_IS_MSVC)
+ const DWORD MS_VC_EXCEPTION = 0x406D1388;


This approach doesn't seem so useful since it's works if you are currently attached to a debugger.

If it's possible to check at compile time what the target OS is, I'd suggest calling the Windows 10 function if it is available and for older systems just do nothing. If that's not possible I'd say just remove this block entirely and leave a comment that we can use SetThreadDescription in the future.

In 33dee2f I deprecated support for older Windows but we'll probably want to wait at least a release or two before pulling the trigger on it.

randombit

Thanks! This is looking very good now. I left a few comments.

randombit · 2023-09-29T21:25:08Z

src/lib/utils/os_utils.h

@@ -148,6 +152,10 @@ void page_allow_access(void* page);
 */
 void page_named(void* page, size_t size);

+#if defined(BOTAN_TARGET_OS_HAS_THREADS)
+void set_thread_name(std::thread& thread, std::string& name);


Ah I guess name cannot be a string_view since we need to get the null terminated value, so I think it should be a const std::string&

randombit · 2023-09-29T21:25:29Z

src/lib/utils/thread_utils/thread_pool.cpp

@@ -41,6 +41,8 @@ Thread_Pool& Thread_Pool::global_instance() {

 Thread_Pool::Thread_Pool(std::optional<size_t> opt_pool_size) {
 m_shutdown = false;
+ // On Linux, it is 16 length max, including terminator
+ std::string tname = "Botan thread";


This should be const

randombit · 2023-09-29T21:27:27Z

src/lib/utils/os_utils.cpp

+ #elif defined(BOTAN_TARGET_OS_IS_NETBSD)
+ static_cast<void>(pthread_set_name_np(thread.native_handle(), "%s", const_cast<char*>(name.c_str())));
+ #elif defined(BOTAN_TARGET_OS_HAS_WIN32) && defined(BOTAN_BUILD_COMPILER_IS_MSVC)
+ // Using SetThreadDescription from Win10


There should be a BOTAN_UNUSED here as well - currently Windows CI is failing because the parameters are not used

randombit · 2023-09-29T21:27:50Z

src/lib/utils/os_utils.cpp

+ // TODO other possible oses ?
+ // macOs does not seem to allow to name threads other than the current one.
+ BOTAN_UNUSED(thread);
+ BOTAN_UNUSED(name);


You can pass multiple args to a single invocation of BOTAN_UNUSED - BOTAN_UNUSED(thread, name)

…ssible.

randombit

Looks great. Thanks for addressing the review comments.

devnexen force-pushed the thread_pool_debug branch 3 times, most recently from b91f69d to f53b820 Compare July 15, 2023 07:31

randombit reviewed Jul 15, 2023

View reviewed changes

devnexen force-pushed the thread_pool_debug branch 3 times, most recently from 4229e6b to 02fb333 Compare July 15, 2023 22:07

randombit reviewed Sep 13, 2023

View reviewed changes

devnexen force-pushed the thread_pool_debug branch from 02fb333 to 7fe391f Compare September 13, 2023 19:17

randombit reviewed Sep 29, 2023

View reviewed changes

thead pool debugging support proposal using native capabilities if po…

313e439

…ssible.

devnexen force-pushed the thread_pool_debug branch from 7fe391f to 313e439 Compare September 29, 2023 21:46

randombit approved these changes Sep 30, 2023

View reviewed changes

randombit added this to the Botan 3.2.0 milestone Sep 30, 2023

devnexen merged commit 86381e7 into master Sep 30, 2023
38 checks passed

randombit deleted the thread_pool_debug branch October 5, 2023 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

thead pool debugging support proposal using native capabilities if po… #3628

thead pool debugging support proposal using native capabilities if po… #3628

devnexen commented Jul 15, 2023

coveralls commented Jul 15, 2023 •

edited

Loading

randombit Jul 15, 2023

devnexen Jul 15, 2023

randombit Jul 15, 2023

randombit Jul 15, 2023

randombit Jul 15, 2023

devnexen Jul 15, 2023

randombit Jul 15, 2023

randombit Jul 15, 2023

randombit Jul 15, 2023

randombit Jul 15, 2023

devnexen Jul 15, 2023

randombit Sep 13, 2023

randombit left a comment

randombit Sep 29, 2023

randombit Sep 29, 2023

randombit Sep 29, 2023

randombit Sep 29, 2023

randombit left a comment

thead pool debugging support proposal using native capabilities if po… #3628

thead pool debugging support proposal using native capabilities if po… #3628

Conversation

devnexen commented Jul 15, 2023

coveralls commented Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randombit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randombit left a comment

Choose a reason for hiding this comment

coveralls commented Jul 15, 2023 •

edited

Loading