New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

platform: fix mem_trace trace level guard #5614

Merged
merged 1 commit into from Dec 29, 2017

Conversation

Projects
None yet
8 participants
@maciejbocianski
Member

maciejbocianski commented Nov 29, 2017

Description

This change fix "trace inside trace" issue in mem_trace module
It prevents from tracing malloc/free calls inside __wrap__realloc_r/__wrap__calloc_r/SUB_REALLOC/SUB_CALLOC

Status

READY

Migrations

Change mbed_mem_trace module API
add functions: mbed_mem_trace_level_incr mbed_mem_trace_level_decr

YES

@0xc0170 0xc0170 requested review from deepikabhavnani and c1728p9 Nov 29, 2017

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:mem_trace_fix branch Nov 29, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 29, 2017

This change fix "trace inside trace" issue in mem_trace module

Was this reported anywhere? Can you please amend your commit to add more details how is this fixing the issue and why are we introducing new API - how it should be used?

@0xc0170 0xc0170 added needs: work and removed needs: review labels Nov 29, 2017

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Nov 29, 2017

Problem was discovered during mbed_drivers-mem_trace test refactoring.
As I understand (and according description in mbed_mem_trace.c) we don't want to trace nested calls of malloc/free inside realloc/calloc - current implementation doesn't work.
Maybe I'm wrong ?

mbed_mem_trace.c:

/* 'trace_level' guards "trace inside trace" situations (for example, the implementation
 * of realloc() might call malloc() internally, and since malloc() is also traced, this could
 * result in two calls to the callback function instead of one. */
static uint8_t trace_level;
@bulislaw

This comment has been minimized.

Member

bulislaw commented Dec 1, 2017

Could be change the nomenclature used to lock/unlock? And it can be binary, I don't think we need levels really.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:mem_trace_fix branch 2 times, most recently to 5500f5d Dec 4, 2017

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Dec 4, 2017

@bulislaw updated

@0xc0170 0xc0170 added needs: review and removed needs: work labels Dec 4, 2017

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Dec 7, 2017

ping

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 7, 2017

I do not fully understand this fix. trave_level (typo there? should be trace_level) was implemented internally to prevent this.

Why do we need API to expose locking? It can not be hidden, internally done. Is this expected from a user to use everytime mem_trace API is invoked? I can see that we as a caller, we use mem_trace_mutex above, why this is done on top of mutex (it's invoked before the mutex).

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Dec 7, 2017

Why do we need API to expose locking? It can not be hidden, internally done. Is this expected from a user to use everytime mem_trace API is invoked? I can see that we as a caller, we use mem_trace_mutex above, why this is done on top of mutex (it's invoked before the mutex).

mem_trace_mutex protects only single trace callback call
When we enter realloc we don't want to trace it's internal calls of malloc/free this is why we lock trace_lock_count at the function beginning.
Without this lock realloc could report also malloc/free trace

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 7, 2017

How previously that level variable inside was failing to prevent this ?

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Dec 7, 2017

Previously it was protecting mem_trace_cb call only.
And since it's impossible to call mem_trace_cb recursively it was protecting nothing and was redundancy to mem_trace_mutex

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 11, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 11, 2017

Build : SUCCESS

Build number : 675
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5614/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@@ -91,6 +91,9 @@ extern "C" {
extern "C" void * __wrap__malloc_r(struct _reent * r, size_t size) {
void *ptr = NULL;
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();

This comment has been minimized.

@c1728p9

c1728p9 Dec 11, 2017

Contributor

The memory trace mutex should be held here, otherwise another thread my have acquired this lock but not the mutex, causing logging on other threads to be dropped. Could mbed_mem_trace_lock() include mem_trace_mutex->lock() to simply this?

This comment has been minimized.

@maciejbocianski

maciejbocianski Dec 12, 2017

Member

I mistakenly assumed that whole __wrap__malloc_r is protected by __malloc_lock but actually __real__malloc_r is protected

This comment has been minimized.

@maciejbocianski

maciejbocianski Dec 12, 2017

Member

Sure we could encapsulate mem_trace_mutex inside mbed_mem_trace.c module but since it's C file we have to use raw OS mutex.
Other solution could be changing this module to cpp and use SingletonPtr<PlatformMutex>

This comment has been minimized.

@maciejbocianski

maciejbocianski Dec 15, 2017

Member

I'm not sure which solution will be acceptable?

static uint8_t trace_level;
static uint8_t trace_lock_count;
#define TRACE_IS_LOCKED() (trace_lock_count > 1)

This comment has been minimized.

@c1728p9

c1728p9 Dec 11, 2017

Contributor

This returns false even if lock count is 1. Maybe a different name, such as TRACE_FIRST_LOCK would be more appropriate.

void mbed_mem_trace_lock()
{
mem_trace_mutex->lock();
core_util_atomic_incr_u8(&trace_lock_count, 1);

This comment has been minimized.

@c1728p9

c1728p9 Dec 18, 2017

Contributor

nitpick - this doesn't need to be atomically incremented since this is protected by the mutex.

This comment has been minimized.

@maciejbocianski

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:mem_trace_fix branch from 923f32d to d78d8c0 Dec 19, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 19, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 19, 2017

Build : SUCCESS

Build number : 719
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5614/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 19, 2017

/morph test

@mbed-ci

This comment has been minimized.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:mem_trace_fix branch from d78d8c0 to 1e04d19 Dec 20, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Dec 20, 2017

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:mem_trace_fix branch from 1e04d19 to 5c417d0 Dec 21, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 22, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 22, 2017

Build : SUCCESS

Build number : 754
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5614/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:mem_trace_fix branch from 5c417d0 to aa22ef1 Dec 23, 2017

@cmonr

This comment has been minimized.

Contributor

cmonr commented Dec 28, 2017

Restarting builds. Results were not reported.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 28, 2017

Build : SUCCESS

Build number : 765
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5614/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Dec 28, 2017

Restarting uVisor test due to odd failure with K64F.

/morph uvisor-test

@mbed-ci

This comment has been minimized.

@adbridge adbridge merged commit c4211e1 into ARMmbed:master Dec 29, 2017

17 checks passed

AWS-CI uVisor Build & Test Verification build successful.
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment