From 29c218f1b44b3ff589df5f433deba89f0c943a0d Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 16 Aug 2018 12:46:09 +0200 Subject: [PATCH 1/3] Slightly de-uglify Unix version of wxThreadInternal::Create() Just use wxUnusedVar() instead of defining and using the ugly WXUNUSED_STACKSIZE() macro. Also change an out of date comment as the thread priority is not the only attribute being changed here. No real changes. --- src/unix/threadpsx.cpp | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/unix/threadpsx.cpp b/src/unix/threadpsx.cpp index aac8d1d795..68614fc78a 100644 --- a/src/unix/threadpsx.cpp +++ b/src/unix/threadpsx.cpp @@ -987,14 +987,7 @@ wxThreadInternal::~wxThreadInternal() { } -#ifdef HAVE_PTHREAD_ATTR_SETSTACKSIZE - #define WXUNUSED_STACKSIZE(identifier) identifier -#else - #define WXUNUSED_STACKSIZE(identifier) WXUNUSED(identifier) -#endif - -wxThreadError wxThreadInternal::Create(wxThread *thread, - unsigned int WXUNUSED_STACKSIZE(stackSize)) +wxThreadError wxThreadInternal::Create(wxThread *thread, unsigned int stackSize) { if ( GetState() != STATE_NEW ) { @@ -1002,13 +995,15 @@ wxThreadError wxThreadInternal::Create(wxThread *thread, return wxTHREAD_RUNNING; } - // set up the thread attribute: right now, we only set thread priority + // set up the thread attribute: such as priority and stack size pthread_attr_t attr; pthread_attr_init(&attr); #ifdef HAVE_PTHREAD_ATTR_SETSTACKSIZE if (stackSize) pthread_attr_setstacksize(&attr, stackSize); +#else + wxUnusedVar(stackSize); #endif #ifdef HAVE_THREAD_PRIORITY_FUNCTIONS From 51c8496087f390c20544140014daa2d80063d3d0 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 16 Aug 2018 12:58:10 +0200 Subject: [PATCH 2/3] Don't change thread priority if it is default under Unix Skip all the code dealing with the priority/scheduling parameters if the priority is just wxPRIORITY_DEFAULT anyhow, as it's unnecessary to do anything in this case and it can result in spurious errors. Also extract this code into a separate SetThreadPriority() function to make wxThreadInternal::Create() itself shorter and more clear. Closes #18195. --- docs/changes.txt | 1 + src/unix/threadpsx.cpp | 124 ++++++++++++++++++++++++----------------- 2 files changed, 74 insertions(+), 51 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index d48e03e78f..2cd559dd2f 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -96,6 +96,7 @@ All: - Make wxList and wxVector iterators conform to input iterator requirements. - Fix MT-safety problem when reading and writing from wxSocket (jkubalik). - Fix build issues under HaikuOS (mill-j). +- Avoid spurious errors on thread creation under NetBSD. All (GUI): diff --git a/src/unix/threadpsx.cpp b/src/unix/threadpsx.cpp index 68614fc78a..6611e5ece4 100644 --- a/src/unix/threadpsx.cpp +++ b/src/unix/threadpsx.cpp @@ -987,6 +987,74 @@ wxThreadInternal::~wxThreadInternal() { } +static bool SetThreadPriority(pthread_attr_t& attr, int prio) +{ + if ( prio == wxPRIORITY_DEFAULT ) + { + // Don't even try to do anything, there is no need for it and it could + // result in failures if we don't handle setting the priority correctly + // under the current platform, see e.g. #18195. + return true; + } + +#ifdef HAVE_THREAD_PRIORITY_FUNCTIONS + int policy; + if ( pthread_attr_getschedpolicy(&attr, &policy) != 0 ) + { + wxLogError(_("Cannot retrieve thread scheduling policy.")); + return false; + } + +#ifdef __VMS__ + /* the pthread.h contains too many spaces. This is a work-around */ +# undef sched_get_priority_max +#undef sched_get_priority_min +#define sched_get_priority_max(_pol_) \ + (_pol_ == SCHED_OTHER ? PRI_FG_MAX_NP : PRI_FIFO_MAX) +#define sched_get_priority_min(_pol_) \ + (_pol_ == SCHED_OTHER ? PRI_FG_MIN_NP : PRI_FIFO_MIN) +#endif + + int max_prio = sched_get_priority_max(policy), + min_prio = sched_get_priority_min(policy); + + if ( min_prio == -1 || max_prio == -1 ) + { + wxLogError(_("Cannot get priority range for scheduling policy %d."), + policy); + return false; + } + + if ( max_prio == min_prio ) + { + // notify the programmer that this doesn't work here + wxLogWarning(_("Thread priority setting is ignored.")); + return false; + } + + struct sched_param sp; + if ( pthread_attr_getschedparam(&attr, &sp) != 0 ) + { + wxFAIL_MSG(wxT("pthread_attr_getschedparam() failed")); + return false; + } + + sp.sched_priority = min_prio + (prio*(max_prio - min_prio))/100; + + if ( pthread_attr_setschedparam(&attr, &sp) != 0 ) + { + wxFAIL_MSG(wxT("pthread_attr_setschedparam(priority) failed")); + return false; + } + + return true; +#else // !HAVE_THREAD_PRIORITY_FUNCTIONS + wxUnusedVar(attr); + + return false; +#endif // HAVE_THREAD_PRIORITY_FUNCTIONS +} + wxThreadError wxThreadInternal::Create(wxThread *thread, unsigned int stackSize) { if ( GetState() != STATE_NEW ) @@ -1006,60 +1074,14 @@ wxThreadError wxThreadInternal::Create(wxThread *thread, unsigned int stackSize) wxUnusedVar(stackSize); #endif -#ifdef HAVE_THREAD_PRIORITY_FUNCTIONS - int policy; - if ( pthread_attr_getschedpolicy(&attr, &policy) != 0 ) + if ( !SetThreadPriority(attr, GetPriority()) ) { - wxLogError(_("Cannot retrieve thread scheduling policy.")); + // We currently ignore the failure to set the thread priority as it + // seems better to create a thread with default priority than to + // not create one at all. + wxLogDebug("Failed to set thread priority to %d", GetPriority()); } -#ifdef __VMS__ - /* the pthread.h contains too many spaces. This is a work-around */ -# undef sched_get_priority_max -#undef sched_get_priority_min -#define sched_get_priority_max(_pol_) \ - (_pol_ == SCHED_OTHER ? PRI_FG_MAX_NP : PRI_FIFO_MAX) -#define sched_get_priority_min(_pol_) \ - (_pol_ == SCHED_OTHER ? PRI_FG_MIN_NP : PRI_FIFO_MIN) -#endif - - int max_prio = sched_get_priority_max(policy), - min_prio = sched_get_priority_min(policy), - prio = GetPriority(); - - if ( min_prio == -1 || max_prio == -1 ) - { - wxLogError(_("Cannot get priority range for scheduling policy %d."), - policy); - } - else if ( max_prio == min_prio ) - { - if ( prio != wxPRIORITY_DEFAULT ) - { - // notify the programmer that this doesn't work here - wxLogWarning(_("Thread priority setting is ignored.")); - } - //else: we have default priority, so don't complain - - // anyhow, don't do anything because priority is just ignored - } - else - { - struct sched_param sp; - if ( pthread_attr_getschedparam(&attr, &sp) != 0 ) - { - wxFAIL_MSG(wxT("pthread_attr_getschedparam() failed")); - } - - sp.sched_priority = min_prio + (prio*(max_prio - min_prio))/100; - - if ( pthread_attr_setschedparam(&attr, &sp) != 0 ) - { - wxFAIL_MSG(wxT("pthread_attr_setschedparam(priority) failed")); - } - } -#endif // HAVE_THREAD_PRIORITY_FUNCTIONS - #ifdef HAVE_PTHREAD_ATTR_SETSCOPE // this will make the threads created by this process really concurrent if ( pthread_attr_setscope(&attr, PTHREAD_SCOPE_SYSTEM) != 0 ) From cec14a334c28150c2237314534eb29b0d2853afb Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 16 Aug 2018 13:14:19 +0200 Subject: [PATCH 3/3] Document that wxThread::SetPriority() is broken under Unix The code setting thread priority doesn't work without changing the scheduling policy as thread priorities are simply ignored when using the default SCHED_OTHER (at least under Linux and NetBSD, but probably other systems too). See #18195. --- interface/wx/thread.h | 3 +++ src/unix/threadpsx.cpp | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/interface/wx/thread.h b/interface/wx/thread.h index e99caa1605..f308327a29 100644 --- a/interface/wx/thread.h +++ b/interface/wx/thread.h @@ -1248,6 +1248,9 @@ public: be only set after creating the thread with CreateThread(). But under all platforms this method can be called either before launching the thread using Run() or after doing it. + + Please note that currently this function is not implemented when using + the default (@c SCHED_OTHER) scheduling policy under POSIX systems. */ void SetPriority(unsigned int priority); diff --git a/src/unix/threadpsx.cpp b/src/unix/threadpsx.cpp index 6611e5ece4..db797e696f 100644 --- a/src/unix/threadpsx.cpp +++ b/src/unix/threadpsx.cpp @@ -1005,6 +1005,12 @@ static bool SetThreadPriority(pthread_attr_t& attr, int prio) return false; } + // TODO: on most (all?) systems, thread priorities can't be used with + // SCHED_OTHER policy, so we need to check if this is the current + // policy and change it to something else (SCHED_FIFO or SCHED_RR?) + // in order to be able to actually change the priority as without + // doing it the code below just not going to work. + #ifdef __VMS__ /* the pthread.h contains too many spaces. This is a work-around */ # undef sched_get_priority_max