[B.A.T.M.A.N.] [PATCH 0/7] alfred: Allow out-of-order TXEND

[B.A.T.M.A.N.] [PATCH 0/7] alfred: Allow out-of-order TXEND

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

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(-)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[B.A.T.M.A.N.] [PATCH 1/7] alfred: Avoid hash search for transaction cleanup

[Sven Eckelmann]

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);
-- 
2.10.2

[B.A.T.M.A.N.] [PATCH 2/7] alfred: Move tx finish functionality in extra function

[Sven Eckelmann]

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;
 }
-- 
2.10.2

[B.A.T.M.A.N.] [PATCH 3/7] alfred: Remove checks for committed/dropped transaction

[Sven Eckelmann]

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) {
-- 
2.10.2

[B.A.T.M.A.N.] [PATCH 4/7] alfred: Don't force cleanup of transaction on TXEND

[Sven Eckelmann]

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);
-- 
2.10.2

[B.A.T.M.A.N.] [PATCH 5/7] alfred: Use expected packet count to finished transactions

[Sven Eckelmann]

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);
-- 
2.10.2

[B.A.T.M.A.N.] [PATCH 6/7] alfred: Allow PUSH_DATA to finish transactions

[Sven Eckelmann]

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;
-- 
2.10.2

[B.A.T.M.A.N.] [PATCH 7/7] alfred: Allow TXEND to start new transaction

[Sven Eckelmann]

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,
-- 
2.10.2

Re: [B.A.T.M.A.N.] [PATCH 0/7] alfred: Allow out-of-order TXEND

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

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:
> 
> 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.

Adopted this series in commits 323c1ed..12a478c.

Thanks,
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]