[B.A.T.M.A.N.] [PATCH] batman-adv: pass a unique flag arg instead of a sequence of bool ones

[B.A.T.M.A.N.] [PATCH] batman-adv: pass a unique flag arg instead of a sequence of bool ones

[Antonio Quartulli]

now tt_local_event() takes a flags argument instead of a sequence of
boolean values which would grow up with the time.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 translation-table.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/translation-table.c b/translation-table.c
index 4208dc7..796303d 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -143,8 +143,8 @@ static void tt_global_entry_free_ref(struct tt_global_entry *tt_global_entry)
 		kfree_rcu(tt_global_entry, rcu);
 }
 
-static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
-			   const uint8_t *addr, bool roaming)
+static void tt_local_event(struct bat_priv *bat_priv, const uint8_t *addr,
+			   int flags)
 {
 	struct tt_change_node *tt_change_node;
 
@@ -153,10 +153,10 @@ static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
 	if (!tt_change_node)
 		return;
 
-	tt_change_node->change.flags = op;
-	if (roaming)
-		tt_change_node->change.flags |= TT_CLIENT_ROAM;
+	/* NOPURGE flag has not to go on the wire */
+	flags &= ~TT_CLIENT_NOPURGE;
 
+	tt_change_node->change.flags = flags;
 	memcpy(tt_change_node->change.addr, addr, ETH_ALEN);
 
 	spin_lock_bh(&bat_priv->tt_changes_list_lock);
@@ -203,8 +203,6 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
 	if (!tt_local_entry)
 		goto out;
 
-	tt_local_event(bat_priv, NO_FLAGS, addr, false);
-
 	bat_dbg(DBG_TT, bat_priv,
 		"Creating new local tt entry: %pM (ttvn: %d)\n", addr,
 		(uint8_t)atomic_read(&bat_priv->ttvn));
@@ -218,6 +216,8 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
 	if (compare_eth(addr, soft_iface->dev_addr))
 		tt_local_entry->flags |= TT_CLIENT_NOPURGE;
 
+	tt_local_event(bat_priv, addr, tt_local_entry->flags);
+
 	hash_add(bat_priv->tt_local_hash, compare_ltt, choose_orig,
 		 tt_local_entry, &tt_local_entry->hash_entry);
 
@@ -386,7 +386,8 @@ void tt_local_remove(struct bat_priv *bat_priv, const uint8_t *addr,
 	if (!tt_local_entry)
 		goto out;
 
-	tt_local_event(bat_priv, TT_CLIENT_DEL, tt_local_entry->addr, roaming);
+	tt_local_event(bat_priv, tt_local_entry->addr,
+		       tt_local_entry->flags | TT_CLIENT_DEL);
 	tt_local_del(bat_priv, tt_local_entry, message);
 out:
 	if (tt_local_entry)
@@ -416,8 +417,8 @@ static void tt_local_purge(struct bat_priv *bat_priv)
 					    TT_LOCAL_TIMEOUT * 1000))
 				continue;
 
-			tt_local_event(bat_priv, TT_CLIENT_DEL,
-				       tt_local_entry->addr, false);
+			tt_local_event(bat_priv, tt_local_entry->addr,
+				       tt_local_entry->flags | TT_CLIENT_DEL);
 			atomic_dec(&bat_priv->num_local_tt);
 			bat_dbg(DBG_TT, bat_priv, "Deleting local "
 				"tt entry (%pM): timed out\n",
-- 
1.7.3.4

[B.A.T.M.A.N.] [PATCHv2] batman-adv: pass a unique flag arg instead of a sequence of bool ones

[Antonio Quartulli]

now tt_local_event() takes a flags argument instead of a sequence of
boolean values which would grow up with the time.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---

tt_local_remove() wasn't using the "bool roaming" parameter into account.

 translation-table.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/translation-table.c b/translation-table.c
index 4208dc7..4537f02 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -143,8 +143,8 @@ static void tt_global_entry_free_ref(struct tt_global_entry *tt_global_entry)
 		kfree_rcu(tt_global_entry, rcu);
 }
 
-static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
-			   const uint8_t *addr, bool roaming)
+static void tt_local_event(struct bat_priv *bat_priv, const uint8_t *addr,
+			   int flags)
 {
 	struct tt_change_node *tt_change_node;
 
@@ -153,10 +153,10 @@ static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
 	if (!tt_change_node)
 		return;
 
-	tt_change_node->change.flags = op;
-	if (roaming)
-		tt_change_node->change.flags |= TT_CLIENT_ROAM;
+	/* NOPURGE flag has not to go on the wire */
+	flags &= ~TT_CLIENT_NOPURGE;
 
+	tt_change_node->change.flags = flags;
 	memcpy(tt_change_node->change.addr, addr, ETH_ALEN);
 
 	spin_lock_bh(&bat_priv->tt_changes_list_lock);
@@ -203,8 +203,6 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
 	if (!tt_local_entry)
 		goto out;
 
-	tt_local_event(bat_priv, NO_FLAGS, addr, false);
-
 	bat_dbg(DBG_TT, bat_priv,
 		"Creating new local tt entry: %pM (ttvn: %d)\n", addr,
 		(uint8_t)atomic_read(&bat_priv->ttvn));
@@ -218,6 +216,8 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
 	if (compare_eth(addr, soft_iface->dev_addr))
 		tt_local_entry->flags |= TT_CLIENT_NOPURGE;
 
+	tt_local_event(bat_priv, addr, tt_local_entry->flags);
+
 	hash_add(bat_priv->tt_local_hash, compare_ltt, choose_orig,
 		 tt_local_entry, &tt_local_entry->hash_entry);
 
@@ -386,7 +386,9 @@ void tt_local_remove(struct bat_priv *bat_priv, const uint8_t *addr,
 	if (!tt_local_entry)
 		goto out;
 
-	tt_local_event(bat_priv, TT_CLIENT_DEL, tt_local_entry->addr, roaming);
+	tt_local_event(bat_priv, tt_local_entry->addr,
+		       tt_local_entry->flags | TT_CLIENT_DEL |
+		       (roaming ? TT_CLIENT_ROAM : NO_FLAGS));
 	tt_local_del(bat_priv, tt_local_entry, message);
 out:
 	if (tt_local_entry)
@@ -416,8 +418,8 @@ static void tt_local_purge(struct bat_priv *bat_priv)
 					    TT_LOCAL_TIMEOUT * 1000))
 				continue;
 
-			tt_local_event(bat_priv, TT_CLIENT_DEL,
-				       tt_local_entry->addr, false);
+			tt_local_event(bat_priv, tt_local_entry->addr,
+				       tt_local_entry->flags | TT_CLIENT_DEL);
 			atomic_dec(&bat_priv->num_local_tt);
 			bat_dbg(DBG_TT, bat_priv, "Deleting local "
 				"tt entry (%pM): timed out\n",
-- 
1.7.3.4

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: pass a unique flag arg instead of a sequence of bool ones

[Andrew Lunn]

Hi Antonio

> +	/* NOPURGE flag has not to go on the wire */
> +	flags &= ~TT_CLIENT_NOPURGE;
>  
> +	tt_change_node->change.flags = flags;

I don't remember the packet format. But i assume flags on the wire is
a u8? How about making TT_CLIENT_NOPURGE 1<<8 or bigger, so it is
automatically not sent on the wire? It will also make it easier
sometime in the future when you want to use the bit on the wire that
TT_CLIENT_NOPURGE is currently taking.

You might also want to change the function parameters flags from int
to uint, so avoiding sign extension problems.

   Andrew

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: pass a unique flag arg instead of a sequence of bool ones

[Antonio Quartulli]

Hi Andrew,

On Mon, Jun 27, 2011 at 07:33:19AM +0200, Andrew Lunn wrote:
> Hi Antonio
> 
> > +	/* NOPURGE flag has not to go on the wire */
> > +	flags &= ~TT_CLIENT_NOPURGE;
> >  
> > +	tt_change_node->change.flags = flags;
> 
> I don't remember the packet format. But i assume flags on the wire is
> a u8? How about making TT_CLIENT_NOPURGE 1<<8 or bigger, so it is
> automatically not sent on the wire? It will also make it easier
> sometime in the future when you want to use the bit on the wire that
> TT_CLIENT_NOPURGE is currently taking.
> 

Even if we want to add other flags that don't need to be sent on the
wire. I like this idea.

We can have a uint16_t flags field in the
tt_local/global_entry structure, where the 8 tailing bits only are sent
within the messages. Instead, the 8 leading bits are reserved for local purposes.

> You might also want to change the function parameters flags from int
> to uint, so avoiding sign extension problems.

Yes, thanks!

> 
>    Andrew


-- 
Antonio Quartulli

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

[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: split tt_local/global_entry flags field for local and remote computations

[Antonio Quartulli]

The tt_global/local_entry 'flags' field contains flags used either for
local and remote computations. To optimise the usage of this field, its
length has been increased to 16bit in order to use the eight leading bits
(from 0 to 7) to store flags that have to be sent on the wire, while the
eight ending ones are used for local computation only.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 packet.h |   10 ++++++----
 types.h  |    4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/packet.h b/packet.h
index 8fd2fde..590e4a6 100644
--- a/packet.h
+++ b/packet.h
@@ -78,11 +78,13 @@ enum tt_query_flags {
 	TT_FULL_TABLE = 1 << 2
 };
 
-/* TT_CLIENT flags */
+/* TT_CLIENT flags.
+ * Flags from 1 to 1 << 7 are sent on the wire, while flags from 1 << 8 to
+ * 1 << 15 are used for local computation only */
 enum tt_client_flags {
-	TT_CLIENT_DEL     = 0x01,
-	TT_CLIENT_ROAM    = 0x02,
-	TT_CLIENT_NOPURGE = 0x04
+	TT_CLIENT_DEL     = 1 << 0,
+	TT_CLIENT_ROAM    = 1 << 1,
+	TT_CLIENT_NOPURGE = 1 << 8
 };
 
 struct batman_packet {
diff --git a/types.h b/types.h
index 582283a..25bd1db 100644
--- a/types.h
+++ b/types.h
@@ -224,7 +224,7 @@ struct socket_packet {
 struct tt_local_entry {
 	uint8_t addr[ETH_ALEN];
 	unsigned long last_seen;
-	uint8_t flags;
+	uint16_t flags;
 	atomic_t refcount;
 	struct rcu_head rcu;
 	struct hlist_node hash_entry;
@@ -234,7 +234,7 @@ struct tt_global_entry {
 	uint8_t addr[ETH_ALEN];
 	struct orig_node *orig_node;
 	uint8_t ttvn;
-	uint8_t flags; /* only TT_GLOBAL_ROAM is used */
+	uint16_t flags; /* only TT_GLOBAL_ROAM is used */
 	unsigned long roam_at; /* time at which TT_GLOBAL_ROAM was set */
 	atomic_t refcount;
 	struct rcu_head rcu;
-- 
1.7.3.4

[B.A.T.M.A.N.] [PATCHv3 2/2] batman-adv: pass a unique flag arg instead of a sequence of bool ones

[Antonio Quartulli]

now tt_local_event() takes a flags argument instead of a sequence of
boolean values which would grow up with the time.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---

Now TT_CLIENT_PURGE is not more filtered out as it automatically trashed when the local flags
field value (16bit) is assigned to the on-wire one (8 bits)

 translation-table.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/translation-table.c b/translation-table.c
index 4208dc7..06d361d 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -143,8 +143,8 @@ static void tt_global_entry_free_ref(struct tt_global_entry *tt_global_entry)
 		kfree_rcu(tt_global_entry, rcu);
 }
 
-static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
-			   const uint8_t *addr, bool roaming)
+static void tt_local_event(struct bat_priv *bat_priv, const uint8_t *addr,
+			   uint8_t flags)
 {
 	struct tt_change_node *tt_change_node;
 
@@ -153,10 +153,7 @@ static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
 	if (!tt_change_node)
 		return;
 
-	tt_change_node->change.flags = op;
-	if (roaming)
-		tt_change_node->change.flags |= TT_CLIENT_ROAM;
-
+	tt_change_node->change.flags = flags;
 	memcpy(tt_change_node->change.addr, addr, ETH_ALEN);
 
 	spin_lock_bh(&bat_priv->tt_changes_list_lock);
@@ -203,8 +200,6 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
 	if (!tt_local_entry)
 		goto out;
 
-	tt_local_event(bat_priv, NO_FLAGS, addr, false);
-
 	bat_dbg(DBG_TT, bat_priv,
 		"Creating new local tt entry: %pM (ttvn: %d)\n", addr,
 		(uint8_t)atomic_read(&bat_priv->ttvn));
@@ -218,6 +213,8 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
 	if (compare_eth(addr, soft_iface->dev_addr))
 		tt_local_entry->flags |= TT_CLIENT_NOPURGE;
 
+	tt_local_event(bat_priv, addr, tt_local_entry->flags);
+
 	hash_add(bat_priv->tt_local_hash, compare_ltt, choose_orig,
 		 tt_local_entry, &tt_local_entry->hash_entry);
 
@@ -386,7 +383,9 @@ void tt_local_remove(struct bat_priv *bat_priv, const uint8_t *addr,
 	if (!tt_local_entry)
 		goto out;
 
-	tt_local_event(bat_priv, TT_CLIENT_DEL, tt_local_entry->addr, roaming);
+	tt_local_event(bat_priv, tt_local_entry->addr,
+		       tt_local_entry->flags | TT_CLIENT_DEL |
+		       (roaming ? TT_CLIENT_ROAM : NO_FLAGS));
 	tt_local_del(bat_priv, tt_local_entry, message);
 out:
 	if (tt_local_entry)
@@ -416,8 +415,8 @@ static void tt_local_purge(struct bat_priv *bat_priv)
 					    TT_LOCAL_TIMEOUT * 1000))
 				continue;
 
-			tt_local_event(bat_priv, TT_CLIENT_DEL,
-				       tt_local_entry->addr, false);
+			tt_local_event(bat_priv, tt_local_entry->addr,
+				       tt_local_entry->flags | TT_CLIENT_DEL);
 			atomic_dec(&bat_priv->num_local_tt);
 			bat_dbg(DBG_TT, bat_priv, "Deleting local "
 				"tt entry (%pM): timed out\n",
-- 
1.7.3.4

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: split tt_local/global_entry flags field for local and remote computations

[Marek Lindner]

On Thursday, June 30, 2011 01:13:59 Antonio Quartulli wrote:
> The tt_global/local_entry 'flags' field contains flags used either for
> local and remote computations. To optimise the usage of this field, its
> length has been increased to 16bit in order to use the eight leading bits
> (from 0 to 7) to store flags that have to be sent on the wire, while the
> eight ending ones are used for local computation only.

Applied in revision 68f78c7.

Thanks,
Marek

Re: [B.A.T.M.A.N.] [PATCHv3 2/2] batman-adv: pass a unique flag arg instead of a sequence of bool ones

[Marek Lindner]

On Thursday, June 30, 2011 01:14:00 Antonio Quartulli wrote:
> now tt_local_event() takes a flags argument instead of a sequence of
> boolean values which would grow up with the time.

Applied in revision fc01f17.

Thanks,
Marek