Skip to content

Commit

Permalink
Merge #18620 #19296
Browse files Browse the repository at this point in the history 
18620: core: add core_mutex_debug to aid debugging deadlocks r=benpicco a=maribu

### Contribution description

Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in
on log messages such as

    [mutex] waiting for thread 1 (pc = 0x800024d)

being added whenever `mutex_lock()` blocks. This makes tracing down
deadlocks easier.

### Testing procedure

Run e.g.

```sh
USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test
```

which should provide output such as

```
Welcome to pyterm!
Type '/exit' to exit.
READY
s
[mutex] waiting for thread 1 (pc = 0x8000f35)
START
main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug)
Test Application for mutex_cancel / mutex_lock_cancelable
=========================================================

Test without cancellation: OK
Test early cancellation: OK
Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d)
OK
Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0)
OK
TEST PASSED
```

```sh
$ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 
0x0800024d
/home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51
```

### Issues/PRs references

Depends on and includes #18619

19296: nanocoap: allow to define CoAP resources as XFA r=benpicco a=benpicco



Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>
Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
  • Loading branch information
@bors @benpicco
4 people authored Apr 25, 2023
3 parents 8b58e55 + 3e86e39 + 9495dc4 commit 8aaca17
Show file tree
Hide file tree
Showing 15 changed files with 284 additions and 32 deletions.
3 changes: 3 additions & 0 deletions core/Kconfig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ config MODULE_CORE_MSG_BUS
help
Messaging Bus API for inter process message broadcast.

config MODULE_CORE_MUTEX_DEBUG
bool "Aid debugging deadlocks by printing on whom mutex_lock() is waiting"

config MODULE_CORE_MUTEX_PRIORITY_INHERITANCE
bool "Use priority inheritance to mitigate priority inversion for mutexes"

Expand Down
35 changes: 34 additions & 1 deletion core/include/mutex.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,25 @@
* `MUTEX_LOCK`.
* - The scheduler is run, so that if the unblocked waiting thread can
* run now, in case it has a higher priority than the running thread.
*
* Debugging deadlocks
* -------------------
*
* The module `core_mutex_debug` can be used to print on whom `mutex_lock()`
* is waiting. This information includes the thread ID of the owner and the
* program counter (PC) from where `mutex_lock()` was called. Note that the
* information is only valid if:
*
* - The mutex was locked by a thread, and not e.g. by `MUTEX_INIT_LOCKED`
* - The function `cpu_get_caller_pc()` is implemented for the target
* architecture. (The thread ID will remain valid, though.)
* - The caller PC is briefly 0 when the current owner passes over ownership
* to the next thread, but that thread didn't get CPU time yet to write its
* PC into the data structure. Even worse, on architectures where an aligned
* function-pointer-sized write is not atomic, the value may briefly be
* bogus. Chances are close to zero this ever hits and since this only
* effects debug output, the ostrich algorithm was chosen here.
*
* @{
*
* @file
Expand All @@ -112,6 +131,7 @@
#include <stdint.h>
#include <stdbool.h>

#include "architecture.h"
#include "kernel_defines.h"
#include "list.h"
#include "thread.h"
Expand All @@ -130,7 +150,8 @@ typedef struct {
* @internal
*/
list_node_t queue;
#if defined(DOXYGEN) || defined(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE)
#if defined(DOXYGEN) || defined(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE) \
|| defined(MODULE_CORE_MUTEX_DEBUG)
/**
* @brief The current owner of the mutex or `NULL`
* @note Only available if module core_mutex_priority_inheritance
Expand All @@ -141,6 +162,18 @@ typedef struct {
* this will have the value of `NULL`.
*/
kernel_pid_t owner;
#endif
#if defined(DOXYGEN) || defined(MODULE_CORE_MUTEX_DEBUG)
/**
* @brief Program counter of the call to @ref mutex_lock that most
* recently acquired this mutex
*
* This is used when the module `core_mutex_debug` is used to debug
* deadlocks and is non-existing otherwise
*/
uinttxtptr_t owner_calling_pc;
#endif
#if defined(DOXYGEN) || defined(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE)
/**
* @brief Original priority of the owner
* @note Only available if module core_mutex_priority_inheritance
Expand Down
49 changes: 42 additions & 7 deletions core/mutex.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,18 @@
* function into both @ref mutex_lock and @ref mutex_lock_cancelable is,
* therefore, beneficial for the majority of applications.
*/
static inline __attribute__((always_inline)) void _block(mutex_t *mutex,
unsigned irq_state)
static inline __attribute__((always_inline))
void _block(mutex_t *mutex,
unsigned irq_state,
uinttxtptr_t pc)
{
/* pc is only used when MODULE_CORE_MUTEX_DEBUG */
(void)pc;
#if IS_USED(MODULE_CORE_MUTEX_DEBUG)
printf("[mutex] waiting for thread %" PRIkernel_pid " (pc = 0x%" PRIxTXTPTR
")\n",
mutex->owner, mutex->owner_calling_pc);
#endif
thread_t *me = thread_get_active();

/* Fail visibly even if a blocking action is called from somewhere where
Expand Down Expand Up @@ -80,10 +89,17 @@ static inline __attribute__((always_inline)) void _block(mutex_t *mutex,
irq_restore(irq_state);
thread_yield_higher();
/* We were woken up by scheduler. Waker removed us from queue. */
#if IS_USED(MODULE_CORE_MUTEX_DEBUG)
mutex->owner_calling_pc = pc;
#endif
}

bool mutex_lock_internal(mutex_t *mutex, bool block)
{
uinttxtptr_t pc = 0;
#if IS_USED(MODULE_CORE_MUTEX_DEBUG)
pc = cpu_get_caller_pc();
#endif
unsigned irq_state = irq_disable();

DEBUG("PID[%" PRIkernel_pid "] mutex_lock_internal(block=%u).\n",
Expand All @@ -92,9 +108,15 @@ bool mutex_lock_internal(mutex_t *mutex, bool block)
if (mutex->queue.next == NULL) {
/* mutex is unlocked. */
mutex->queue.next = MUTEX_LOCKED;
#ifdef MODULE_CORE_MUTEX_PRIORITY_INHERITANCE
#if IS_USED(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE) \
|| IS_USED(MODULE_CORE_MUTEX_DEBUG)
thread_t *me = thread_get_active();
mutex->owner = me->pid;
#endif
#if IS_USED(MODULE_CORE_MUTEX_DEBUG)
mutex->owner_calling_pc = pc;
#endif
#if IS_USED(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE)
mutex->owner_original_priority = me->priority;
#endif
DEBUG("PID[%" PRIkernel_pid "] mutex_lock(): early out.\n",
Expand All @@ -106,14 +128,18 @@ bool mutex_lock_internal(mutex_t *mutex, bool block)
irq_restore(irq_state);
return false;
}
_block(mutex, irq_state);
_block(mutex, irq_state, pc);
}

return true;
}

int mutex_lock_cancelable(mutex_cancel_t *mc)
{
uinttxtptr_t pc = 0;
#if IS_USED(MODULE_CORE_MUTEX_DEBUG)
pc = cpu_get_caller_pc();
#endif
unsigned irq_state = irq_disable();

DEBUG("PID[%" PRIkernel_pid "] mutex_lock_cancelable()\n",
Expand All @@ -131,9 +157,15 @@ int mutex_lock_cancelable(mutex_cancel_t *mc)
if (mutex->queue.next == NULL) {
/* mutex is unlocked. */
mutex->queue.next = MUTEX_LOCKED;
#ifdef MODULE_CORE_MUTEX_PRIORITY_INHERITANCE
#if IS_USED(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE) \
|| IS_USED(MODULE_CORE_MUTEX_DEBUG)
thread_t *me = thread_get_active();
mutex->owner = me->pid;
#endif
#if IS_USED(MODULE_CORE_MUTEX_DEBUG)
mutex->owner_calling_pc = pc;
#endif
#if IS_USED(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE)
mutex->owner_original_priority = me->priority;
#endif
DEBUG("PID[%" PRIkernel_pid "] mutex_lock_cancelable() early out.\n",
Expand All @@ -142,7 +174,7 @@ int mutex_lock_cancelable(mutex_cancel_t *mc)
return 0;
}
else {
_block(mutex, irq_state);
_block(mutex, irq_state, pc);
if (mc->cancelled) {
DEBUG("PID[%" PRIkernel_pid "] mutex_lock_cancelable() "
"cancelled.\n", thread_getpid());
Expand Down Expand Up @@ -185,7 +217,7 @@ void mutex_unlock(mutex_t *mutex)

uint16_t process_priority = process->priority;

#ifdef MODULE_CORE_MUTEX_PRIORITY_INHERITANCE
#if IS_USED(MODULE_CORE_MUTEX_PRIORITY_INHERITANCE)
thread_t *owner = thread_get(mutex->owner);
if ((owner) && (owner->priority != mutex->owner_original_priority)) {
DEBUG("PID[%" PRIkernel_pid "] prio %u --> %u\n",
Expand All @@ -194,6 +226,9 @@ void mutex_unlock(mutex_t *mutex)
sched_change_priority(owner, mutex->owner_original_priority);
}
#endif
#if IS_USED(MODULE_CORE_MUTEX_DEBUG)
mutex->owner_calling_pc = 0;
#endif

irq_restore(irqstate);
sched_switch(process_priority);
Expand Down
18 changes: 18 additions & 0 deletions examples/nanocoap_server/Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ USEMODULE += sock_udp
USEMODULE += gnrc_icmpv6_echo

USEMODULE += nanocoap_sock
USEMODULE += nanocoap_resources

USEMODULE += xtimer

Expand All @@ -42,6 +43,23 @@ ifneq (,$(filter $(BOARD),$(LOW_MEMORY_BOARDS)))
USEMODULE += prng_minstd
endif

# Enable fileserver for boards with plenty of memory
HIGH_MEMORY_BOARDS := native same54-xpro mcb2388

ifneq (,$(filter $(BOARD),$(HIGH_MEMORY_BOARDS)))
USEMODULE += gcoap_fileserver
USEMODULE += vfs_default

# we don't want to run GCoAP
CFLAGS += -DCONFIG_GCOAP_NO_AUTO_INIT=1
CFLAGS += -DCONFIG_NANOCOAP_SERVER_WELL_KNOWN_CORE=1

# always enable auto-format for native
ifeq ($(BOARD),native)
USEMODULE += vfs_auto_format
endif
endif

# Change this to 0 show compiler invocation lines by default:
QUIET ?= 1

Expand Down
42 changes: 33 additions & 9 deletions examples/nanocoap_server/coap_handler.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,14 +166,38 @@ ssize_t _sha256_handler(coap_pkt_t* pkt, uint8_t *buf, size_t len, coap_request_
return pkt_pos - (uint8_t*)pkt->hdr;
}

/* must be sorted by path (ASCII order) */
const coap_resource_t coap_resources[] = {
COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER,
{ "/echo/", COAP_GET | COAP_MATCH_SUBTREE, _echo_handler, NULL },
{ "/riot/board", COAP_GET, _riot_board_handler, NULL },
{ "/riot/value", COAP_GET | COAP_PUT | COAP_POST, _riot_value_handler, NULL },
{ "/riot/ver", COAP_GET, _riot_block2_handler, NULL },
{ "/sha256", COAP_POST, _sha256_handler, NULL },
NANOCOAP_RESOURCE(echo) {
.path = "/echo/", .methods = COAP_GET | COAP_MATCH_SUBTREE, .handler = _echo_handler
};
NANOCOAP_RESOURCE(board) {
.path = "/riot/board", .methods = COAP_GET, .handler = _riot_board_handler
};
NANOCOAP_RESOURCE(value) {
.path = "/riot/value", .methods = COAP_GET | COAP_PUT | COAP_POST, .handler = _riot_value_handler
};
NANOCOAP_RESOURCE(ver) {
.path = "/riot/ver", .methods = COAP_GET, .handler = _riot_block2_handler
};
NANOCOAP_RESOURCE(sha256) {
.path = "/sha256", .methods = COAP_POST, .handler = _sha256_handler
};

const unsigned coap_resources_numof = ARRAY_SIZE(coap_resources);
/* we can also include the fileserver module */
#ifdef MODULE_GCOAP_FILESERVER
#include "net/gcoap/fileserver.h"
#include "vfs_default.h"

NANOCOAP_RESOURCE(fileserver) {
.path = "/vfs",
.methods = COAP_GET
#if IS_USED(MODULE_GCOAP_FILESERVER_PUT)
| COAP_PUT
#endif
#if IS_USED(MODULE_GCOAP_FILESERVER_DELETE)
| COAP_DELETE
#endif
| COAP_MATCH_SUBTREE,
.handler = gcoap_fileserver_handler,
.context = VFS_DEFAULT_DATA
};
#endif
9 changes: 9 additions & 0 deletions sys/Makefile.dep
View file
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,15 @@ ifneq (,$(filter nanocoap_dtls,$(USEMODULE)))
USEPKG += tinydtls
endif

ifneq (,$(filter nanocoap_server_auto_init,$(USEMODULE)))
USEMODULE += nanocoap_server
endif

ifneq (,$(filter nanocoap_server,$(USEMODULE)))
USEMODULE += nanocoap_resources
USEMODULE += nanocoap_sock
endif

ifneq (,$(filter nanocoap_sock,$(USEMODULE)))
USEMODULE += sock_udp
USEMODULE += sock_util
Expand Down
4 changes: 4 additions & 0 deletions sys/auto_init/auto_init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ extern void gcoap_init(void);
AUTO_INIT(gcoap_init,
AUTO_INIT_PRIO_MOD_GCOAP);
#endif
#if IS_USED(MODULE_NANOCOAP_SERVER_AUTO_INIT)
extern void auto_init_nanocoap_server(void);
AUTO_INIT(auto_init_nanocoap_server, AUTO_INIT_PRIO_MOD_NANOCOAP);
#endif
#if IS_USED(MODULE_DEVFS)
extern void auto_init_devfs(void);
AUTO_INIT(auto_init_devfs,
Expand Down
6 changes: 6 additions & 0 deletions sys/auto_init/include/auto_init_priorities.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,12 @@ extern "C" {
#define AUTO_INIT_PRIO_MOD_UWB_CORE 1230
#endif
#ifndef AUTO_INIT_PRIO_MOD_GCOAP
/**
* @brief nanoCoAP server priority
*/
#define AUTO_INIT_PRIO_MOD_NANOCOAP 1235
#endif
#ifndef AUTO_INIT_PRIO_MOD_GCOAP
/**
* @brief GCoAP priority
*/
Expand Down
26 changes: 26 additions & 0 deletions sys/include/net/nanocoap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
#include "bitfield.h"
#include "byteorder.h"
#include "iolist.h"
#include "macros/utils.h"
#include "net/coap.h"
#else
#include "coap.h"
Expand All @@ -102,6 +103,10 @@
typedef void sock_udp_ep_t;
#endif

#if defined(MODULE_NANOCOAP_RESOURCES)
#include "xfa.h"
#endif

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -402,15 +407,29 @@ typedef struct {
uint8_t *opt; /**< Pointer to the placed option */
} coap_block_slicer_t;

#if defined(MODULE_NANOCOAP_RESOURCES) || DOXYGEN
/**
* @brief CoAP XFA resource entry
*
* @param name internal name of the resource entry, must be unique
*/
#define NANOCOAP_RESOURCE(name) \
XFA_CONST(coap_resources_xfa, 0) coap_resource_t CONCAT(coap_resource_, name) =
#else
/**
* @brief Global CoAP resource list
* @deprecated Use @ref NANOCOAP_RESOURCE instead.
* The function will be removed after the 2024.01 release.
*/
extern const coap_resource_t coap_resources[];

/**
* @brief Number of entries in global CoAP resource list
* @deprecated Use @ref NANOCOAP_RESOURCE instead.
* The function will be removed after the 2024.01 release.
*/
extern const unsigned coap_resources_numof;
#endif

/**
* @name Functions -- Header Read/Write
Expand Down Expand Up @@ -2048,6 +2067,13 @@ extern ssize_t coap_well_known_core_default_handler(coap_pkt_t *pkt, \
coap_request_ctx_t *context);
/**@}*/

/**
* @brief Respond to `/.well-known/core` to list all resources on the server
*/
#ifndef CONFIG_NANOCOAP_SERVER_WELL_KNOWN_CORE
#define CONFIG_NANOCOAP_SERVER_WELL_KNOWN_CORE !IS_USED(MODULE_GCOAP)
#endif

/**
* @brief Checks if a CoAP resource path matches a given URI
*
Expand Down
Loading

0 comments on commit 8aaca17

Please sign in to comment.