Skip to content

mutex:move mutex holder to struct mutex_s.#7426

Closed
XinStellaris wants to merge 1 commit into
apache:masterfrom
XinStellaris:trace_mutex_holder
Closed

mutex:move mutex holder to struct mutex_s.#7426
XinStellaris wants to merge 1 commit into
apache:masterfrom
XinStellaris:trace_mutex_holder

Conversation

@XinStellaris

Copy link
Copy Markdown
Contributor

Signed-off-by: 田昕 tianxin7@xiaomi.com

Summary

We move the mutex holder to struct mutex_s so that we can trace who holds the mutex when deadlock happens.
We also print possible mutex holder in sched_dumpstack.

Impact

mutex_t and related code.

Testing

Tested on ESP32C3

@XinStellaris

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 @liguiding please review

@XinStellaris

Copy link
Copy Markdown
Contributor Author

I am not sure whether we should print mutex holder in sched_dumpstack.
It may be helpful when we lack coredump or gdb.

Comment thread libs/libc/sched/sched_dumpstack.c Outdated
@GUIDINGLI

Copy link
Copy Markdown
Contributor

I am not sure whether we should print mutex holder in sched_dumpstack. It may be helpful when we lack coredump or gdb.

In a real device and there is no JTAG (mainly for the device just sold out), this method will be helpful for debugging

@pkarashchenko

Copy link
Copy Markdown
Contributor

This change reminded me #6376 (comment) discussion.

Comment thread libs/libc/sched/sched_dumpstack.c Outdated
* for debugging only.
*/

if (tid >= 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not move to ps and crash dump?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmd_ps seems a good place to go.
What do you mean by crash dump? board_crashdump?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. arm_dumpstate

Comment thread libs/libc/sched/sched_dumpstack.c Outdated
if (tid >= 0)
{
tcb = nxsched_get_tcb(tid);
if (tcb != NULL && tcb->task_state == TSTATE_WAIT_SEM)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not all semaphore have holder field.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a matter of fact, I don't know how to tell the difference.
There is no way to differentiate waiting on a sem or on a mutex.
So I just print possible mutex holder anyway.

What's your suggestion?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's check the wait object. If semaphore has PRIOINHERIT_FLAGS_ENABLE flag then most probably it is a mutex and we can upcast it to mutex_t. That is one of the possible variants

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it can't handle the project which doesn't enable priority inheritance.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we design an internal function like:
int _nxsem_wait(FAR sem_t *sem, bool is_mutex)

Then we can differentiate mutex and sem.
After that, we can add a new TSTATE_WAIT_MUTEX, then we know if a task is waiting on mutex, and we can print the mutex holder in such case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An other approach could add a new flag in sem_t::flags to indicate whether is mutex or sem

Comment thread libs/libc/sched/sched_dumpstack.c Outdated
Comment thread include/nuttx/mutex.h Outdated
Comment thread include/nuttx/mutex.h
Comment thread include/nuttx/mutex.h
Comment thread include/nuttx/mutex.h
Comment thread include/nuttx/mutex.h

@pkarashchenko pkarashchenko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we need to design and implement a separate kernel object and cleanup priority inheritance from semaphore

@XinStellaris XinStellaris force-pushed the trace_mutex_holder branch 2 times, most recently from 84f8778 to 12aad4e Compare October 27, 2022 07:32
@XinStellaris

Copy link
Copy Markdown
Contributor Author

@anjiahao1 please review this

So that we can trace who holds the mutex when deadlock happens.

Signed-off-by: 田昕 <tianxin7@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants