Hello Amit/Robert/Others, I was looking into the code for heat_cpu.c in pm-qa. I think there are some issues with the current implementation. Please find details below -
1. #define NR_THREAD 3
pid_t thr_id[NR_THREAD];
I think the array thr_id should be dynamically allocated as number of
threads depend on number of cpus. However, we can eliminate the need for
this array as mentioned below at no 7.
2. There is no error checking for sysconf (...) call (line 83). If this
call fails num_cpus will be -1 which is a problem.
3. at line 122,
pthread_t *p_thread_ptr = (pthread_t *) malloc( num_cpus *
sizeof(pthread_attr_t));
we are allocating space for pthread_t. So sizeof(ptread_t) should be
used instead of sizeof(pthread_attr_t).
4. at line 144, SCHED_NORMAL is used. On my Ubuntu 13.04, <sched.h> does
not define SCHED_NORMAL. So it gives compile error if ANDROID is defined.
heat_cpu.c:145:44: error: ‘SCHED_NORMAL’ undeclared (first use in this
function)
We can either use SCHED_OTHER (which is equivalent to SCHED_NORMAL) or
include <linux/sched.h>
5. at line 168, error message is misleading. This error is related to
failure from pthread_create, not during setting affinity.
6. at line 76, the call to gettid results in linking error -
heat_cpu.c: In function ‘do_loop’:
heat_cpu.c:77:2: warning: implicit declaration of function ‘gettid’
[-Wimplicit-function-declaration]
/tmp/ccAhcUGE.o: In function `do_loop':
heat_cpu.c:(.text+0x4e): undefined reference to `gettid'
There is no libc wrapper for gettid. So I think it should be called as
below -
#include <sys/syscall.h>
thr_id[thread] = syscall(SYS_gettid);
With this changes the compilation warning and link error goes away and
it works. However, this call can be eliminated as suggested below at no 7.
7. There is a potential race condition between line 171 & 182, despite the
synchronization mechanism (mutex & condition variable) used. There is no
guaranty that do_loop(...) will execute first and thr_id[thread] =
gettid(); will execute before setting affinity as noted from below debug
output -
setting affinity for : 0
setting affinity for : 0
setting affinity for : 0
setting affinity for : 0
The code is setting affinity for tid 0, which it should not, despite the
synchronization code.
However, we can eliminate need for thr_id and the associated
synchronization if we use pthread_setaffinity_np(...) .
Would you care to test the alternative implementation using the patch
below (also attached)?
--------------------------------------------------------------------
diff --git a/utils/heat_cpu.c b/utils/heat_cpu.c
index 90d8a97..236cb1d 100644
--- a/utils/heat_cpu.c
+++ b/utils/heat_cpu.c
@@ -47,23 +47,12 @@
#include <string.h>
#include <math.h>
-#define NR_THREAD 3
-
int cont = 1;
-int cpu_index;
long long a = 1, c = 1;
float f = M_PI;
-pid_t thr_id[NR_THREAD];
int moderate_inst;
-
-#ifdef ANDROID
-int conditionMet;
-pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
-pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
-#endif
-
void *do_loop(void *data)
{
const long long b = 3;
@@ -71,14 +60,6 @@ void *do_loop(void *data)
a = 1;
c = 1;
-#ifdef ANDROID
- long int thread = (long int) data;
- thr_id[thread] = gettid();
- pthread_mutex_lock(&mutex);
- while (!conditionMet)
- pthread_cond_wait(&cond, &mutex);
- pthread_mutex_unlock(&mutex);
-#endif
while (cont) {
if (!moderate_inst) {
@@ -96,9 +77,15 @@ void *do_loop(void *data)
int main(int arg_count, char *argv[])
{
int ret, i;
- int num_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+ int num_cpus;
cpu_set_t cpuset;
+ num_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+ if (num_cpus < 0) {
+ printf("ERROR: sysconf error\n");
+ return -1;
+ }
+
printf("Num CPUs: %d\n", num_cpus);
if (arg_count > 1) {
if (!strcmp("moderate", argv[1])) {
@@ -120,7 +107,7 @@ int main(int arg_count, char *argv[])
}
pthread_t *p_thread_ptr = (pthread_t *) malloc(
- num_cpus * sizeof(pthread_attr_t));
+ num_cpus * sizeof(pthread_t));
if (!p_thread_ptr) {
printf("ERROR: out of memory\n");
@@ -141,7 +128,7 @@ int main(int arg_count, char *argv[])
#endif
#else
- ret = pthread_attr_setschedpolicy(&p[i], SCHED_NORMAL);
+ ret = pthread_attr_setschedpolicy(&p[i], SCHED_OTHER);
#endif
/* for each new object */
CPU_SET(i, &cpuset);
@@ -156,7 +143,7 @@ int main(int arg_count, char *argv[])
return ret;
}
#endif
- CPU_CLR(i, &cpuset);
+ CPU_CLR(i, &cpuset);
}
@@ -165,29 +152,23 @@ int main(int arg_count, char *argv[])
ret = pthread_create(&p_thread_ptr[i], &p[i],
do_loop, (void *)i);
if (ret < 0)
- printf("Error setting affinity for cpu%d\n", i);
+ printf("Error creating thread for cpu%d\n", i);
#ifdef ANDROID
CPU_ZERO(&cpuset);
CPU_SET(i, &cpuset);
- ret = sched_setaffinity(thr_id[i], sizeof(cpuset), &cpuset);
+ ret = pthread_setaffinity_np(p_thread_ptr[i],
sizeof(cpuset), &cpuset);
if (ret) {
- printf("Error setting affinity on pthread th_id\n");
+ printf("Error setting affinity on pthread\n");
printf("Error: %s\n", strerror(ret));
return ret;
}
#endif
}
-#ifdef ANDROID
- pthread_mutex_lock(&mutex);
- conditionMet = 1;
- pthread_cond_broadcast(&cond);
- pthread_mutex_unlock(&mutex);
-#endif
-
while (1)
sleep(1);
+
return 0;
}
--
Thanks,
-Meraj
heat_cpu_patch
Description: Binary data
_______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
