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

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