Hi,
alfred allows to have out-of-order push data packet delivery. But these push data packets are only accepted when they finish with an TXEND that has as its seqno the amount of packets alfred received for this transaction. A TXEND packet which is received before the last push data frame was received would therefore cause the complete transaction to be canceled on the receiver side.
It was observed on a TP-Link TL-WR841N/ND v8 [1] that after the upgrade to Gluon 2016.2 many (if not the most) transaction were out of order. The TXEND was received before the actual push data was submitted. The master server reacted to this by:
1. TXEND packet received to unknown transaction with seqno != 0 -> TXEND packet was dropped 2. PUSH_DATA packet received with seqno 0 received -> new transaction was created 3. transaction timeout happened -> transaction was dropped
This patchset now changes the behavior to automatically create transactions when TXEND packets are received. The routines to decode TXEND and PUSH_DATA packets will check if all frames of a transaction were received and commit the data when the check was successful.
This stabilized the statistics [2] for the previously mentioned node.
Kind regards, Sven
[1] https://stats.freifunk-vogtland.net/dashboard/db/node?var-node=10feedb73eb2&... [2] https://stats.freifunk-vogtland.net/dashboard/db/node?var-node=10feedb73eb2&...
Sven Eckelmann (7): alfred: Avoid hash search for transaction cleanup alfred: Move tx finish functionality in extra function alfred: Remove checks for committed/dropped transaction alfred: Don't force cleanup of transaction on TXEND alfred: Use expected packet count to finished transactions alfred: Allow PUSH_DATA to finish transactions alfred: Allow TXEND to start new transaction
alfred.h | 11 +++++--- recv.c | 84 +++++++++++++++++++++++++++++++------------------------------ unix_sock.c | 2 +- 3 files changed, 51 insertions(+), 46 deletions(-)
A transaction can either be cleaned up on timeout or on TXEND. alfred has in both situations already the head of the transaction list. An extra search in the transaction hash is therefore not needed.
Signed-off-by: Sven Eckelmann sven@narfation.org --- alfred.h | 3 --- recv.c | 16 +--------------- 2 files changed, 1 insertion(+), 18 deletions(-)
diff --git a/alfred.h b/alfred.h index b2b08b0..7e7811b 100644 --- a/alfred.h +++ b/alfred.h @@ -159,9 +159,6 @@ int recv_alfred_packet(struct globals *globals, struct interface *interface, int recv_sock); struct transaction_head * transaction_add(struct globals *globals, struct ether_addr mac, uint16_t id); -struct transaction_head * -transaction_clean_hash(struct globals *globals, - struct transaction_head *search); struct transaction_head *transaction_clean(struct globals *globals, struct transaction_head *head); /* send.c */ diff --git a/recv.c b/recv.c index 98539cb..b6b6eab 100644 --- a/recv.c +++ b/recv.c @@ -160,18 +160,6 @@ struct transaction_head *transaction_clean(struct globals *globals, return head; }
-struct transaction_head * -transaction_clean_hash(struct globals *globals, struct transaction_head *search) -{ - struct transaction_head *head; - - head = hash_find(globals->transaction_hash, search); - if (!head) - return head; - - return transaction_clean(globals, head); -} - static int process_alfred_push_data(struct globals *globals, struct in6_addr *source, struct alfred_push_data_v0 *push) @@ -369,9 +357,7 @@ static int process_alfred_status_txend(struct globals *globals, free(transaction_packet); }
- head = transaction_clean_hash(globals, &search); - if (!head) - return -1; + transaction_clean(globals, head);
if (head->client_socket < 0) free(head);
The process_alfred_status_txend has two main functionalities. It first tries to find a transaction to be able to get its status. When it was found then it either commits the data or just cleans it up.
The latter will be used by later by process_alfred_push_data to implement out-of-order txend packets. Thus split process_alfred_status_txend now into two functions to avoid code duplication.
Signed-off-by: Sven Eckelmann sven@narfation.org --- recv.c | 67 ++++++++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/recv.c b/recv.c index b6b6eab..6878e5e 100644 --- a/recv.c +++ b/recv.c @@ -160,6 +160,44 @@ struct transaction_head *transaction_clean(struct globals *globals, return head; }
+static int finish_alfred_transaction(struct globals *globals, + struct transaction_head *head, + struct ether_addr mac, + uint16_t num_packets) +{ + struct transaction_packet *transaction_packet, *safe; + + /* this transaction was already finished/dropped */ + if (head->finished != 0) + return -1; + + /* missing packets -> cleanup everything */ + if (head->num_packet == num_packets) + head->finished = -1; + else + head->finished = 1; + + list_for_each_entry_safe(transaction_packet, safe, &head->packet_list, + list) { + if (head->finished == 1) + finish_alfred_push_data(globals, mac, + transaction_packet->push); + + list_del(&transaction_packet->list); + free(transaction_packet->push); + free(transaction_packet); + } + + transaction_clean(globals, head); + + if (head->client_socket < 0) + free(head); + else + unix_sock_req_data_finish(globals, head); + + return 1; +} + static int process_alfred_push_data(struct globals *globals, struct in6_addr *source, struct alfred_push_data_v0 *push) @@ -313,7 +351,6 @@ static int process_alfred_status_txend(struct globals *globals, struct alfred_status_v0 *request) { struct transaction_head search, *head; - struct transaction_packet *transaction_packet, *safe; struct ether_addr mac; int len, ret;
@@ -336,33 +373,7 @@ static int process_alfred_status_txend(struct globals *globals, if (!head) return -1;
- /* this transaction was already finished/dropped */ - if (head->finished != 0) - return -1; - - /* missing packets -> cleanup everything */ - if (head->num_packet != ntohs(request->tx.seqno)) - head->finished = -1; - else - head->finished = 1; - - list_for_each_entry_safe(transaction_packet, safe, &head->packet_list, - list) { - if (head->finished == 1) - finish_alfred_push_data(globals, mac, - transaction_packet->push); - - list_del(&transaction_packet->list); - free(transaction_packet->push); - free(transaction_packet); - } - - transaction_clean(globals, head); - - if (head->client_socket < 0) - free(head); - else - unix_sock_req_data_finish(globals, head); + finish_alfred_transaction(globals, head, mac, ntohs(request->tx.seqno));
return 0; }
Transactions will be removed from the transaction has when they were committed. Thus is is not necessary to check in the push_data/txend packet processing functions for committed/dropped transactions.
Signed-off-by: Sven Eckelmann sven@narfation.org --- recv.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/recv.c b/recv.c index 6878e5e..21ea539 100644 --- a/recv.c +++ b/recv.c @@ -167,10 +167,6 @@ static int finish_alfred_transaction(struct globals *globals, { struct transaction_packet *transaction_packet, *safe;
- /* this transaction was already finished/dropped */ - if (head->finished != 0) - return -1; - /* missing packets -> cleanup everything */ if (head->num_packet == num_packets) head->finished = -1; @@ -233,10 +229,6 @@ static int process_alfred_push_data(struct globals *globals, } clock_gettime(CLOCK_MONOTONIC, &head->last_rx_time);
- /* this transaction was already finished/dropped */ - if (head->finished != 0) - return -1; - found = 0; list_for_each_entry(transaction_packet, &head->packet_list, list) { if (transaction_packet->push->tx.seqno == push->tx.seqno) {
A committed transaction has always to be cleaned up/removed when the TXEND was received. But TXEND packets which are out-of-order may still be waiting for PUSH_DATA packets which will be receive later. Thus TXEND packets which don't trigger a commit of the PUSH_DATA should not automatically drop the transaction.
A transaction which doesn't get all PUSH_DATA packets will be timeout automatically and no extra handling is necessary.
Signed-off-by: Sven Eckelmann sven@narfation.org --- alfred.h | 2 +- recv.c | 13 +++++-------- 2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/alfred.h b/alfred.h index 7e7811b..9d0f6a6 100644 --- a/alfred.h +++ b/alfred.h @@ -71,7 +71,7 @@ struct transaction_head { struct ether_addr server_addr; uint16_t id; uint8_t requested_type; - int finished; + uint16_t finished; int num_packet; int client_socket; struct timespec last_rx_time; diff --git a/recv.c b/recv.c index 21ea539..dd0b021 100644 --- a/recv.c +++ b/recv.c @@ -167,17 +167,14 @@ static int finish_alfred_transaction(struct globals *globals, { struct transaction_packet *transaction_packet, *safe;
- /* missing packets -> cleanup everything */ - if (head->num_packet == num_packets) - head->finished = -1; - else - head->finished = 1; + /* finish when all packets received */ + if (head->num_packet != num_packets) + return 0;
+ head->finished = 1; list_for_each_entry_safe(transaction_packet, safe, &head->packet_list, list) { - if (head->finished == 1) - finish_alfred_push_data(globals, mac, - transaction_packet->push); + finish_alfred_push_data(globals, mac, transaction_packet->push);
list_del(&transaction_packet->list); free(transaction_packet->push);
The TXEND packet will specify the number of PUSH_DATA packets are required to finish a transaction. This number has therefore to be stored when a TXEND packet is receiver to later be able to decide whether the transaction can be finished or not.
Signed-off-by: Sven Eckelmann sven@narfation.org --- alfred.h | 8 +++++++- recv.c | 11 +++++------ unix_sock.c | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/alfred.h b/alfred.h index 9d0f6a6..eb8200c 100644 --- a/alfred.h +++ b/alfred.h @@ -71,7 +71,7 @@ struct transaction_head { struct ether_addr server_addr; uint16_t id; uint8_t requested_type; - uint16_t finished; + uint16_t txend_packets; int num_packet; int client_socket; struct timespec last_rx_time; @@ -161,6 +161,12 @@ struct transaction_head * transaction_add(struct globals *globals, struct ether_addr mac, uint16_t id); struct transaction_head *transaction_clean(struct globals *globals, struct transaction_head *head); + +static inline bool transaction_finished(struct transaction_head *head) +{ + return head->txend_packets == head->num_packet; +} + /* send.c */ int push_data(struct globals *globals, struct interface *interface, struct in6_addr *destination, enum data_source max_source_level, diff --git a/recv.c b/recv.c index dd0b021..1f56016 100644 --- a/recv.c +++ b/recv.c @@ -131,7 +131,7 @@ transaction_add(struct globals *globals, struct ether_addr mac, uint16_t id) head->server_addr = mac; head->id = id; head->requested_type = 0; - head->finished = 0; + head->txend_packets = 0; head->num_packet = 0; head->client_socket = -1; clock_gettime(CLOCK_MONOTONIC, &head->last_rx_time); @@ -162,16 +162,14 @@ struct transaction_head *transaction_clean(struct globals *globals,
static int finish_alfred_transaction(struct globals *globals, struct transaction_head *head, - struct ether_addr mac, - uint16_t num_packets) + struct ether_addr mac) { struct transaction_packet *transaction_packet, *safe;
/* finish when all packets received */ - if (head->num_packet != num_packets) + if (!transaction_finished(head)) return 0;
- head->finished = 1; list_for_each_entry_safe(transaction_packet, safe, &head->packet_list, list) { finish_alfred_push_data(globals, mac, transaction_packet->push); @@ -362,7 +360,8 @@ static int process_alfred_status_txend(struct globals *globals, if (!head) return -1;
- finish_alfred_transaction(globals, head, mac, ntohs(request->tx.seqno)); + head->txend_packets = ntohs(request->tx.seqno); + finish_alfred_transaction(globals, head, mac);
return 0; } diff --git a/unix_sock.c b/unix_sock.c index 150ad32..edc7e0b 100644 --- a/unix_sock.c +++ b/unix_sock.c @@ -276,7 +276,7 @@ int unix_sock_req_data_finish(struct globals *globals, requested_type = head->requested_type; id = head->id; client_sock = head->client_socket; - if (head->finished != 1) + if (!transaction_finished(head)) send_data = 0;
free(head);
A TXEND packet which is received in the middle of a transaction cannot finish this transaction. This can only be done by a PUSH_DATA when the rest of the packets were received.
Signed-off-by: Sven Eckelmann sven@narfation.org --- recv.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/recv.c b/recv.c index 1f56016..9fabfa4 100644 --- a/recv.c +++ b/recv.c @@ -250,6 +250,8 @@ static int process_alfred_push_data(struct globals *globals, list_add_tail(&transaction_packet->list, &head->packet_list); head->num_packet++;
+ finish_alfred_transaction(globals, head, mac); + return 0; err: return -1;
A TXEND which comes before all other PUSH_DATA packets also has to create its own transaction. Otherwise it cannot store the number of expected packets for this transaction.
The PUSH_DATA packets can then trigger the finish of the transaction. Or the transaction will timeout automatically.
Signed-off-by: Sven Eckelmann sven@narfation.org --- recv.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/recv.c b/recv.c index 9fabfa4..bd208fd 100644 --- a/recv.c +++ b/recv.c @@ -359,13 +359,28 @@ static int process_alfred_status_txend(struct globals *globals, search.id = ntohs(request->tx.id);
head = hash_find(globals->transaction_hash, &search); - if (!head) - return -1; + if (!head) { + /* slave must create the transactions to be able to correctly + * wait for it */ + if (globals->opmode != OPMODE_MASTER) + goto err; + + /* 0-packet txend for unknown transaction */ + if (ntohs(request->tx.seqno) == 0) + goto err; + + head = transaction_add(globals, mac, ntohs(request->tx.id)); + if (!head) + goto err; + } + clock_gettime(CLOCK_MONOTONIC, &head->last_rx_time);
head->txend_packets = ntohs(request->tx.seqno); finish_alfred_transaction(globals, head, mac);
return 0; +err: + return -1; }
int recv_alfred_packet(struct globals *globals, struct interface *interface,
On Saturday, November 12, 2016 10:24:32 AM CET Sven Eckelmann wrote:
Hi,
alfred allows to have out-of-order push data packet delivery. But these push data packets are only accepted when they finish with an TXEND that has as its seqno the amount of packets alfred received for this transaction. A TXEND packet which is received before the last push data frame was received would therefore cause the complete transaction to be canceled on the receiver side.
It was observed on a TP-Link TL-WR841N/ND v8 [1] that after the upgrade to Gluon 2016.2 many (if not the most) transaction were out of order. The TXEND was received before the actual push data was submitted. The master server reacted to this by:
- TXEND packet received to unknown transaction with seqno != 0 -> TXEND packet was dropped
- PUSH_DATA packet received with seqno 0 received -> new transaction was created
- transaction timeout happened -> transaction was dropped
This patchset now changes the behavior to automatically create transactions when TXEND packets are received. The routines to decode TXEND and PUSH_DATA packets will check if all frames of a transaction were received and commit the data when the check was successful.
This stabilized the statistics [2] for the previously mentioned node.
Adopted this series in commits 323c1ed..12a478c.
Thanks, Simon
b.a.t.m.a.n@lists.open-mesh.org
-
Simon Wunderlich -
Sven Eckelmann