[B.A.T.M.A.N.] [PATCHv2 0/5] new feature: Speedy Join 2012-06-27

[B.A.T.M.A.N.] [PATCHv2 0/5] new feature: Speedy Join 2012-06-27

[Antonio Quartulli]

Hello people,

this is the second version of the Speedy Join feature.

Since version v1 I changed a few things:
- indentation and style has been fixed taking into consideration Sven's
  suggestions
- Old patch 4 has been removed. TT flags do not really need to be carry along
  with the tt_entry in a roaming_adv packet. (at least this is not needed for
  the purpose of this feature). A roaming client will be marked as ROAM on the
  new mesh node and therefore it will be already purged if nobody claims it.

Thank you,
	Antonio

[B.A.T.M.A.N.] [PATCHv2 1/4] batman-adv: add reference counting for type batadv_tt_orig_list_entry

[Antonio Quartulli]

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 translation-table.c |   16 ++++++++++++----
 types.h             |    1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/translation-table.c b/translation-table.c
index 245cc9a..e22f558 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -152,6 +152,8 @@ static void batadv_tt_orig_list_entry_free_rcu(struct rcu_head *rcu)
 static void
 batadv_tt_orig_list_entry_free_ref(struct batadv_tt_orig_list_entry *orig_entry)
 {
+	if (!atomic_dec_and_test(&orig_entry->refcount))
+		return;
 	/* to avoid race conditions, immediately decrease the tt counter */
 	atomic_dec(&orig_entry->orig_node->tt_size);
 	call_rcu(&orig_entry->rcu, batadv_tt_orig_list_entry_free_rcu);
@@ -639,12 +641,17 @@ batadv_tt_global_entry_has_orig(const struct batadv_tt_global_entry *entry,
 	rcu_read_lock();
 	head = &entry->orig_list;
 	hlist_for_each_entry_rcu(tmp_orig_entry, node, head, list) {
-		if (tmp_orig_entry->orig_node == orig_node) {
-			found = true;
-			break;
-		}
+		if (tmp_orig_entry->orig_node != orig_node)
+			continue;
+		if (!atomic_inc_not_zero(&tmp_orig_entry->refcount))
+			continue;
+
+		found = true;
+		batadv_tt_orig_list_entry_free_ref(tmp_orig_entry);
+		break;
 	}
 	rcu_read_unlock();
+
 	return found;
 }
 
@@ -663,6 +670,7 @@ batadv_tt_global_add_orig_entry(struct batadv_tt_global_entry *tt_global_entry,
 	atomic_inc(&orig_node->tt_size);
 	orig_entry->orig_node = orig_node;
 	orig_entry->ttvn = ttvn;
+	atomic_set(&orig_entry->refcount, 0);
 
 	spin_lock_bh(&tt_global_entry->list_lock);
 	hlist_add_head_rcu(&orig_entry->list,
diff --git a/types.h b/types.h
index 64b4317..82b97c3 100644
--- a/types.h
+++ b/types.h
@@ -281,6 +281,7 @@ struct batadv_tt_global_entry {
 struct batadv_tt_orig_list_entry {
 	struct batadv_orig_node *orig_node;
 	uint8_t ttvn;
+	atomic_t refcount;
 	struct rcu_head rcu;
 	struct hlist_node list;
 };
-- 
1.7.9.4

[B.A.T.M.A.N.] [PATCHv2 2/4] batman-adv: implement batadv_tt_global_entry_get_orig()

[Antonio Quartulli]

batadv_tt_global_entry_get_orig() searches the originator list associated to a
given tt_global_entry and possibly for the tt_orig_list_entry associated to the
orig_node passed as argument.

batadv_tt_global_entry_has_orig() has been modified in order to use the new
function and avoid code duplication. Now it also returns bool instead of int.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 translation-table.c |   35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/translation-table.c b/translation-table.c
index e22f558..bcbd9aa 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -626,17 +626,17 @@ static void batadv_tt_changes_list_free(struct batadv_priv *bat_priv)
 	spin_unlock_bh(&bat_priv->tt_changes_list_lock);
 }
 
-/* find out if an orig_node is already in the list of a tt_global_entry.
- * returns 1 if found, 0 otherwise
+/* find out if an orig_node is already in the list of a tt_global_entry and
+ * returns it with an increased refcounter if found, NULL otherwise.
+ *
  */
-static bool
-batadv_tt_global_entry_has_orig(const struct batadv_tt_global_entry *entry,
+static struct batadv_tt_orig_list_entry *
+batadv_tt_global_entry_get_orig(const struct batadv_tt_global_entry *entry,
 				const struct batadv_orig_node *orig_node)
 {
-	struct batadv_tt_orig_list_entry *tmp_orig_entry;
+	struct batadv_tt_orig_list_entry *tmp_orig_entry, *orig_entry = NULL;
 	const struct hlist_head *head;
 	struct hlist_node *node;
-	bool found = false;
 
 	rcu_read_lock();
 	head = &entry->orig_list;
@@ -646,13 +646,30 @@ batadv_tt_global_entry_has_orig(const struct batadv_tt_global_entry *entry,
 		if (!atomic_inc_not_zero(&tmp_orig_entry->refcount))
 			continue;
 
-		found = true;
-		batadv_tt_orig_list_entry_free_ref(tmp_orig_entry);
+		orig_entry = tmp_orig_entry;
 		break;
 	}
 	rcu_read_unlock();
 
-	return found;
+	return orig_entry;
+}
+
+/* find out if an orig_node is already in the list of a tt_global_entry.
+ * returns true if found, false otherwise
+ */
+static bool
+batadv_tt_global_entry_has_orig(const struct batadv_tt_global_entry *entry,
+				const struct batadv_orig_node *orig_node)
+{
+	bool res = false;
+	struct batadv_tt_orig_list_entry *orig_entry;
+
+	orig_entry = batadv_tt_global_entry_get_orig(entry, orig_node);
+	if (orig_entry) {
+		res = true;
+		batadv_tt_orig_list_entry_free_ref(orig_entry);
+	}
+	return res;
 }
 
 static void
-- 
1.7.9.4

[B.A.T.M.A.N.] [PATCHv2 3/4] batman-adv: detect not yet announced clients

[Antonio Quartulli]

With the current TT mechanism a new client joining the network is not
immediately able to communicate with other hosts because its MAC address has not
been announced yet. This situation holds until the first OGM containing its
joining event will be spread over the mesh network.

This behaviour can be acceptable in networks where the originator interval is a
small value (e.g. 1sec) but if that value is set to an higher time (e.g. 5secs)
the client could suffer from several malfunctions like DHCP client timeouts,
etc.

This patch adds an early detection mechanism that makes nodes in the network
able to recognise "not yet announced clients" by means of the broadcast packets
they emitted on connection (e.g. ARP or DHCP request). The added client will
then be confirmed upon receiving the OGM claiming it or purged if such OGM
is not received within a fixed amount of time.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 main.h              |    4 ++
 packet.h            |    1 +
 translation-table.c |  123 ++++++++++++++++++++++++++++++++++++++++-----------
 translation-table.h |    4 +-
 types.h             |    1 +
 5 files changed, 105 insertions(+), 28 deletions(-)

diff --git a/main.h b/main.h
index 6dca9c4..a4297ea 100644
--- a/main.h
+++ b/main.h
@@ -43,6 +43,10 @@
 #define BATADV_PURGE_TIMEOUT 200000 /* 200 seconds */
 #define BATADV_TT_LOCAL_TIMEOUT 3600000 /* in miliseconds */
 #define BATADV_TT_CLIENT_ROAM_TIMEOUT 600000 /* in miliseconds */
+/* Used to compute the timeout value for the entries marked with TT_CLIENT_TEMP:
+ * temp_timeout_value = temp_timeout_fact * orig_int
+ */
+#define BATADV_TT_CLIENT_TEMP_TIMEOUT_FACT 10UL
 /* sliding packet range of received originator messages in sequence numbers
  * (should be a multiple of our word size)
  */
diff --git a/packet.h b/packet.h
index 65d66e4..db21230 100644
--- a/packet.h
+++ b/packet.h
@@ -85,6 +85,7 @@ enum batadv_tt_client_flags {
 	BATADV_TT_CLIENT_DEL     = 1 << 0,
 	BATADV_TT_CLIENT_ROAM    = 1 << 1,
 	BATADV_TT_CLIENT_WIFI    = 1 << 2,
+	BATADV_TT_CLIENT_TEMP	 = 1 << 3,
 	BATADV_TT_CLIENT_NOPURGE = 1 << 8,
 	BATADV_TT_CLIENT_NEW     = 1 << 9,
 	BATADV_TT_CLIENT_PENDING = 1 << 10,
diff --git a/translation-table.c b/translation-table.c
index bcbd9aa..48b7cf2 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -34,6 +34,19 @@ static void batadv_send_roam_adv(struct batadv_priv *bat_priv, uint8_t *client,
 static void batadv_tt_purge(struct work_struct *work);
 static void
 batadv_tt_global_del_orig_list(struct batadv_tt_global_entry *tt_global_entry);
+static void batadv_tt_global_del(struct batadv_priv *bat_priv,
+				 struct batadv_orig_node *orig_node,
+				 const unsigned char *addr,
+				 const char *message, bool roaming);
+
+/* the temporary client timeout is defined as a multiple of the originator
+ * interval
+ */
+static unsigned long batadv_tt_client_temp_timeout(struct batadv_priv *bat_priv)
+{
+	return BATADV_TT_CLIENT_TEMP_TIMEOUT_FACT *
+	       atomic_read(&bat_priv->orig_interval);
+}
 
 /* returns 1 if they are the same mac addr */
 static int batadv_compare_tt(const struct hlist_node *node, const void *data2)
@@ -267,6 +280,7 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
 		tt_local_entry->common.flags |= BATADV_TT_CLIENT_WIFI;
 	atomic_set(&tt_local_entry->common.refcount, 2);
 	tt_local_entry->last_seen = jiffies;
+	tt_local_entry->common.added_at = tt_local_entry->last_seen;
 
 	/* the batman interface mac address should never be purged */
 	if (batadv_compare_eth(addr, soft_iface->dev_addr))
@@ -719,6 +733,8 @@ int batadv_tt_global_add(struct batadv_priv *bat_priv,
 		common->flags = flags;
 		tt_global_entry->roam_at = 0;
 		atomic_set(&common->refcount, 2);
+		common->added_at = jiffies;
+
 
 		INIT_HLIST_HEAD(&tt_global_entry->orig_list);
 		spin_lock_init(&tt_global_entry->list_lock);
@@ -738,6 +754,16 @@ int batadv_tt_global_add(struct batadv_priv *bat_priv,
 						ttvn);
 	} else {
 		/* there is already a global entry, use this one. */
+		/* if we are trying to add a temporary node, but we found an
+		 * already existent entry, we can exit directly
+		 */
+		if (flags & BATADV_TT_CLIENT_TEMP)
+			goto out;
+
+		/* if the client was temporary added before receiving the first
+		 * OGM announcing it, we have to clear the TEMP flag
+		 */
+		tt_global_entry->common.flags &= ~BATADV_TT_CLIENT_TEMP;
 
 		/* If there is the BATADV_TT_CLIENT_ROAM flag set, there is only
 		 * one originator left in the list and we previously received a
@@ -795,11 +821,12 @@ batadv_tt_global_print_entry(struct batadv_tt_global_entry *tt_global_entry,
 	hlist_for_each_entry_rcu(orig_entry, node, head, list) {
 		flags = tt_common_entry->flags;
 		last_ttvn = atomic_read(&orig_entry->orig_node->last_ttvn);
-		seq_printf(seq, " * %pM  (%3u) via %pM     (%3u)   [%c%c]\n",
+		seq_printf(seq,	" * %pM  (%3u) via %pM     (%3u)   [%c%c%c]\n",
 			   tt_global_entry->common.addr, orig_entry->ttvn,
 			   orig_entry->orig_node->orig, last_ttvn,
 			   (flags & BATADV_TT_CLIENT_ROAM ? 'R' : '.'),
-			   (flags & BATADV_TT_CLIENT_WIFI ? 'W' : '.'));
+			   (flags & BATADV_TT_CLIENT_WIFI ? 'W' : '.'),
+			   (flags & BATADV_TT_CLIENT_TEMP ? 'T' : '.'));
 	}
 }
 
@@ -1054,46 +1081,63 @@ void batadv_tt_global_del_orig(struct batadv_priv *bat_priv,
 	orig_node->tt_initialised = false;
 }
 
-static void batadv_tt_global_roam_purge_list(struct batadv_priv *bat_priv,
-					     struct hlist_head *head)
+static bool batadv_tt_global_to_purge(struct batadv_priv *bat_priv,
+				      struct batadv_tt_common_entry *tt_common,
+				      char **msg)
 {
-	struct batadv_tt_common_entry *tt_common_entry;
-	struct batadv_tt_global_entry *tt_global_entry;
-	struct hlist_node *node, *node_tmp;
+	bool purge = false;
+	unsigned long roam_timeout = BATADV_TT_CLIENT_ROAM_TIMEOUT;
+	unsigned long temp_timeout = batadv_tt_client_temp_timeout(bat_priv);
 
-	hlist_for_each_entry_safe(tt_common_entry, node, node_tmp, head,
-				  hash_entry) {
-		tt_global_entry = container_of(tt_common_entry,
-					       struct batadv_tt_global_entry,
-					       common);
-		if (!(tt_global_entry->common.flags & BATADV_TT_CLIENT_ROAM))
-			continue;
-		if (!batadv_has_timed_out(tt_global_entry->roam_at,
-					  BATADV_TT_CLIENT_ROAM_TIMEOUT))
-			continue;
-
-		batadv_dbg(BATADV_DBG_TT, bat_priv,
-			   "Deleting global tt entry (%pM): Roaming timeout\n",
-			   tt_global_entry->common.addr);
+	if ((tt_common->flags & BATADV_TT_CLIENT_ROAM) &&
+	    batadv_has_timed_out(tt_common->added_at, roam_timeout)) {
+		purge = true;
+		*msg = "Roaming timeout\n";
+	}
 
-		hlist_del_rcu(node);
-		batadv_tt_global_entry_free_ref(tt_global_entry);
+	if ((tt_common->flags & BATADV_TT_CLIENT_TEMP) &&
+	    batadv_has_timed_out(tt_common->added_at, temp_timeout)) {
+		purge = true;
+		*msg = "Temporary client timeout\n";
 	}
+
+	return purge;
 }
 
-static void batadv_tt_global_roam_purge(struct batadv_priv *bat_priv)
+static void batadv_tt_global_purge(struct batadv_priv *bat_priv)
 {
 	struct batadv_hashtable *hash = bat_priv->tt_global_hash;
 	struct hlist_head *head;
+	struct hlist_node *node, *node_tmp;
 	spinlock_t *list_lock; /* protects write access to the hash lists */
 	uint32_t i;
+	char *msg = NULL;
+	struct batadv_tt_common_entry *tt_common;
+	struct batadv_tt_global_entry *tt_global;
 
 	for (i = 0; i < hash->size; i++) {
 		head = &hash->table[i];
 		list_lock = &hash->list_locks[i];
 
 		spin_lock_bh(list_lock);
-		batadv_tt_global_roam_purge_list(bat_priv, head);
+		hlist_for_each_entry_safe(tt_common, node, node_tmp, head,
+					  hash_entry) {
+			if (!batadv_tt_global_to_purge(bat_priv, tt_common,
+						       &msg))
+				continue;
+
+			tt_global = container_of(tt_common,
+						 struct batadv_tt_global_entry,
+						 common);
+
+			batadv_dbg(BATADV_DBG_TT, bat_priv,
+				   "Deleting global tt entry (%pM): %s\n",
+				   tt_global->common.addr, msg);
+
+			hlist_del_rcu(node);
+
+			batadv_tt_global_entry_free_ref(tt_global);
+		}
 		spin_unlock_bh(list_lock);
 	}
 
@@ -1234,6 +1278,11 @@ static uint16_t batadv_tt_global_crc(struct batadv_priv *bat_priv,
 			 */
 			if (tt_common->flags & BATADV_TT_CLIENT_ROAM)
 				continue;
+			/* Temporary clients have not been announced yet, so
+			 * they have to be skipped while computing the global
+			 * crc */
+			if (tt_common->flags & BATADV_TT_CLIENT_TEMP)
+				continue;
 
 			/* find out if this global entry is announced by this
 			 * originator
@@ -1387,7 +1436,8 @@ static int batadv_tt_global_valid(const void *entry_ptr,
 	const struct batadv_tt_global_entry *tt_global_entry;
 	const struct batadv_orig_node *orig_node = data_ptr;
 
-	if (tt_common_entry->flags & BATADV_TT_CLIENT_ROAM)
+	if (tt_common_entry->flags & BATADV_TT_CLIENT_ROAM ||
+	    tt_common_entry->flags & BATADV_TT_CLIENT_TEMP)
 		return 0;
 
 	tt_global_entry = container_of(tt_common_entry,
@@ -2115,7 +2165,7 @@ static void batadv_tt_purge(struct work_struct *work)
 	bat_priv = container_of(delayed_work, struct batadv_priv, tt_work);
 
 	batadv_tt_local_purge(bat_priv);
-	batadv_tt_global_roam_purge(bat_priv);
+	batadv_tt_global_purge(bat_priv);
 	batadv_tt_req_purge(bat_priv);
 	batadv_tt_roam_purge(bat_priv);
 
@@ -2389,3 +2439,22 @@ bool batadv_tt_global_client_is_roaming(struct batadv_priv *bat_priv,
 out:
 	return ret;
 }
+
+bool batadv_tt_add_temporary_global_entry(struct batadv_priv *bat_priv,
+					  struct batadv_orig_node *orig_node,
+					  const unsigned char *addr)
+{
+	bool ret = false;
+
+	if (!batadv_tt_global_add(bat_priv, orig_node, addr,
+				  BATADV_TT_CLIENT_TEMP,
+				  atomic_read(&orig_node->last_ttvn)))
+		goto out;
+
+	batadv_dbg(BATADV_DBG_TT, bat_priv,
+		   "Added temporary global client (addr: %pM orig: %pM)\n",
+		   addr, orig_node->orig);
+	ret = true;
+out:
+	return ret;
+}
diff --git a/translation-table.h b/translation-table.h
index ffa8735..811fffd 100644
--- a/translation-table.h
+++ b/translation-table.h
@@ -59,6 +59,8 @@ int batadv_tt_append_diff(struct batadv_priv *bat_priv,
 			  int packet_min_len);
 bool batadv_tt_global_client_is_roaming(struct batadv_priv *bat_priv,
 					uint8_t *addr);
-
+bool batadv_tt_add_temporary_global_entry(struct batadv_priv *bat_priv,
+					  struct batadv_orig_node *orig_node,
+					  const unsigned char *addr);
 
 #endif /* _NET_BATMAN_ADV_TRANSLATION_TABLE_H_ */
diff --git a/types.h b/types.h
index 82b97c3..591bd28 100644
--- a/types.h
+++ b/types.h
@@ -262,6 +262,7 @@ struct batadv_tt_common_entry {
 	uint8_t addr[ETH_ALEN];
 	struct hlist_node hash_entry;
 	uint16_t flags;
+	unsigned long added_at;
 	atomic_t refcount;
 	struct rcu_head rcu;
 };
-- 
1.7.9.4

[B.A.T.M.A.N.] [PATCHv2 4/4] batman-adv: change interface_rx to get orig node

[Antonio Quartulli]

In order to understand  where a broadcast packet is coming from and use
this information to detect not yet announced clients, this patch modifies the
interface_rx() function by passing a new argument: the orig node
corresponding to the node that originated the received packet (if known).
This new argument if not NULL for broadcast packets only (other packets does not
have source field).

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 routing.c        |   10 ++++++----
 soft-interface.c |    6 +++++-
 soft-interface.h |    5 +++--
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/routing.c b/routing.c
index bc2b88b..b043ef9 100644
--- a/routing.c
+++ b/routing.c
@@ -1025,8 +1025,9 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 
 	/* packet for me */
 	if (batadv_is_my_mac(unicast_packet->dest)) {
-		batadv_interface_rx(recv_if->soft_iface, skb, recv_if,
-				    hdr_size);
+		batadv_interface_rx(recv_if->soft_iface, skb, recv_if, hdr_size,
+				    NULL);
+
 		return NET_RX_SUCCESS;
 	}
 
@@ -1063,7 +1064,7 @@ int batadv_recv_ucast_frag_packet(struct sk_buff *skb,
 			return NET_RX_SUCCESS;
 
 		batadv_interface_rx(recv_if->soft_iface, new_skb, recv_if,
-				    sizeof(struct batadv_unicast_packet));
+				    sizeof(struct batadv_unicast_packet), NULL);
 		return NET_RX_SUCCESS;
 	}
 
@@ -1150,7 +1151,8 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
 		goto out;
 
 	/* broadcast for me */
-	batadv_interface_rx(recv_if->soft_iface, skb, recv_if, hdr_size);
+	batadv_interface_rx(recv_if->soft_iface, skb, recv_if, hdr_size,
+			    orig_node);
 	ret = NET_RX_SUCCESS;
 	goto out;
 
diff --git a/soft-interface.c b/soft-interface.c
index c77473e..8606a5e 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -268,7 +268,7 @@ end:
 
 void batadv_interface_rx(struct net_device *soft_iface,
 			 struct sk_buff *skb, struct batadv_hard_iface *recv_if,
-			 int hdr_size)
+			 int hdr_size, struct batadv_orig_node *orig_node)
 {
 	struct batadv_priv *bat_priv = netdev_priv(soft_iface);
 	struct ethhdr *ethhdr;
@@ -316,6 +316,10 @@ void batadv_interface_rx(struct net_device *soft_iface,
 
 	soft_iface->last_rx = jiffies;
 
+	if (orig_node)
+		batadv_tt_add_temporary_global_entry(bat_priv, orig_node,
+						     ethhdr->h_source);
+
 	if (batadv_is_ap_isolated(bat_priv, ethhdr->h_source, ethhdr->h_dest))
 		goto dropped;
 
diff --git a/soft-interface.h b/soft-interface.h
index 852c683..07a08fe 100644
--- a/soft-interface.h
+++ b/soft-interface.h
@@ -21,8 +21,9 @@
 #define _NET_BATMAN_ADV_SOFT_INTERFACE_H_
 
 int batadv_skb_head_push(struct sk_buff *skb, unsigned int len);
-void batadv_interface_rx(struct net_device *soft_iface, struct sk_buff *skb,
-			 struct batadv_hard_iface *recv_if, int hdr_size);
+void batadv_interface_rx(struct net_device *soft_iface,
+			 struct sk_buff *skb, struct batadv_hard_iface *recv_if,
+			 int hdr_size, struct batadv_orig_node *orig_node);
 struct net_device *batadv_softif_create(const char *name);
 void batadv_softif_destroy(struct net_device *soft_iface);
 int batadv_softif_is_valid(const struct net_device *net_dev);
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCHv2 1/4] batman-adv: add reference counting for type batadv_tt_orig_list_entry

[Marek Lindner]

On Wednesday, June 27, 2012 10:23:04 Antonio Quartulli wrote:
> @@ -639,12 +641,17 @@ batadv_tt_global_entry_has_orig(const struct
> batadv_tt_global_entry *entry, rcu_read_lock();
>  	head = &entry->orig_list;
>  	hlist_for_each_entry_rcu(tmp_orig_entry, node, head, list) {
> -		if (tmp_orig_entry->orig_node == orig_node) {
> -			found = true;
> -			break;
> -		}
> +		if (tmp_orig_entry->orig_node != orig_node)
> +			continue;
> +		if (!atomic_inc_not_zero(&tmp_orig_entry->refcount))
> +			continue;
> +
> +		found = true;
> +		batadv_tt_orig_list_entry_free_ref(tmp_orig_entry);

Instead of having this weird increment and immediate decrement this patch 
should add a general "orig_entry_get" / "orig_entry_find" function like we do 
everywhere else. Check the following functions to get an idea:
 * batadv_primary_if_get_selected()
 * batadv_orig_node_get_router()
 * batadv_tt_hash_find()


> @@ -663,6 +670,7 @@ batadv_tt_global_add_orig_entry(struct
> batadv_tt_global_entry *tt_global_entry, atomic_inc(&orig_node->tt_size);
>  	orig_entry->orig_node = orig_node;
>  	orig_entry->ttvn = ttvn;
> +	atomic_set(&orig_entry->refcount, 0);

This looks extremely broken. We init with 0, never increase the counter and 
when we should free, we decrease first before checking if it is 0 ?

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCHv2 2/4] batman-adv: implement batadv_tt_global_entry_get_orig()

[Marek Lindner]

On Wednesday, June 27, 2012 10:23:05 Antonio Quartulli wrote:
> @@ -626,17 +626,17 @@ static void batadv_tt_changes_list_free(struct
> batadv_priv *bat_priv) spin_unlock_bh(&bat_priv->tt_changes_list_lock);
>  }
>  
> -/* find out if an orig_node is already in the list of a tt_global_entry.
> - * returns 1 if found, 0 otherwise
> +/* find out if an orig_node is already in the list of a tt_global_entry and 
> + * returns it with an increased refcounter if found, NULL otherwise.
> + *
>   */

This empty line does not look David-compliant.

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCHv2 3/4] batman-adv: detect not yet announced clients

[Marek Lindner]

On Wednesday, June 27, 2012 10:23:06 Antonio Quartulli wrote:
> @@ -43,6 +43,10 @@
>  #define BATADV_PURGE_TIMEOUT 200000 /* 200 seconds */
>  #define BATADV_TT_LOCAL_TIMEOUT 3600000 /* in miliseconds */
>  #define BATADV_TT_CLIENT_ROAM_TIMEOUT 600000 /* in miliseconds */
> +/* Used to compute the timeout value for the entries marked with
> TT_CLIENT_TEMP: + * temp_timeout_value = temp_timeout_fact * orig_int
> + */
> +#define BATADV_TT_CLIENT_TEMP_TIMEOUT_FACT 10UL

You implictely assume that all nodes have the originator interval which might 
not be the case. As soon as they are different this will horribly fail.

Why not using the originator sequence number instead ? It will give you a time 
independent timeout. You could say if we haven't received a TT ADD 3 sequence 
numbers after we added the temporary client is gets merged.

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCHv2 1/4] batman-adv: add reference counting for type batadv_tt_orig_list_entry

[Antonio Quartulli]

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

On Sat, Jun 30, 2012 at 11:55:25AM +0200, Marek Lindner wrote:
> On Wednesday, June 27, 2012 10:23:04 Antonio Quartulli wrote:
> > @@ -639,12 +641,17 @@ batadv_tt_global_entry_has_orig(const struct
> > batadv_tt_global_entry *entry, rcu_read_lock();
> >  	head = &entry->orig_list;
> >  	hlist_for_each_entry_rcu(tmp_orig_entry, node, head, list) {
> > -		if (tmp_orig_entry->orig_node == orig_node) {
> > -			found = true;
> > -			break;
> > -		}
> > +		if (tmp_orig_entry->orig_node != orig_node)
> > +			continue;
> > +		if (!atomic_inc_not_zero(&tmp_orig_entry->refcount))
> > +			continue;
> > +
> > +		found = true;
> > +		batadv_tt_orig_list_entry_free_ref(tmp_orig_entry);
> 
> Instead of having this weird increment and immediate decrement this patch 
> should add a general "orig_entry_get" / "orig_entry_find" function like we do 
> everywhere else. Check the following functions to get an idea:
>  * batadv_primary_if_get_selected()
>  * batadv_orig_node_get_router()
>  * batadv_tt_hash_find()
> 

Well, actually that is what I implemented in patch 2/4 (ok, I'll fix the
function names to reflect what we already have). In this patch I simply tried to
implement refcounting without changing too much code.

By the way, I will introduce a get function with this patch directly.

> 
> > @@ -663,6 +670,7 @@ batadv_tt_global_add_orig_entry(struct
> > batadv_tt_global_entry *tt_global_entry, atomic_inc(&orig_node->tt_size);
> >  	orig_entry->orig_node = orig_node;
> >  	orig_entry->ttvn = ttvn;
> > +	atomic_set(&orig_entry->refcount, 0);
> 
> This looks extremely broken. We init with 0, never increase the counter and 
> when we should free, we decrease first before checking if it is 0 ?
> 

definitely broken. Will fix it!


Thank you,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [B.A.T.M.A.N.] [PATCHv2 3/4] batman-adv: detect not yet announced clients

[Antonio Quartulli]

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

On Sat, Jun 30, 2012 at 12:01:25PM +0200, Marek Lindner wrote:
> On Wednesday, June 27, 2012 10:23:06 Antonio Quartulli wrote:
> > @@ -43,6 +43,10 @@
> >  #define BATADV_PURGE_TIMEOUT 200000 /* 200 seconds */
> >  #define BATADV_TT_LOCAL_TIMEOUT 3600000 /* in miliseconds */
> >  #define BATADV_TT_CLIENT_ROAM_TIMEOUT 600000 /* in miliseconds */
> > +/* Used to compute the timeout value for the entries marked with
> > TT_CLIENT_TEMP: + * temp_timeout_value = temp_timeout_fact * orig_int
> > + */
> > +#define BATADV_TT_CLIENT_TEMP_TIMEOUT_FACT 10UL
> 
> You implictely assume that all nodes have the originator interval which might 
> not be the case. As soon as they are different this will horribly fail.
> 
> Why not using the originator sequence number instead ? It will give you a time 
> independent timeout. You could say if we haven't received a TT ADD 3 sequence 
> numbers after we added the temporary client is gets merged.

That is a very good idea. The problem is that in a much more general scenario we
have to keep in mind that the client could roam before being announced. At that
point, we will be waiting for an announcement coming from a wrong originator.

Actually we have the same problem with the ROAM flag. Right now we simply wait X
seconds: if nobody claims this guy within such amount of time we purge it. We
should probably do the same here...(we can reuse the same value we use for the
roam flag).

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]