alfred: Allow out-of-order TXEND
cover alfred: Allow out-of-order TXEND
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&from=1478782587039&to=1478902771655
[2] https://stats.freifunk-vogtland.net/dashboard/db/node?var-node=10feedb73eb2&from=1478902641027&to=1478938850519
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(-)
PATCH 1/7 alfred: Avoid hash search for transaction cleanup
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.
--- alfred.h | 3 --- recv.c | 16 +--------------- 2 files changed, 1 insertion(+), 18 deletions(-)
alfred.h +0 -3
@@ -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 */
recv.c +1 -15
@@ -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);
Signature · 1 line
2.10.2
PATCH 2/7 alfred: Move tx finish functionality in extra function
In reply to Sven EckelmannThe 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.
--- recv.c | 67 ++++++++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 39 insertions(+), 28 deletions(-)
recv.c +39 -28
@@ -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 ; }
Signature · 1 line
2.10.2
PATCH 3/7 alfred: Remove checks for committed/dropped transaction
In reply to Sven EckelmannTransactions 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.
--- recv.c | 8 -------- 1 file changed, 8 deletions(-)
recv.c +0 -8
@@ -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) {
Signature · 1 line
2.10.2
PATCH 4/7 alfred: Don't force cleanup of transaction on TXEND
In reply to Sven EckelmannA 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.
--- alfred.h | 2 +- recv.c | 13 +++++-------- 2 files changed, 6 insertions(+), 9 deletions(-)
alfred.h +1 -1
@@ -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;
recv.c +5 -8
@@ -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);
Signature · 1 line
2.10.2
PATCH 5/7 alfred: Use expected packet count to finished transactions
In reply to Sven EckelmannThe 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.
--- alfred.h | 8 +++++++- recv.c | 11 +++++------ unix_sock.c | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-)
alfred.h +7 -1
@@ -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,
recv.c +5 -6
@@ -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 ; }
unix_sock.c +1 -1
@@ -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);
Signature · 1 line
2.10.2
PATCH 6/7 alfred: Allow PUSH_DATA to finish transactions
In reply to Sven EckelmannA 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.
--- recv.c | 2 ++ 1 file changed, 2 insertions(+)
recv.c +2 -0
@@ -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 ;
Signature · 1 line
2.10.2
PATCH 7/7 alfred: Allow TXEND to start new transaction
In reply to Sven EckelmannA 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.
--- recv.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
recv.c +17 -2
@@ -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,
Signature · 1 line
2.10.2
Quoted 21 lines · CET Sven Eckelmann
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_DATApacket received with seqno 0 received
-> new transaction was created
3. transaction timeout happened
-> transaction was dropped
Quoted 6 lines
This patchset now changes the behavior to automatically create transactions when TXEND packets are received. The routines to decode TXEND and
PUSH_DATApackets 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