On Tue, Mar 24, 2015 at 11:17 AM, Maxim Uvarov <[email protected]> wrote: > On 03/24/15 01:35, Bill Fischofer wrote: >> >> No, it isn't. The review says that the change looks accurate and properly >> written. However, since there was no explanation as to why it was made I >> can only assume it was a change of personal perference on the part of the >> author. I have no problem with that. >> > > I looked to function then jumped on ctag variable definition and vim ctags > went to static variable in the beginning of the file, but it should jump to > declaration of the same function name in the beginning of function. So I > needed to rename variable on top or variable in functions. Because of > defining variable with global_ prefix is confusing for me, I renamed > variables inside the functions.
The global_mem variable is tightly coupled with global_shm, which has the same visibility problem, you should do something about it too while you're down this path. And there is one more thing, struct per_thread_mem_t has a member called global_mem, it was only natural that the local variables were named the same. Maybe it would be better to change the name of the struct member to g_mem or something like that and use the same name at function level scope. > > Thanks, > Maxim. > > >> On Mon, Mar 23, 2015 at 3:40 PM, Anders Roxell <[email protected] >> <mailto:[email protected]>> wrote: >> >> >> >> On 23 March 2015 at 20:39, Bill Fischofer >> <[email protected] <mailto:[email protected]>> wrote: >> >> Not sure why this is needed, but: >> >> Reviewed-by: Bill Fischofer <[email protected] >> <mailto:[email protected]>> >> >> >> >> Bill is this the same as saying >> >> Whatevered-by: Bill Fischofer <[email protected] >> <mailto:[email protected]>> >> >> Cheers, >> Anders >> >> >> On Mon, Mar 23, 2015 at 8:04 AM, Maxim Uvarov >> <[email protected] <mailto:[email protected]>> wrote: >> >> global_mem is global static variable usage same name variable >> inside function is confusing. Rename local vars from >> global_mem >> to mem. >> >> Signed-off-by: Maxim Uvarov <[email protected] >> <mailto:[email protected]>> >> >> --- >> test/validation/odp_synchronizers.c | 158 >> ++++++++++++++++++------------------ >> 1 file changed, 79 insertions(+), 79 deletions(-) >> >> diff --git a/test/validation/odp_synchronizers.c >> b/test/validation/odp_synchronizers.c >> index ab9164f..b8f4e6a 100644 >> --- a/test/validation/odp_synchronizers.c >> +++ b/test/validation/odp_synchronizers.c >> @@ -107,7 +107,7 @@ static void >> thread_delay(per_thread_mem_t *per_thread_mem, uint32_t >> iterations) >> /* Initialise per-thread memory */ >> static per_thread_mem_t *thread_init(void) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> odp_shm_t global_shm; >> uint32_t per_thread_mem_len; >> @@ -122,10 +122,10 @@ static per_thread_mem_t >> *thread_init(void) >> per_thread_mem->thread_core = odp_cpu_id(); >> >> global_shm = odp_shm_lookup(GLOBAL_SHM_NAME); >> - global_mem = odp_shm_addr(global_shm); >> + mem = odp_shm_addr(global_shm); >> CU_ASSERT(global_mem != NULL); >> >> - per_thread_mem->global_mem = global_mem; >> + per_thread_mem->global_mem = mem; >> >> return per_thread_mem; >> } >> @@ -160,13 +160,13 @@ static void >> custom_barrier_wait(custom_barrier_t *custom_barrier) >> static uint32_t barrier_test(per_thread_mem_t >> *per_thread_mem, >> odp_bool_t no_barrier_test) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> uint32_t barrier_errs, iterations, cnt, >> i_am_slow_thread; >> uint32_t thread_num, slow_thread_num, >> next_slow_thread, num_threads; >> uint32_t lock_owner_delay, barrier_cnt1, barrier_cnt2; >> >> thread_num = odp_cpu_id() + 1; >> - global_mem = per_thread_mem->global_mem; >> + mem = per_thread_mem->global_mem; >> num_threads = global_mem->g_num_threads; >> iterations = BARRIER_ITERATIONS; >> >> @@ -175,10 +175,10 @@ static uint32_t >> barrier_test(per_thread_mem_t *per_thread_mem, >> >> for (cnt = 1; cnt < iterations; cnt++) { >> /* Wait here until all of the threads >> reach this point */ >> - custom_barrier_wait(&global_mem->custom_barrier1[cnt]); >> + custom_barrier_wait(&mem->custom_barrier1[cnt]); >> >> - barrier_cnt1 = global_mem->barrier_cnt1; >> - barrier_cnt2 = global_mem->barrier_cnt2; >> + barrier_cnt1 = mem->barrier_cnt1; >> + barrier_cnt2 = mem->barrier_cnt2; >> >> if ((barrier_cnt1 != cnt) || (barrier_cnt2 >> != cnt)) { >> printf("thread_num=%u barrier_cnts of %u %u cnt=%u\n", >> @@ -187,9 +187,9 @@ static uint32_t >> barrier_test(per_thread_mem_t *per_thread_mem, >> } >> >> /* Wait here until all of the threads >> reach this point */ >> - custom_barrier_wait(&global_mem->custom_barrier2[cnt]); >> + custom_barrier_wait(&mem->custom_barrier2[cnt]); >> >> - slow_thread_num = global_mem->slow_thread_num; >> + slow_thread_num = mem->slow_thread_num; >> i_am_slow_thread = thread_num == >> slow_thread_num; >> next_slow_thread = slow_thread_num + 1; >> if (num_threads < next_slow_thread) >> @@ -206,30 +206,30 @@ static uint32_t >> barrier_test(per_thread_mem_t *per_thread_mem, >> if (i_am_slow_thread) { >> thread_delay(per_thread_mem, lock_owner_delay); >> lock_owner_delay += BASE_DELAY; >> - if ((global_mem->barrier_cnt1 != >> cnt) || >> - (global_mem->barrier_cnt2 != cnt) || >> - (global_mem->slow_thread_num >> + if ((mem->barrier_cnt1 != cnt) || >> + (mem->barrier_cnt2 != cnt) || >> + (mem->slow_thread_num >> != slow_thread_num)) >> barrier_errs++; >> } >> >> if (no_barrier_test == 0) >> - odp_barrier_wait(&global_mem->test_barriers[cnt]); >> + odp_barrier_wait(&mem->test_barriers[cnt]); >> >> - global_mem->barrier_cnt1 = cnt + 1; >> + mem->barrier_cnt1 = cnt + 1; >> odp_sync_stores(); >> >> if (i_am_slow_thread) { >> - global_mem->slow_thread_num = next_slow_thread; >> - global_mem->barrier_cnt2 = cnt + 1; >> + mem->slow_thread_num = next_slow_thread; >> + mem->barrier_cnt2 = cnt + 1; >> odp_sync_stores(); >> } else { >> - while (global_mem->barrier_cnt2 != >> (cnt + 1)) >> + while (mem->barrier_cnt2 != (cnt + 1)) >> thread_delay(per_thread_mem, BASE_DELAY); >> } >> } >> >> - if ((global_mem->g_verbose) && (barrier_errs != 0)) >> + if ((mem->g_verbose) && (barrier_errs != 0)) >> printf("\nThread %u (id=%d core=%d) had %u >> barrier_errs" >> " in %u iterations\n", thread_num, >> per_thread_mem->thread_id, >> @@ -293,14 +293,14 @@ static void >> spinlock_api_test(odp_spinlock_t *spinlock) >> >> static void *spinlock_api_tests(void *arg UNUSED) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> odp_spinlock_t local_spin_lock; >> >> per_thread_mem = thread_init(); >> - global_mem = per_thread_mem->global_mem; >> + mem = per_thread_mem->global_mem; >> >> - odp_barrier_wait(&global_mem->global_barrier); >> + odp_barrier_wait(&mem->global_barrier); >> >> spinlock_api_test(&local_spin_lock); >> spinlock_api_test(&per_thread_mem->per_thread_spinlock); >> @@ -331,14 +331,14 @@ static void >> ticketlock_api_test(odp_ticketlock_t *ticketlock) >> >> static void *ticketlock_api_tests(void *arg UNUSED) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> odp_ticketlock_t local_ticket_lock; >> >> per_thread_mem = thread_init(); >> - global_mem = per_thread_mem->global_mem; >> + mem = per_thread_mem->global_mem; >> >> - odp_barrier_wait(&global_mem->global_barrier); >> + odp_barrier_wait(&mem->global_barrier); >> >> ticketlock_api_test(&local_ticket_lock); >> ticketlock_api_test(&per_thread_mem->per_thread_ticketlock); >> @@ -365,14 +365,14 @@ static void >> rwlock_api_test(odp_rwlock_t *rw_lock) >> >> static void *rwlock_api_tests(void *arg UNUSED) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> odp_rwlock_t local_rwlock; >> >> per_thread_mem = thread_init(); >> - global_mem = per_thread_mem->global_mem; >> + mem = per_thread_mem->global_mem; >> >> - odp_barrier_wait(&global_mem->global_barrier); >> + odp_barrier_wait(&mem->global_barrier); >> >> rwlock_api_test(&local_rwlock); >> rwlock_api_test(&per_thread_mem->per_thread_rwlock); >> @@ -384,17 +384,17 @@ static void *rwlock_api_tests(void >> *arg UNUSED) >> >> static void *no_lock_functional_test(void *arg UNUSED) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> uint32_t thread_num, resync_cnt, rs_idx, >> iterations, cnt; >> uint32_t sync_failures, current_errs, >> lock_owner_delay; >> >> thread_num = odp_cpu_id() + 1; >> per_thread_mem = thread_init(); >> - global_mem = per_thread_mem->global_mem; >> - iterations = global_mem->g_iterations; >> + mem = per_thread_mem->global_mem; >> + iterations = mem->g_iterations; >> >> - odp_barrier_wait(&global_mem->global_barrier); >> + odp_barrier_wait(&mem->global_barrier); >> >> sync_failures = 0; >> current_errs = 0; >> @@ -403,20 +403,20 @@ static void >> *no_lock_functional_test(void *arg UNUSED) >> lock_owner_delay = BASE_DELAY; >> >> for (cnt = 1; cnt <= iterations; cnt++) { >> - global_mem->global_lock_owner = thread_num; >> + mem->global_lock_owner = thread_num; >> odp_sync_stores(); >> thread_delay(per_thread_mem, lock_owner_delay); >> >> - if (global_mem->global_lock_owner != >> thread_num) { >> + if (mem->global_lock_owner != thread_num) { >> current_errs++; >> sync_failures++; >> } >> >> - global_mem->global_lock_owner = 0; >> + mem->global_lock_owner = 0; >> odp_sync_stores(); >> thread_delay(per_thread_mem, MIN_DELAY); >> >> - if (global_mem->global_lock_owner == >> thread_num) { >> + if (mem->global_lock_owner == thread_num) { >> current_errs++; >> sync_failures++; >> } >> @@ -430,10 +430,10 @@ static void >> *no_lock_functional_test(void *arg UNUSED) >> /* Try to resync all of the threads to >> increase contention */ >> if ((rs_idx < NUM_RESYNC_BARRIERS) && >> ((cnt % resync_cnt) == (resync_cnt - 1))) >> - odp_barrier_wait(&global_mem->barrier_array[rs_idx++]); >> + odp_barrier_wait(&mem->barrier_array[rs_idx++]); >> } >> >> - if (global_mem->g_verbose) >> + if (mem->g_verbose) >> printf("\nThread %u (id=%d core=%d) had %u >> sync_failures" >> " in %u iterations\n", thread_num, >> per_thread_mem->thread_id, >> @@ -454,7 +454,7 @@ static void >> *no_lock_functional_test(void *arg UNUSED) >> >> static void *spinlock_functional_test(void *arg UNUSED) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> uint32_t thread_num, resync_cnt, rs_idx, >> iterations, cnt; >> uint32_t sync_failures, is_locked_errs, current_errs; >> @@ -462,10 +462,10 @@ static void >> *spinlock_functional_test(void *arg UNUSED) >> >> thread_num = odp_cpu_id() + 1; >> per_thread_mem = thread_init(); >> - global_mem = per_thread_mem->global_mem; >> - iterations = global_mem->g_iterations; >> + mem = per_thread_mem->global_mem; >> + iterations = mem->g_iterations; >> >> - odp_barrier_wait(&global_mem->global_barrier); >> + odp_barrier_wait(&mem->global_barrier); >> >> sync_failures = 0; >> is_locked_errs = 0; >> @@ -476,13 +476,13 @@ static void >> *spinlock_functional_test(void *arg UNUSED) >> >> for (cnt = 1; cnt <= iterations; cnt++) { >> /* Acquire the shared global lock */ >> - odp_spinlock_lock(&global_mem->global_spinlock); >> + odp_spinlock_lock(&mem->global_spinlock); >> >> /* Make sure we have the lock AND didn't >> previously own it */ >> - if >> (odp_spinlock_is_locked(&global_mem->global_spinlock) != 1) >> + if >> (odp_spinlock_is_locked(&mem->global_spinlock) != 1) >> is_locked_errs++; >> >> - if (global_mem->global_lock_owner != 0) { >> + if (mem->global_lock_owner != 0) { >> current_errs++; >> sync_failures++; >> } >> @@ -491,19 +491,19 @@ static void >> *spinlock_functional_test(void *arg UNUSED) >> * then we see if anyone else has snuck in >> and changed the >> * global_lock_owner to be themselves >> */ >> - global_mem->global_lock_owner = thread_num; >> + mem->global_lock_owner = thread_num; >> odp_sync_stores(); >> thread_delay(per_thread_mem, lock_owner_delay); >> - if (global_mem->global_lock_owner != >> thread_num) { >> + if (mem->global_lock_owner != thread_num) { >> current_errs++; >> sync_failures++; >> } >> >> /* Release shared lock, and make sure we >> no longer have it */ >> - global_mem->global_lock_owner = 0; >> + mem->global_lock_owner = 0; >> odp_sync_stores(); >> - odp_spinlock_unlock(&global_mem->global_spinlock); >> - if (global_mem->global_lock_owner == >> thread_num) { >> + odp_spinlock_unlock(&mem->global_spinlock); >> + if (mem->global_lock_owner == thread_num) { >> current_errs++; >> sync_failures++; >> } >> @@ -517,10 +517,10 @@ static void >> *spinlock_functional_test(void *arg UNUSED) >> /* Try to resync all of the threads to >> increase contention */ >> if ((rs_idx < NUM_RESYNC_BARRIERS) && >> ((cnt % resync_cnt) == (resync_cnt - 1))) >> - odp_barrier_wait(&global_mem->barrier_array[rs_idx++]); >> + odp_barrier_wait(&mem->barrier_array[rs_idx++]); >> } >> >> - if ((global_mem->g_verbose) && >> + if ((mem->g_verbose) && >> ((sync_failures != 0) || (is_locked_errs != 0))) >> printf("\nThread %u (id=%d core=%d) had %u >> sync_failures" >> " and %u is_locked_errs in %u >> iterations\n", thread_num, >> @@ -537,7 +537,7 @@ static void >> *spinlock_functional_test(void *arg UNUSED) >> >> static void *ticketlock_functional_test(void *arg UNUSED) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> uint32_t thread_num, resync_cnt, rs_idx, >> iterations, cnt; >> uint32_t sync_failures, is_locked_errs, current_errs; >> @@ -545,11 +545,11 @@ static void >> *ticketlock_functional_test(void *arg UNUSED) >> >> thread_num = odp_cpu_id() + 1; >> per_thread_mem = thread_init(); >> - global_mem = per_thread_mem->global_mem; >> - iterations = global_mem->g_iterations; >> + mem = per_thread_mem->global_mem; >> + iterations = mem->g_iterations; >> >> /* Wait here until all of the threads have also >> reached this point */ >> - odp_barrier_wait(&global_mem->global_barrier); >> + odp_barrier_wait(&mem->global_barrier); >> >> sync_failures = 0; >> is_locked_errs = 0; >> @@ -560,14 +560,14 @@ static void >> *ticketlock_functional_test(void *arg UNUSED) >> >> for (cnt = 1; cnt <= iterations; cnt++) { >> /* Acquire the shared global lock */ >> - odp_ticketlock_lock(&global_mem->global_ticketlock); >> + odp_ticketlock_lock(&mem->global_ticketlock); >> >> /* Make sure we have the lock AND didn't >> previously own it */ >> - if >> (odp_ticketlock_is_locked(&global_mem->global_ticketlock) >> + if >> (odp_ticketlock_is_locked(&mem->global_ticketlock) >> != 1) >> is_locked_errs++; >> >> - if (global_mem->global_lock_owner != 0) { >> + if (mem->global_lock_owner != 0) { >> current_errs++; >> sync_failures++; >> } >> @@ -576,19 +576,19 @@ static void >> *ticketlock_functional_test(void *arg UNUSED) >> * then we see if anyone else has snuck in >> and changed the >> * global_lock_owner to be themselves >> */ >> - global_mem->global_lock_owner = thread_num; >> + mem->global_lock_owner = thread_num; >> odp_sync_stores(); >> thread_delay(per_thread_mem, lock_owner_delay); >> - if (global_mem->global_lock_owner != >> thread_num) { >> + if (mem->global_lock_owner != thread_num) { >> current_errs++; >> sync_failures++; >> } >> >> /* Release shared lock, and make sure we >> no longer have it */ >> - global_mem->global_lock_owner = 0; >> + mem->global_lock_owner = 0; >> odp_sync_stores(); >> - odp_ticketlock_unlock(&global_mem->global_ticketlock); >> - if (global_mem->global_lock_owner == >> thread_num) { >> + odp_ticketlock_unlock(&mem->global_ticketlock); >> + if (mem->global_lock_owner == thread_num) { >> current_errs++; >> sync_failures++; >> } >> @@ -602,10 +602,10 @@ static void >> *ticketlock_functional_test(void *arg UNUSED) >> /* Try to resync all of the threads to >> increase contention */ >> if ((rs_idx < NUM_RESYNC_BARRIERS) && >> ((cnt % resync_cnt) == (resync_cnt - 1))) >> - odp_barrier_wait(&global_mem->barrier_array[rs_idx++]); >> + odp_barrier_wait(&mem->barrier_array[rs_idx++]); >> } >> >> - if ((global_mem->g_verbose) && >> + if ((mem->g_verbose) && >> ((sync_failures != 0) || (is_locked_errs != 0))) >> printf("\nThread %u (id=%d core=%d) had %u >> sync_failures" >> " and %u is_locked_errs in %u >> iterations\n", thread_num, >> @@ -622,18 +622,18 @@ static void >> *ticketlock_functional_test(void *arg UNUSED) >> >> static void *rwlock_functional_test(void *arg UNUSED) >> { >> - global_shared_mem_t *global_mem; >> + global_shared_mem_t *mem; >> per_thread_mem_t *per_thread_mem; >> uint32_t thread_num, resync_cnt, rs_idx, >> iterations, cnt; >> uint32_t sync_failures, current_errs, >> lock_owner_delay; >> >> thread_num = odp_cpu_id() + 1; >> per_thread_mem = thread_init(); >> - global_mem = per_thread_mem->global_mem; >> - iterations = global_mem->g_iterations; >> + mem = per_thread_mem->global_mem; >> + iterations = mem->g_iterations; >> >> /* Wait here until all of the threads have also >> reached this point */ >> - odp_barrier_wait(&global_mem->global_barrier); >> + odp_barrier_wait(&mem->global_barrier); >> >> sync_failures = 0; >> current_errs = 0; >> @@ -643,10 +643,10 @@ static void >> *rwlock_functional_test(void *arg UNUSED) >> >> for (cnt = 1; cnt <= iterations; cnt++) { >> /* Acquire the shared global lock */ >> - odp_rwlock_write_lock(&global_mem->global_rwlock); >> + odp_rwlock_write_lock(&mem->global_rwlock); >> >> /* Make sure we have lock now AND didn't >> previously own it */ >> - if (global_mem->global_lock_owner != 0) { >> + if (mem->global_lock_owner != 0) { >> current_errs++; >> sync_failures++; >> } >> @@ -655,19 +655,19 @@ static void >> *rwlock_functional_test(void *arg UNUSED) >> * then we see if anyone else has snuck in >> and changed the >> * global_lock_owner to be themselves >> */ >> - global_mem->global_lock_owner = thread_num; >> + mem->global_lock_owner = thread_num; >> odp_sync_stores(); >> thread_delay(per_thread_mem, lock_owner_delay); >> - if (global_mem->global_lock_owner != >> thread_num) { >> + if (mem->global_lock_owner != thread_num) { >> current_errs++; >> sync_failures++; >> } >> >> /* Release shared lock, and make sure we >> no longer have it */ >> - global_mem->global_lock_owner = 0; >> + mem->global_lock_owner = 0; >> odp_sync_stores(); >> - odp_rwlock_write_unlock(&global_mem->global_rwlock); >> - if (global_mem->global_lock_owner == >> thread_num) { >> + odp_rwlock_write_unlock(&mem->global_rwlock); >> + if (mem->global_lock_owner == thread_num) { >> current_errs++; >> sync_failures++; >> } >> @@ -681,10 +681,10 @@ static void >> *rwlock_functional_test(void *arg UNUSED) >> /* Try to resync all of the threads to >> increase contention */ >> if ((rs_idx < NUM_RESYNC_BARRIERS) && >> ((cnt % resync_cnt) == (resync_cnt - 1))) >> - odp_barrier_wait(&global_mem->barrier_array[rs_idx++]); >> + odp_barrier_wait(&mem->barrier_array[rs_idx++]); >> } >> >> - if ((global_mem->g_verbose) && (sync_failures != 0)) >> + if ((mem->g_verbose) && (sync_failures != 0)) >> printf("\nThread %u (id=%d core=%d) had %u >> sync_failures" >> " in %u iterations\n", thread_num, >> per_thread_mem->thread_id, >> -- >> 1.9.1 >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] <mailto:[email protected]> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] <mailto:[email protected]> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
