gnrc_tcp: improvement: Replace tcbs msg queue with mbox #6969
Conversation
799a33d to
c1e23e2
Compare
sys/include/net/gnrc/tcp/tcb.h
Outdated
| @@ -75,8 +76,8 @@ typedef struct _transmission_control_block { | |||
| xtimer_t tim_tout; /**< Timer struct for timeouts */ | |||
| msg_t msg_tout; /**< Message, sent on timeouts */ | |||
| gnrc_pktsnip_t *pkt_retransmit; /**< Pointer to packet in "retransmit queue" */ | |||
| kernel_pid_t owner; /**< PID of this connection handling thread */ | |||
| msg_t msg_queue[GNRC_TCP_TCB_MSG_QUEUE_SIZE]; /**< TCB message queue */ | |||
| msg_t mbox_raw[GNRC_TCP_TCB_MSG_QUEUE_SIZE]; /**< TCB message queue */ | |||
There was a problem hiding this comment.
update comment and MACRO to match usage of mbox, its no message queue (in that sense) anymore 😉
There was a problem hiding this comment.
Will do in the evening
| { | ||
| msg_t msg; | ||
| msg.type = ((cb_arg_t *) arg)->msg_type; | ||
| mbox_try_put(((cb_arg_t *) arg)->mbox_ptr, &msg); |
There was a problem hiding this comment.
Is this safe? This will access tcb->mbox without a mutex locked, or not?
There was a problem hiding this comment.
I am assuming that its safe, whats the point of having a synchronization utility that can't be used concurrently?
There was a problem hiding this comment.
And those functions are only called on timer expiry. The setup and teardown of the timers are enclosed in a mutex protected sections.
There was a problem hiding this comment.
mhm, right - but somehow I feel you should pass the whole tcb to this function, and check its state before putting the message. For instance, I'm unsure (you are the expert) what happens if the connection was reset or aborted in the meantime; that is: before the timer fires and this callback is processed?
There was a problem hiding this comment.
The only thing that does this is placing a message in a mbox (actually it would be a nice function for xtimer ;) ). Upon message retrieval the TCB state is checked before an action is taken. It should be safe but I check for your concerns later.
There was a problem hiding this comment.
I didn't dig any deeper into this, just some quick thoughts - likely I'm wrong, but that's why I'm asking you 😄
There was a problem hiding this comment.
I'll look into this. Its better to double check than overlook a hidden race condition ;)
There was a problem hiding this comment.
Okay checked the usage of the callback function, it seems safe to me. On timer expiration the connection should be terminated in most cases anyway so the current state is mostly irrelevant. It seems fine to me :)
363f82f to
516a964
Compare
|
please squash |
516a964 to
42e15ed
Compare
|
Done |
GNRC TCP improvement:
Currently the GNRC TCP API functions assign a message queue to a calling thread to store messages for TCP operation. This assignment is problematic in cases where the caller uses a message queue outside to the TCP context.
To solve this issue, I replaced the message queue with an mbox stored in the TCB.