diff --git a/sys/include/net/gnrc/rpl.h b/sys/include/net/gnrc/rpl.h index 06b7dd3863..8a494dd29e 100644 --- a/sys/include/net/gnrc/rpl.h +++ b/sys/include/net/gnrc/rpl.h @@ -540,7 +540,17 @@ extern const ipv6_addr_t ipv6_addr_all_rpl_nodes; #ifdef MODULE_NETSTATS_RPL /** - * @brief Statistics for RPL control messages + * @brief Statistics for RPL control messages + * @warning Access to this structure need to be synchronized with the RPL + * thread to avoid reading corrupted valued. The RPL thread will + * disable IRQs while updating the fields. A reader that disables + * IRQs while reading values from here will read back correct and + * consistent values. + * @details Note that on 32 bit platforms reading individual fields usually is + * safe without locking, as a 32 bit read typically is an atomic + * operation. However, reasoning about e.g. the relation between TX + * packets and TX bytes will still require IRQs disabled, as the stats + * could be updated between the two reads even on 32 bit platforms. */ extern netstats_rpl_t gnrc_rpl_netstats; #endif diff --git a/sys/net/gnrc/routing/rpl/gnrc_rpl_internal/netstats.h b/sys/net/gnrc/routing/rpl/gnrc_rpl_internal/netstats.h index 7645afde12..c0d781f674 100644 --- a/sys/net/gnrc/routing/rpl/gnrc_rpl_internal/netstats.h +++ b/sys/net/gnrc/routing/rpl/gnrc_rpl_internal/netstats.h @@ -38,6 +38,9 @@ extern "C" { */ static inline void gnrc_rpl_netstats_rx_DIO(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dio.rx_mcast_count++; netstats->dio.rx_mcast_bytes += len; @@ -46,6 +49,7 @@ static inline void gnrc_rpl_netstats_rx_DIO(netstats_rpl_t *netstats, size_t len netstats->dio.rx_ucast_count++; netstats->dio.rx_ucast_bytes += len; } + irq_restore(irq_state); } /** @@ -57,6 +61,9 @@ static inline void gnrc_rpl_netstats_rx_DIO(netstats_rpl_t *netstats, size_t len */ static inline void gnrc_rpl_netstats_tx_DIO(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dio.tx_mcast_count++; netstats->dio.tx_mcast_bytes += len; @@ -65,6 +72,7 @@ static inline void gnrc_rpl_netstats_tx_DIO(netstats_rpl_t *netstats, size_t len netstats->dio.tx_ucast_count++; netstats->dio.tx_ucast_bytes += len; } + irq_restore(irq_state); } /** @@ -76,6 +84,9 @@ static inline void gnrc_rpl_netstats_tx_DIO(netstats_rpl_t *netstats, size_t len */ static inline void gnrc_rpl_netstats_rx_DIS(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dis.rx_mcast_count++; netstats->dis.rx_mcast_bytes += len; @@ -84,6 +95,7 @@ static inline void gnrc_rpl_netstats_rx_DIS(netstats_rpl_t *netstats, size_t len netstats->dis.rx_ucast_count++; netstats->dis.rx_ucast_bytes += len; } + irq_restore(irq_state); } /** @@ -95,6 +107,9 @@ static inline void gnrc_rpl_netstats_rx_DIS(netstats_rpl_t *netstats, size_t len */ static inline void gnrc_rpl_netstats_tx_DIS(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dis.tx_mcast_count++; netstats->dis.tx_mcast_bytes += len; @@ -103,6 +118,7 @@ static inline void gnrc_rpl_netstats_tx_DIS(netstats_rpl_t *netstats, size_t len netstats->dis.tx_ucast_count++; netstats->dis.tx_ucast_bytes += len; } + irq_restore(irq_state); } /** @@ -114,6 +130,9 @@ static inline void gnrc_rpl_netstats_tx_DIS(netstats_rpl_t *netstats, size_t len */ static inline void gnrc_rpl_netstats_rx_DAO(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dao.rx_mcast_count++; netstats->dao.rx_mcast_bytes += len; @@ -122,6 +141,7 @@ static inline void gnrc_rpl_netstats_rx_DAO(netstats_rpl_t *netstats, size_t len netstats->dao.rx_ucast_count++; netstats->dao.rx_ucast_bytes += len; } + irq_restore(irq_state); } /** @@ -133,6 +153,9 @@ static inline void gnrc_rpl_netstats_rx_DAO(netstats_rpl_t *netstats, size_t len */ static inline void gnrc_rpl_netstats_tx_DAO(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dao.tx_mcast_count++; netstats->dao.tx_mcast_bytes += len; @@ -141,6 +164,7 @@ static inline void gnrc_rpl_netstats_tx_DAO(netstats_rpl_t *netstats, size_t len netstats->dao.tx_ucast_count++; netstats->dao.tx_ucast_bytes += len; } + irq_restore(irq_state); } /** @@ -152,6 +176,9 @@ static inline void gnrc_rpl_netstats_tx_DAO(netstats_rpl_t *netstats, size_t len */ static inline void gnrc_rpl_netstats_rx_DAO_ACK(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dao_ack.rx_mcast_count++; netstats->dao_ack.rx_mcast_bytes += len; @@ -160,6 +187,7 @@ static inline void gnrc_rpl_netstats_rx_DAO_ACK(netstats_rpl_t *netstats, size_t netstats->dao_ack.rx_ucast_count++; netstats->dao_ack.rx_ucast_bytes += len; } + irq_restore(irq_state); } /** @@ -171,6 +199,9 @@ static inline void gnrc_rpl_netstats_rx_DAO_ACK(netstats_rpl_t *netstats, size_t */ static inline void gnrc_rpl_netstats_tx_DAO_ACK(netstats_rpl_t *netstats, size_t len, int cast) { + /* other threads (e.g. the shell thread via the rpl shell command) read + * out the data, so we have to sync accesses */ + unsigned irq_state = irq_disable(); if (cast == GNRC_RPL_NETSTATS_MULTICAST) { netstats->dao_ack.tx_mcast_count++; netstats->dao_ack.tx_mcast_bytes += len; @@ -179,6 +210,7 @@ static inline void gnrc_rpl_netstats_tx_DAO_ACK(netstats_rpl_t *netstats, size_t netstats->dao_ack.tx_ucast_count++; netstats->dao_ack.tx_ucast_bytes += len; } + irq_restore(irq_state); } #ifdef __cplusplus diff --git a/sys/shell/commands/sc_gnrc_rpl.c b/sys/shell/commands/sc_gnrc_rpl.c index 801a7f3546..26262959a3 100644 --- a/sys/shell/commands/sc_gnrc_rpl.c +++ b/sys/shell/commands/sc_gnrc_rpl.c @@ -206,12 +206,38 @@ int _gnrc_rpl_send_dis(void) #ifdef MODULE_NETSTATS_RPL static void _print_stats_block(netstats_rpl_block_t *block, const char *name) { + /* In the following we need to sync with the RPL thread via disabling IRQs + * to avoid reading corrupted data. The simpler strategy would be to + * disable IRQs during the whole printing (so in _stats()_, but stdio could + * be via a slow UART and, hence, have severe impact on the real time + * capabilities of the system. The second and simplest strategy would be to + * memcpy the whole netstats_rpl_t on to the stack with IRQs disabled and + * print the stack copy with IRQs re-enabled. However, that structure is + * 128 B in size, so that we would easily provoke a stack-overflow. + * + * Our strategy instead is to read the data four values at a time with + * IRQs disabled, and print them with IRQs re-enabled. The disadvantage is + * that stats may get updated while printing one group of values. However, + * the stats are grouped such that closely related values are read together. + * Hence, related metrics will always refer to the same state of the stats. + */ + unsigned irq_state = irq_disable(); + uint32_t rx_ucast = block->rx_ucast_count; + uint32_t tx_ucast = block->tx_ucast_count; + uint32_t rx_mcast = block->rx_mcast_count; + uint32_t tx_mcast = block->tx_mcast_count; + irq_restore(irq_state); printf("%7s #packets: %10" PRIu32 " / %-10" PRIu32 " %10" PRIu32 " / %-10" PRIu32 "\n", - name, block->rx_ucast_count, block->tx_ucast_count, - block->rx_mcast_count, block->tx_mcast_count); + name, rx_ucast, tx_ucast, rx_mcast, tx_mcast); + + irq_state = irq_disable(); + rx_ucast = block->rx_ucast_bytes; + tx_ucast = block->tx_ucast_bytes; + rx_mcast = block->rx_mcast_bytes; + tx_mcast = block->tx_mcast_bytes; + irq_restore(irq_state); printf("%7s #bytes: %10" PRIu32 " / %-10" PRIu32 " %10" PRIu32 " / %-10" PRIu32 "\n", - name, block->rx_ucast_bytes, block->tx_ucast_bytes, - block->rx_mcast_bytes, block->tx_mcast_bytes); + name, rx_ucast, tx_ucast, rx_mcast, tx_mcast); } int _stats(void)