Skip to content

Commit a7d5ac9

Browse files
elenaf9elenaf9
committed
fixup! gnrc/netapi/netnotify: ACK notify event directly
Use mutex, counter and condvar to collect the exact count of acknowledgements for a notify event. With this, we can now block for all replies after dispatching to all receivers and after releasing the lock on netreg. This a) maintains the receivers priority order and b) prevents possible deadlocks that could happen if we hold the netreg lock while blocking for an ACK.
1 parent bee053b commit a7d5ac9

File tree

5 files changed

+70
-55
lines changed

5 files changed

+70
-55
lines changed

‎sys/include/net/gnrc/netapi/notify.h‎

Lines changed: 37 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,9 @@
2626
*
2727
* @note Notification events are associated with type-dependent event data.
2828
* Receivers should not access the data directly, but instead use the
29-
* the dedicated API functions.
30-
* If the data is read directly, the receiver is responsible for setting
31-
* the @ref NETAPI_NOTIFY_FLAG_ACK flag for the sending thread.
32-
*
29+
* the dedicated API functions to copy the data from the sender's.
30+
* If the data is read manually, the receiver is responsible for
31+
* calling @ref gnrc_netapi_notify_ack to unblock the sender.
3332
*
3433
* @author Elena Frank <[email protected]>
3534
*/
@@ -39,23 +38,15 @@ extern "C" {
3938
#endif
4039

4140
#include "net/ipv6/addr.h"
42-
#include "sched.h"
43-
44-
/**
45-
* @brief Thread flag for acknowledging a received notify even.
46-
*
47-
* @note The flag should only be set after all event data was copied
48-
* by receiver and can be freed by the sender.
49-
*/
50-
#define NETAPI_NOTIFY_FLAG_ACK (1u << 10)
41+
#include "cond.h"
42+
#include "mutex.h"
5143

5244
/**
5345
* @brief Definition of notification event types in the network stack.
5446
*
5547
* @note Expand at will.
5648
* If event data is associated with the type, a helper function must also
57-
* be added for parsing the data and setting the @ref NETAPI_NOTIFY_FLAG_ACK
58-
* on the sender.
49+
* be added for parsing the data and calling @ref gnrc_netapi_notify_ack.
5950
*/
6051
typedef enum {
6152
NETAPI_NOTIFY_L2_CONNECTED, /**< Connection established on layer 2. */
@@ -64,13 +55,23 @@ typedef enum {
6455
NETAPI_NOTIFY_L3_UNREACHABLE, /**< Node became unreachable on the network layer. */
6556
} netapi_notify_t;
6657

58+
/**
59+
* @brief Data structure to acknowledge netapi notification events.
60+
*/
61+
typedef struct {
62+
int counter; /**< ACK counter */
63+
cond_t cond; /**< condition variable to signal change in count */
64+
mutex_t lock; /**< lock for counter */
65+
} gnrc_netapi_notify_ack_t;
66+
6767
/**
6868
* @brief Data structure to be sent for netapi notification events.
6969
*/
7070
typedef struct {
71-
netapi_notify_t event; /**< the type of event */
72-
void *_data; /**< associated event data. */
73-
uint16_t _data_len; /**< size of the event data */
71+
netapi_notify_t event; /**< the type of event */
72+
void *_data; /**< associated event data. */
73+
uint16_t _data_len; /**< size of the event data */
74+
gnrc_netapi_notify_ack_t *ack; /**< acknowledge event */
7475
} gnrc_netapi_notify_t;
7576

7677
/**
@@ -83,13 +84,26 @@ typedef struct {
8384
kernel_pid_t if_pid; /**< PID of network interface */
8485
} netapi_notify_l2_connection_t;
8586

87+
/**
88+
* @brief Acknowledge that a notify event was received and its data read.
89+
*
90+
* @param[in] ack Pointer to the event's acknowledgment structure.
91+
*/
92+
static inline void gnrc_netapi_notify_ack(gnrc_netapi_notify_ack_t *ack)
93+
{
94+
mutex_lock(&ack->lock);
95+
ack->counter++;
96+
/* Signal that the counter changed. */
97+
cond_signal(&ack->cond);
98+
mutex_unlock(&ack->lock);
99+
}
100+
86101
/**
87102
* @brief Parse the connection event data associated with @ref NETAPI_NOTIFY_L2_CONNECTED
88103
* and @ref NETAPI_NOTIFY_L2_DISCONNECTED events.
89104
*
90-
* @note This will set the @ref NETAPI_NOTIFY_FLAG_ACK for the sending thread.
105+
* @note This will call @ref gnrc_netapi_notify_ack.
91106
*
92-
* @param[in] sender_pid PID of the netapi sender.
93107
* @param[in] notify Pointer to the received notify event.
94108
* @param[out] data Connection data received in the @p notify event.
95109
* data.l2addr_len should be set to the size of the l2addr buffer.
@@ -99,24 +113,21 @@ typedef struct {
99113
* @retval -ENOBUFS if the length of l2addr in @p data is smaller than the
100114
* received l2addr.
101115
*/
102-
uint8_t gnrc_netapi_notify_copy_l2_connection_data(kernel_pid_t sender_pid,
103-
gnrc_netapi_notify_t *notify,
116+
uint8_t gnrc_netapi_notify_copy_l2_connection_data(gnrc_netapi_notify_t *notify,
104117
netapi_notify_l2_connection_t *data);
105118
/**
106119
* @brief Parse the ipv6 address associated with @ref NETAPI_NOTIFY_L3_DISCOVERED and
107120
* @ref NETAPI_NOTIFY_L3_UNREACHABLE events.
108121
*
109-
* @note This will set the @ref NETAPI_NOTIFY_FLAG_ACK for the sending thread.
122+
* @note This will call @ref gnrc_netapi_notify_ack.
110123
*
111-
* @param[in] sender_pid PID of the netapi sender.
112124
* @param[in] notify Pointer to the received notify event.
113125
* @param[out] addr IPv6 address of the remote.
114126
*
115127
* @retval sizeof(ipv6_addr_t) on success.
116128
* @retval -EINVAL if @p notify is of a wrong @ref netapi_notify_t type.
117129
*/
118-
int gnrc_netapi_notify_copy_l3_address(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify,
119-
ipv6_addr_t *addr);
130+
int gnrc_netapi_notify_copy_l3_address(gnrc_netapi_notify_t *notify, ipv6_addr_t *addr);
120131

121132
#ifdef __cplusplus
122133
}

‎sys/net/gnrc/netapi/gnrc_netapi.c‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@
2727
#include "net/gnrc/netapi/notify.h"
2828
#include "net/gnrc/pktbuf.h"
2929
#include "net/gnrc/netapi.h"
30-
#include "thread.h"
30+
#include "cond.h"
31+
#include "mutex.h"
3132

3233
#define ENABLE_DEBUG 0
3334
#include "debug.h"
@@ -152,10 +153,16 @@ int gnrc_netapi_dispatch(gnrc_nettype_t type, uint32_t demux_ctx,
152153
int gnrc_netapi_notify(gnrc_nettype_t type, uint32_t demux_ctx, netapi_notify_t event,
153154
void *data, size_t data_len)
154155
{
156+
gnrc_netapi_notify_ack_t ack = {
157+
.counter = 0,
158+
.cond = COND_INIT,
159+
.lock = MUTEX_INIT,
160+
};
155161
gnrc_netapi_notify_t notify = {
156162
.event = event,
157163
._data = data,
158164
._data_len = data_len,
165+
.ack = &ack
159166
};
160167

161168
gnrc_netreg_acquire_shared();
@@ -166,24 +173,27 @@ int gnrc_netapi_notify(gnrc_nettype_t type, uint32_t demux_ctx, netapi_notify_t
166173
/* Look up the registered threads for this message type. */
167174
gnrc_netreg_entry_t *sendto = gnrc_netreg_lookup(type, demux_ctx);
168175

169-
/* Make sure the ACK flag isn't already set. */
170-
thread_flags_clear(NETAPI_NOTIFY_FLAG_ACK);
171-
172176
/* Dispatch to all registered threads sequentially. */
173177
while (sendto) {
174-
uint32_t status = _dispatch_single(sendto, GNRC_NETAPI_MSG_TYPE_NOTIFY, &notify);
175-
176-
/* Need to wait for an ACK to ensure that the data was read and that
177-
there won't be any dangling pointers after the function returned. */
178-
if (data != NULL && status == 0) {
179-
thread_flags_wait_all(NETAPI_NOTIFY_FLAG_ACK);
178+
int status = _dispatch_single(sendto, GNRC_NETAPI_MSG_TYPE_NOTIFY, &notify);
179+
if (status < 0) {
180+
numof--;
180181
}
181-
182182
sendto = gnrc_netreg_getnext(sendto);
183183
}
184184
}
185185

186186
gnrc_netreg_release_shared();
187187

188+
if (data != NULL) {
189+
/* Wait for ACK from all receivers to ensure that the data was read and
190+
that there won't be any dangling pointers after the function returned. */
191+
mutex_lock(&ack.lock);
192+
while (ack.counter < numof) {
193+
cond_wait(&ack.cond, &ack.lock);
194+
}
195+
mutex_unlock(&ack.lock);
196+
}
197+
188198
return numof;
189199
}

‎sys/net/gnrc/netapi/notify/gnrc_netapi_notify.c‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,8 @@
1616

1717
#include "net/gnrc/netapi/notify.h"
1818
#include "net/ipv6/addr.h"
19-
#include "sched.h"
20-
#include "thread.h"
21-
#include <sys/errno.h>
2219

23-
uint8_t gnrc_netapi_notify_copy_l2_connection_data(kernel_pid_t sender_pid,
24-
gnrc_netapi_notify_t *notify,
20+
uint8_t gnrc_netapi_notify_copy_l2_connection_data(gnrc_netapi_notify_t *notify,
2521
netapi_notify_l2_connection_t *data)
2622
{
2723
int data_len;
@@ -51,14 +47,13 @@ uint8_t gnrc_netapi_notify_copy_l2_connection_data(kernel_pid_t sender_pid,
5147
break;
5248
}
5349

54-
/* Unblock the sending thread now that all data was copied over. */
55-
thread_flags_set(thread_get(sender_pid), NETAPI_NOTIFY_FLAG_ACK);
50+
/* Acknowledge the read data */
51+
gnrc_netapi_notify_ack(notify->ack);
5652

5753
return data_len;
5854
}
5955

60-
int gnrc_netapi_notify_copy_l3_address(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify,
61-
ipv6_addr_t *addr)
56+
int gnrc_netapi_notify_copy_l3_address(gnrc_netapi_notify_t *notify, ipv6_addr_t *addr)
6257
{
6358
int data_len;
6459

@@ -77,8 +72,8 @@ int gnrc_netapi_notify_copy_l3_address(kernel_pid_t sender_pid, gnrc_netapi_noti
7772
break;
7873
}
7974

80-
/* Unblock the sending thread now that all data was copied over. */
81-
thread_flags_set(thread_get(sender_pid), NETAPI_NOTIFY_FLAG_ACK);
75+
/* Acknowledge the read data */
76+
gnrc_netapi_notify_ack(notify->ack);
8277

8378
return data_len;
8479
}

‎sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ static inline void _on_l2_disconnected(kernel_pid_t if_pid, uint8_t *l2addr, uin
241241
*
242242
* @param[in] notify The type of notification.
243243
*/
244-
static inline void _netapi_notify_event(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify)
244+
static inline void _netapi_notify_event(gnrc_netapi_notify_t *notify)
245245
{
246246
uint8_t l2addr[CONFIG_GNRC_IPV6_NIB_L2ADDR_MAX_LEN];
247247
netapi_notify_l2_connection_t data = {
@@ -251,7 +251,7 @@ static inline void _netapi_notify_event(kernel_pid_t sender_pid, gnrc_netapi_not
251251
};
252252
netapi_notify_t type = notify->event;
253253

254-
if (gnrc_netapi_notify_copy_l2_connection_data(sender_pid, notify, &data) <= 0) {
254+
if (gnrc_netapi_notify_copy_l2_connection_data(notify, &data) <= 0) {
255255
DEBUG("ipv6: invalid data on netapi notify event.\n");
256256
return;
257257
}
@@ -320,7 +320,7 @@ static void *_event_loop(void *args)
320320
break;
321321
case GNRC_NETAPI_MSG_TYPE_NOTIFY:
322322
DEBUG("ipv6: GNRC_NETAPI_MSG_TYPE_NOTIFY received\n");
323-
_netapi_notify_event(msg.sender_pid, msg.content.ptr);
323+
_netapi_notify_event(msg.content.ptr);
324324
break;
325325

326326
#ifdef MODULE_GNRC_IPV6_EXT_FRAG

‎sys/net/gnrc/routing/rpl/gnrc_rpl.c‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -314,13 +314,12 @@ static inline void _handle_unreachable_neighbor(ipv6_addr_t *addr)
314314
*
315315
* @param[in] notify The type of notification.
316316
*/
317-
static inline void _netapi_notify_event(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify)
317+
static inline void _netapi_notify_event(gnrc_netapi_notify_t *notify)
318318
{
319319
ipv6_addr_t neigh_addr;
320320
netapi_notify_t type = notify->event;
321321

322-
if (gnrc_netapi_notify_copy_l3_address(sender_pid, notify,
323-
&neigh_addr) != sizeof(ipv6_addr_t)) {
322+
if (gnrc_netapi_notify_copy_l3_address(notify, &neigh_addr) != sizeof(ipv6_addr_t)) {
324323
DEBUG("RPL: Received invalid data for netapi notify event.\n");
325324
return;
326325
}
@@ -408,7 +407,7 @@ static void *_event_loop(void *args)
408407
break;
409408
case GNRC_NETAPI_MSG_TYPE_NOTIFY:
410409
DEBUG("RPL: GNRC_NETAPI_MSG_TYPE_NOTIFY received\n");
411-
_netapi_notify_event(msg.sender_pid, msg.content.ptr);
410+
_netapi_notify_event(msg.content.ptr);
412411
break;
413412
default:
414413
break;

0 commit comments

Comments
 (0)