[B.A.T.M.A.N.] [PATCH 0/5] Preparation for SPEEDY_JOIN/ROAM

[B.A.T.M.A.N.] [PATCH 0/5] Preparation for SPEEDY_JOIN/ROAM

[Antonio Quartulli]

Hello everybody,

in preparation to my next patchset, which will enable clients to be detected
from nodes in the network _before_ they are announced by means of the TT
mechanism (more details will come within the companion email email of the
related patchset), I'm sending this 4 patches that fix/clean up something here
and there plus a little behaviour change in the OGM preparation.

In particular (for further details please refer to the commit message of each
patch):

patch 1) changes the behaviour of how an OGM is built. Instead of
preparing/building it at the same time we schedule it, this patch delays such
operation to the instant before sending the message. This patch also introduce a
new entry in the protocol API called ".bat_fill_ogm".

patch 2) in case of ADD and DEL events (or viceversa) of the same client during
the same originator interval, such events can be both nullified instead of being
sent.

patch 3) future operations will require to look for a particular originator in
the orig_list of a global entry, therefore tt_global_entry_has_orig() is
modified accordingly so that it returns the found originator, if any.

patch 4) updates the tt_global_entry ttvn in case of reannouncement of the same
client.

patch 5) changes tt_global_Add() sign. We use to pass several boolean flags to
this function that are then converted to the TT_CLIENT_* ones. This patch
modifies the tt_global_add() sign so that we can directly pass a combination of
TT_CLIENT_* flags instead of several boolean parameters.



Thank you,
	Antonio

[B.A.T.M.A.N.] [PATCH 1/5] batman-adv: don't delay OGM information announcement

[Antonio Quartulli]

In the current implementation the OGM is built and filled at the moment it is
scheduled (1 originator interval before its sending). In this way, all the TT
changes happening between OGM(seqno=X) and OGM(seqno=X+1) will be attached to
OGM(seqno=X+2) (because when changes happened OGM(seqno=X+1) was already built).

The result of this strategy is that TT changes are not sent within the
very next OGM, but they are sent within the second OGM since they happened.
This mechanism will introduce a delay for table resync which could lead to
wrong state in case of multiple roamings.

This patch delays all the operations so that the OGM is filled and ultimated
just before being sent.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 bat_iv_ogm.c |  108 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 send.c       |   75 ++--------------------------------------
 types.h      |    6 ++--
 3 files changed, 99 insertions(+), 90 deletions(-)

diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
index ed743a4..377ee04 100644
--- a/bat_iv_ogm.c
+++ b/bat_iv_ogm.c
@@ -558,28 +558,97 @@ static void bat_iv_ogm_forward(struct orig_node *orig_node,
 			     if_incoming, 0, bat_iv_ogm_fwd_send_time());
 }
 
-static void bat_iv_ogm_schedule(struct hard_iface *hard_iface,
-				int tt_num_changes)
+static void realloc_packet_buffer(struct hard_iface *hard_iface,
+				  int new_len)
 {
-	struct bat_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
+	unsigned char *new_buff;
+
+	new_buff = kmalloc(new_len, GFP_ATOMIC);
+
+	/* keep old buffer if kmalloc should fail */
+	if (new_buff) {
+		memcpy(new_buff, hard_iface->packet_buff,
+		       BATMAN_OGM_HLEN);
+
+		kfree(hard_iface->packet_buff);
+		hard_iface->packet_buff = new_buff;
+		hard_iface->packet_len = new_len;
+	}
+}
+
+/* when calling this function (hard_iface == primary_if) has to be true */
+static int prepare_packet_buffer(struct bat_priv *bat_priv,
+				  struct hard_iface *hard_iface)
+{
+	int new_len;
+
+	new_len = BATMAN_OGM_HLEN +
+		  tt_len((uint8_t)atomic_read(&bat_priv->tt_local_changes));
+
+	/* if we have too many changes for one packet don't send any
+	 * and wait for the tt table request which will be fragmented */
+	if (new_len > hard_iface->soft_iface->mtu)
+		new_len = BATMAN_OGM_HLEN;
+
+	realloc_packet_buffer(hard_iface, new_len);
+
+	bat_priv->tt_crc = tt_local_crc(bat_priv);
+
+	/* reset the sending counter */
+	atomic_set(&bat_priv->tt_ogm_append_cnt, TT_OGM_APPEND_MAX);
+
+	return tt_changes_fill_buffer(bat_priv,
+				      hard_iface->packet_buff + BATMAN_OGM_HLEN,
+				      hard_iface->packet_len - BATMAN_OGM_HLEN);
+}
+
+static int reset_packet_buffer(struct bat_priv *bat_priv,
+				struct hard_iface *hard_iface)
+{
+	realloc_packet_buffer(hard_iface, BATMAN_OGM_HLEN);
+	return 0;
+}
+
+static void bat_iv_fill_ogm(struct bat_priv *bat_priv,
+			    struct forw_packet *forw_packet)
+{
+	struct hard_iface *primary_if, *hard_iface;
 	struct batman_ogm_packet *batman_ogm_packet;
-	struct hard_iface *primary_if;
-	int vis_server;
+	int vis_server, tt_num_changes = -1;
 
-	vis_server = atomic_read(&bat_priv->vis_mode);
+	hard_iface = forw_packet->if_incoming;
 	primary_if = primary_if_get_selected(bat_priv);
-
 	batman_ogm_packet = (struct batman_ogm_packet *)hard_iface->packet_buff;
 
+	if (forw_packet->own && forw_packet->if_incoming == primary_if) {
+		/* if at least one change happened */
+		if (atomic_read(&bat_priv->tt_local_changes) > 0) {
+			tt_commit_changes(bat_priv);
+			tt_num_changes = prepare_packet_buffer(bat_priv,
+							       hard_iface);
+		}
+
+		/* if the changes have been sent often enough */
+		if (!atomic_dec_not_zero(&bat_priv->tt_ogm_append_cnt))
+			tt_num_changes = reset_packet_buffer(bat_priv,
+							     hard_iface);
+
+		batman_ogm_packet =
+			(struct batman_ogm_packet *)hard_iface->packet_buff;
+
+		if (tt_num_changes >= 0)
+			batman_ogm_packet->tt_num_changes = tt_num_changes;
+		batman_ogm_packet->ttvn = atomic_read(&bat_priv->ttvn);
+		batman_ogm_packet->tt_crc = htons(bat_priv->tt_crc);
+	}
+
+	vis_server = atomic_read(&bat_priv->vis_mode);
+	primary_if = primary_if_get_selected(bat_priv);
+
 	/* change sequence number to network order */
 	batman_ogm_packet->seqno =
 			htonl((uint32_t)atomic_read(&hard_iface->seqno));
 
-	batman_ogm_packet->ttvn = atomic_read(&bat_priv->ttvn);
-	batman_ogm_packet->tt_crc = htons(bat_priv->tt_crc);
-	if (tt_num_changes >= 0)
-		batman_ogm_packet->tt_num_changes = tt_num_changes;
-
 	if (vis_server == VIS_TYPE_SERVER_SYNC)
 		batman_ogm_packet->flags |= VIS_SERVER;
 	else
@@ -592,15 +661,23 @@ static void bat_iv_ogm_schedule(struct hard_iface *hard_iface,
 	else
 		batman_ogm_packet->gw_flags = NO_FLAGS;
 
+	if (primary_if)
+		hardif_free_ref(primary_if);
+}
+
+static void bat_iv_ogm_schedule(struct hard_iface *hard_iface)
+{
+	struct bat_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
+	struct batman_ogm_packet *batman_ogm_packet;
+
+	batman_ogm_packet = (struct batman_ogm_packet *)hard_iface->packet_buff;
+
 	atomic_inc(&hard_iface->seqno);
 
 	slide_own_bcast_window(hard_iface);
 	bat_iv_ogm_queue_add(bat_priv, hard_iface->packet_buff,
 			     hard_iface->packet_len, hard_iface, 1,
 			     bat_iv_ogm_emit_send_time(bat_priv));
-
-	if (primary_if)
-		hardif_free_ref(primary_if);
 }
 
 static void bat_iv_ogm_orig_update(struct bat_priv *bat_priv,
@@ -1238,6 +1315,7 @@ static struct bat_algo_ops batman_iv __read_mostly = {
 	.bat_iface_disable = bat_iv_ogm_iface_disable,
 	.bat_iface_update_mac = bat_iv_ogm_iface_update_mac,
 	.bat_primary_iface_set = bat_iv_ogm_primary_iface_set,
+	.bat_fill_ogm = bat_iv_fill_ogm,
 	.bat_ogm_schedule = bat_iv_ogm_schedule,
 	.bat_ogm_emit = bat_iv_ogm_emit,
 };
diff --git a/send.c b/send.c
index cebc14a..c79e0b3 100644
--- a/send.c
+++ b/send.c
@@ -78,62 +78,9 @@ send_skb_err:
 	return NET_XMIT_DROP;
 }
 
-static void realloc_packet_buffer(struct hard_iface *hard_iface,
-				  int new_len)
-{
-	unsigned char *new_buff;
-
-	new_buff = kmalloc(new_len, GFP_ATOMIC);
-
-	/* keep old buffer if kmalloc should fail */
-	if (new_buff) {
-		memcpy(new_buff, hard_iface->packet_buff,
-		       BATMAN_OGM_HLEN);
-
-		kfree(hard_iface->packet_buff);
-		hard_iface->packet_buff = new_buff;
-		hard_iface->packet_len = new_len;
-	}
-}
-
-/* when calling this function (hard_iface == primary_if) has to be true */
-static int prepare_packet_buffer(struct bat_priv *bat_priv,
-				  struct hard_iface *hard_iface)
-{
-	int new_len;
-
-	new_len = BATMAN_OGM_HLEN +
-		  tt_len((uint8_t)atomic_read(&bat_priv->tt_local_changes));
-
-	/* if we have too many changes for one packet don't send any
-	 * and wait for the tt table request which will be fragmented */
-	if (new_len > hard_iface->soft_iface->mtu)
-		new_len = BATMAN_OGM_HLEN;
-
-	realloc_packet_buffer(hard_iface, new_len);
-
-	bat_priv->tt_crc = tt_local_crc(bat_priv);
-
-	/* reset the sending counter */
-	atomic_set(&bat_priv->tt_ogm_append_cnt, TT_OGM_APPEND_MAX);
-
-	return tt_changes_fill_buffer(bat_priv,
-				      hard_iface->packet_buff + BATMAN_OGM_HLEN,
-				      hard_iface->packet_len - BATMAN_OGM_HLEN);
-}
-
-static int reset_packet_buffer(struct bat_priv *bat_priv,
-				struct hard_iface *hard_iface)
-{
-	realloc_packet_buffer(hard_iface, BATMAN_OGM_HLEN);
-	return 0;
-}
-
 void schedule_bat_ogm(struct hard_iface *hard_iface)
 {
 	struct bat_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
-	struct hard_iface *primary_if;
-	int tt_num_changes = -1;
 
 	if ((hard_iface->if_status == IF_NOT_IN_USE) ||
 	    (hard_iface->if_status == IF_TO_BE_REMOVED))
@@ -149,26 +96,7 @@ void schedule_bat_ogm(struct hard_iface *hard_iface)
 	if (hard_iface->if_status == IF_TO_BE_ACTIVATED)
 		hard_iface->if_status = IF_ACTIVE;
 
-	primary_if = primary_if_get_selected(bat_priv);
-
-	if (hard_iface == primary_if) {
-		/* if at least one change happened */
-		if (atomic_read(&bat_priv->tt_local_changes) > 0) {
-			tt_commit_changes(bat_priv);
-			tt_num_changes = prepare_packet_buffer(bat_priv,
-							       hard_iface);
-		}
-
-		/* if the changes have been sent often enough */
-		if (!atomic_dec_not_zero(&bat_priv->tt_ogm_append_cnt))
-			tt_num_changes = reset_packet_buffer(bat_priv,
-							     hard_iface);
-	}
-
-	if (primary_if)
-		hardif_free_ref(primary_if);
-
-	bat_priv->bat_algo_ops->bat_ogm_schedule(hard_iface, tt_num_changes);
+	bat_priv->bat_algo_ops->bat_ogm_schedule(hard_iface);
 }
 
 static void forw_packet_free(struct forw_packet *forw_packet)
@@ -321,6 +249,7 @@ void send_outstanding_bat_ogm_packet(struct work_struct *work)
 	if (atomic_read(&bat_priv->mesh_state) == MESH_DEACTIVATING)
 		goto out;
 
+	bat_priv->bat_algo_ops->bat_fill_ogm(bat_priv, forw_packet);
 	bat_priv->bat_algo_ops->bat_ogm_emit(forw_packet);
 
 	/**
diff --git a/types.h b/types.h
index 995e910..c1fb193 100644
--- a/types.h
+++ b/types.h
@@ -404,8 +404,10 @@ struct bat_algo_ops {
 	/* called when primary interface is selected / changed */
 	void (*bat_primary_iface_set)(struct hard_iface *hard_iface);
 	/* prepare a new outgoing OGM for the send queue */
-	void (*bat_ogm_schedule)(struct hard_iface *hard_iface,
-				 int tt_num_changes);
+	void (*bat_ogm_schedule)(struct hard_iface *hard_iface);
+	/* fill the outgoing OGM */
+	void (*bat_fill_ogm)(struct bat_priv *bat_priv,
+			     struct forw_packet *forw_packet);
 	/* send scheduled OGM */
 	void (*bat_ogm_emit)(struct forw_packet *forw_packet);
 };
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH 1/5] batman-adv: don't delay OGM information announcement

[Martin Hundebøll]

Hi Antonio,

One minor/trivial change below.

On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> In the current implementation the OGM is built and filled at the moment it is
> scheduled (1 originator interval before its sending). In this way, all the TT
> changes happening between OGM(seqno=X) and OGM(seqno=X+1) will be attached to
> OGM(seqno=X+2) (because when changes happened OGM(seqno=X+1) was already built).
>
> The result of this strategy is that TT changes are not sent within the
> very next OGM, but they are sent within the second OGM since they happened.
> This mechanism will introduce a delay for table resync which could lead to
> wrong state in case of multiple roamings.
>
> This patch delays all the operations so that the OGM is filled and ultimated
> just before being sent.
>
> Signed-off-by: Antonio Quartulli<ordex@autistici.org>
> ---
>   bat_iv_ogm.c |  108 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>   send.c       |   75 ++--------------------------------------
>   types.h      |    6 ++--
>   3 files changed, 99 insertions(+), 90 deletions(-)
>
> diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
> index ed743a4..377ee04 100644
> --- a/bat_iv_ogm.c
> +++ b/bat_iv_ogm.c
> @@ -558,28 +558,97 @@ static void bat_iv_ogm_forward(struct orig_node *orig_node,
>   			     if_incoming, 0, bat_iv_ogm_fwd_send_time());
>   }
>
> -static void bat_iv_ogm_schedule(struct hard_iface *hard_iface,
> -				int tt_num_changes)
> +static void realloc_packet_buffer(struct hard_iface *hard_iface,
> +				  int new_len)
>   {
> -	struct bat_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
> +	unsigned char *new_buff;
> +
> +	new_buff = kmalloc(new_len, GFP_ATOMIC);
> +
> +	/* keep old buffer if kmalloc should fail */
> +	if (new_buff) {
> +		memcpy(new_buff, hard_iface->packet_buff,
> +		       BATMAN_OGM_HLEN);
> +
> +		kfree(hard_iface->packet_buff);
> +		hard_iface->packet_buff = new_buff;
> +		hard_iface->packet_len = new_len;
> +	}
> +}
> +
> +/* when calling this function (hard_iface == primary_if) has to be true */
> +static int prepare_packet_buffer(struct bat_priv *bat_priv,
> +				  struct hard_iface *hard_iface)
> +{
> +	int new_len;
> +
> +	new_len = BATMAN_OGM_HLEN +
> +		  tt_len((uint8_t)atomic_read(&bat_priv->tt_local_changes));
> +
> +	/* if we have too many changes for one packet don't send any
> +	 * and wait for the tt table request which will be fragmented */

Better change that comment te end with */ on a newline, before sending to David :)

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

Re: [B.A.T.M.A.N.] [PATCH 1/5] batman-adv: don't delay OGM information announcement

[Antonio Quartulli]

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

On Wed, Apr 18, 2012 at 10:12:42AM +0200, Martin Hundebøll wrote:
> Hi Antonio,
> 
> One minor/trivial change below.
> 
> On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> > +	/* if we have too many changes for one packet don't send any
> > +	 * and wait for the tt table request which will be fragmented */
> 
> Better change that comment te end with */ on a newline, before sending to David :)

I definitely agree. Thank you. I would really like to see this check in
checkpatch.pl :(:(

Cheers,

-- 
Antonio Quartulli

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

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

Re: [B.A.T.M.A.N.] [PATCH 1/5] batman-adv: don't delay OGM information announcement

[Martin Hundebøll]

On 04/18/2012 10:31 AM, Antonio Quartulli wrote:
> On Wed, Apr 18, 2012 at 10:12:42AM +0200, Martin Hundebøll wrote:
>> Hi Antonio,
>>
>> One minor/trivial change below.
>>
>> On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
>>> +	/* if we have too many changes for one packet don't send any
>>> +	 * and wait for the tt table request which will be fragmented */
>>
>> Better change that comment te end with */ on a newline, before sending to David :)
>
> I definitely agree. Thank you. I would really like to see this check in
> checkpatch.pl :(:(

I think you are free to send a patch to checkpatch.pl :)

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

Re: [B.A.T.M.A.N.] [PATCH 1/5] batman-adv: don't delay OGM information announcement

[Antonio Quartulli]

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

On Wed, Apr 18, 2012 at 10:35:44AM +0200, Martin Hundebøll wrote:
> On 04/18/2012 10:31 AM, Antonio Quartulli wrote:
> > On Wed, Apr 18, 2012 at 10:12:42AM +0200, Martin Hundebøll wrote:
> >> Hi Antonio,
> >>
> >> One minor/trivial change below.
> >>
> >> On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> >>> +	/* if we have too many changes for one packet don't send any
> >>> +	 * and wait for the tt table request which will be fragmented */
> >>
> >> Better change that comment te end with */ on a newline, before sending to David :)
> >
> > I definitely agree. Thank you. I would really like to see this check in
> > checkpatch.pl :(:(
> 
> I think you are free to send a patch to checkpatch.pl :)
> 

Good answer :D


-- 
Antonio Quartulli

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

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

[B.A.T.M.A.N.] [PATCH 2/5] batman-adv: clear ADD+DEL (and viceversa) events in the same orig-interval

[Antonio Quartulli]

During an OGM-interval (time between two different OGM sendings) the same client
could roam away and then roam back to us. In this case the node would add two
events to the events list (that is going to be sent appended to the next OGM). A
DEL one and an ADD one. Obviously they will only increase the overhead (either in
the air and on the receiver side) and eventually trigger wrong states/events
without producing any real effect.

For this reason we can safely delete any ADD event with its related DEL one.

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

diff --git a/translation-table.c b/translation-table.c
index 88e4c8e..e02fa90 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -154,23 +154,48 @@ static void tt_orig_list_entry_free_ref(struct tt_orig_list_entry *orig_entry)
 static void tt_local_event(struct bat_priv *bat_priv, const uint8_t *addr,
 			   uint8_t flags)
 {
-	struct tt_change_node *tt_change_node;
+	struct tt_change_node *tt_change_node, *entry, *safe;
+	bool event_removed = false;
 
 	tt_change_node = kmalloc(sizeof(*tt_change_node), GFP_ATOMIC);
-
 	if (!tt_change_node)
 		return;
-
 	tt_change_node->change.flags = flags;
 	memcpy(tt_change_node->change.addr, addr, ETH_ALEN);
 
+	/* check for ADD+DEL or DEL+ADD events */
 	spin_lock_bh(&bat_priv->tt_changes_list_lock);
+	list_for_each_entry_safe(entry, safe, &bat_priv->tt_changes_list,
+				 list) {
+		if (!compare_eth(entry->change.addr, addr))
+			continue;
+		if (!(!(flags & TT_CLIENT_DEL) && /* ADD op */
+		      entry->change.flags & TT_CLIENT_DEL) &&
+		    !(flags & TT_CLIENT_DEL &&
+		      !(entry->change.flags & TT_CLIENT_DEL))) /* ADD op */
+			continue;
+		/* DEL+ADD in the same orig interval have no effect and can be
+		 * removed to avoid silly behaviour on the receiver side. The
+		 * other way around (ADD+DEL) can happen in case of roaming of
+		 * a client still in the NEW state. Roaming of NEW clients is
+		 * now possible due to automatically recognition of "temporary"
+		 * clients */
+		list_del(&entry->list);
+		kfree(entry);
+		event_removed = true;
+		goto unlock;
+	}
+
 	/* track the change in the OGMinterval list */
 	list_add_tail(&tt_change_node->list, &bat_priv->tt_changes_list);
-	atomic_inc(&bat_priv->tt_local_changes);
+
+unlock:
 	spin_unlock_bh(&bat_priv->tt_changes_list_lock);
 
-	atomic_set(&bat_priv->tt_ogm_append_cnt, 0);
+	if (event_removed)
+		atomic_dec(&bat_priv->tt_local_changes);
+	else
+		atomic_inc(&bat_priv->tt_local_changes);
 }
 
 int tt_len(int changes_num)
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: clear ADD+DEL (and viceversa) events in the same orig-interval

[Martin Hundebøll]

Hi Antonio,

On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> During an OGM-interval (time between two different OGM sendings) the same client
> could roam away and then roam back to us. In this case the node would add two
> events to the events list (that is going to be sent appended to the next OGM). A
> DEL one and an ADD one. Obviously they will only increase the overhead (either in
> the air and on the receiver side) and eventually trigger wrong states/events
> without producing any real effect.
>
> For this reason we can safely delete any ADD event with its related DEL one.
>
> Signed-off-by: Antonio Quartulli<ordex@autistici.org>
> ---
>   translation-table.c |   35 ++++++++++++++++++++++++++++++-----
>   1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/translation-table.c b/translation-table.c
> index 88e4c8e..e02fa90 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -154,23 +154,48 @@ static void tt_orig_list_entry_free_ref(struct tt_orig_list_entry *orig_entry)
>   static void tt_local_event(struct bat_priv *bat_priv, const uint8_t *addr,
>   			   uint8_t flags)
>   {
> -	struct tt_change_node *tt_change_node;
> +	struct tt_change_node *tt_change_node, *entry, *safe;
> +	bool event_removed = false;
>
>   	tt_change_node = kmalloc(sizeof(*tt_change_node), GFP_ATOMIC);
> -
>   	if (!tt_change_node)
>   		return;
> -
>   	tt_change_node->change.flags = flags;
>   	memcpy(tt_change_node->change.addr, addr, ETH_ALEN);
>
> +	/* check for ADD+DEL or DEL+ADD events */
>   	spin_lock_bh(&bat_priv->tt_changes_list_lock);
> +	list_for_each_entry_safe(entry, safe,&bat_priv->tt_changes_list,
> +				 list) {
> +		if (!compare_eth(entry->change.addr, addr))
> +			continue;

Please add an empty line here.

> +		if (!(!(flags&  TT_CLIENT_DEL)&&  /* ADD op */
> +		      entry->change.flags&  TT_CLIENT_DEL)&&
> +		    !(flags&  TT_CLIENT_DEL&&
> +		      !(entry->change.flags&  TT_CLIENT_DEL))) /* ADD op */
> +			continue;

This is messy and hard to unerstand. Couldn't you use some tmp vars like this:

int local_del = (flags & TT_CLIENT_DEL) == TT_CLIENT_DEL;
int change_del = (entry->change.flags & TT_CLIENT_DEL) == TT_CLIENT_DEL;

if (local_del == change_del)
	continue;

I'm not 100% sure I understood the original if correctly, but that just proofs the need to rework it :)

> +		/* DEL+ADD in the same orig interval have no effect and can be
> +		 * removed to avoid silly behaviour on the receiver side. The
> +		 * other way around (ADD+DEL) can happen in case of roaming of
> +		 * a client still in the NEW state. Roaming of NEW clients is
> +		 * now possible due to automatically recognition of "temporary"
> +		 * clients */

Remember newline for */ :)

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: clear ADD+DEL (and viceversa) events in the same orig-interval

[Antonio Quartulli]

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

On Wed, Apr 18, 2012 at 10:33:34AM +0200, Martin Hundebøll wrote:
> Hi Antonio,
> 
> On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> > +	/* check for ADD+DEL or DEL+ADD events */
> >   	spin_lock_bh(&bat_priv->tt_changes_list_lock);
> > +	list_for_each_entry_safe(entry, safe,&bat_priv->tt_changes_list,
> > +				 list) {
> > +		if (!compare_eth(entry->change.addr, addr))
> > +			continue;
> 
> Please add an empty line here.

Is this really needed for some specific reason?

> 
> > +		if (!(!(flags&  TT_CLIENT_DEL)&&  /* ADD op */
> > +		      entry->change.flags&  TT_CLIENT_DEL)&&
> > +		    !(flags&  TT_CLIENT_DEL&&
> > +		      !(entry->change.flags&  TT_CLIENT_DEL))) /* ADD op */
> > +			continue;
> 
> This is messy and hard to unerstand. Couldn't you use some tmp vars like this:
> 
> int local_del = (flags & TT_CLIENT_DEL) == TT_CLIENT_DEL;
> int change_del = (entry->change.flags & TT_CLIENT_DEL) == TT_CLIENT_DEL;
> 
> if (local_del == change_del)
> 	continue;
> 
> I'm not 100% sure I understood the original if correctly, but that just proofs the need to rework it :)

eheh, I know I like to mess up the code with boolean formulas :D
Thank you for your feedback, I will simplify it.

> 
> > +		/* DEL+ADD in the same orig interval have no effect and can be
> > +		 * removed to avoid silly behaviour on the receiver side. The
> > +		 * other way around (ADD+DEL) can happen in case of roaming of
> > +		 * a client still in the NEW state. Roaming of NEW clients is
> > +		 * now possible due to automatically recognition of "temporary"
> > +		 * clients */
> 
> Remember newline for */ :)

yes .-.


Thanks again!


-- 
Antonio Quartulli

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

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

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: clear ADD+DEL (and viceversa) events in the same orig-interval

[Martin Hundebøll]

On 04/18/2012 10:37 AM, Antonio Quartulli wrote:
> On Wed, Apr 18, 2012 at 10:33:34AM +0200, Martin Hundebøll wrote:
>> Hi Antonio,
>>
>> On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
>>> +	/* check for ADD+DEL or DEL+ADD events */
>>>    	spin_lock_bh(&bat_priv->tt_changes_list_lock);
>>> +	list_for_each_entry_safe(entry, safe,&bat_priv->tt_changes_list,
>>> +				 list) {
>>> +		if (!compare_eth(entry->change.addr, addr))
>>> +			continue;
>>
>> Please add an empty line here.
>
> Is this really needed for some specific reason?

No, but I like it that way :)

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

[B.A.T.M.A.N.] [PATCH 3/5] batman-adv: let tt_global_entry_has_orig() return the orig_entry or NULL instead of 1 or 0 only

[Antonio Quartulli]

Instead of returning only 1 or 0 this function has to return the found
orig_entry (if any). In this way, operations that have to to modify the
found orig_entry structure will not need to reiterate over the list again to
find the wanted node.

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

diff --git a/translation-table.c b/translation-table.c
index e02fa90..cd6c2dd 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -543,26 +543,26 @@ static void tt_changes_list_free(struct bat_priv *bat_priv)
 }
 
 /* find out if an orig_node is already in the list of a tt_global_entry.
- * returns 1 if found, 0 otherwise
- */
-static bool tt_global_entry_has_orig(const struct tt_global_entry *entry,
-				     const struct orig_node *orig_node)
+ * returns NULL if not found, the pointer to the orig_entry otherwise */
+static struct tt_orig_list_entry *tt_global_entry_has_orig(
+					const struct tt_global_entry *entry,
+					const struct orig_node *orig_node)
 {
 	struct tt_orig_list_entry *tmp_orig_entry;
 	const struct hlist_head *head;
 	struct hlist_node *node;
-	bool found = false;
+	struct tt_orig_list_entry *orig_entry = NULL;
 
 	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;
+			orig_entry = tmp_orig_entry;
 			break;
 		}
 	}
 	rcu_read_unlock();
-	return found;
+	return orig_entry;
 }
 
 static void tt_global_add_orig_entry(struct tt_global_entry *tt_global_entry,
@@ -1262,7 +1262,7 @@ static int tt_global_valid_entry(const void *entry_ptr, const void *data_ptr)
 	tt_global_entry = container_of(tt_common_entry, struct tt_global_entry,
 				       common);
 
-	return tt_global_entry_has_orig(tt_global_entry, orig_node);
+	return (size_t)tt_global_entry_has_orig(tt_global_entry, orig_node);
 }
 
 static struct sk_buff *tt_response_fill_table(uint16_t tt_len, uint8_t ttvn,
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH 3/5] batman-adv: let tt_global_entry_has_orig() return the orig_entry or NULL instead of 1 or 0 only

[Martin Hundebøll]

Hi Antonio,

On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> Instead of returning only 1 or 0 this function has to return the found
> orig_entry (if any). In this way, operations that have to to modify the
> found orig_entry structure will not need to reiterate over the list again to
> find the wanted node.
>
> Signed-off-by: Antonio Quartulli<ordex@autistici.org>
> ---
>   translation-table.c |   16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/translation-table.c b/translation-table.c
> index e02fa90..cd6c2dd 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -543,26 +543,26 @@ static void tt_changes_list_free(struct bat_priv *bat_priv)
>   }
>
>   /* find out if an orig_node is already in the list of a tt_global_entry.
> - * returns 1 if found, 0 otherwise
> - */
> -static bool tt_global_entry_has_orig(const struct tt_global_entry *entry,
> -				     const struct orig_node *orig_node)
> + * returns NULL if not found, the pointer to the orig_entry otherwise */
> +static struct tt_orig_list_entry *tt_global_entry_has_orig(
> +					const struct tt_global_entry *entry,
> +					const struct orig_node *orig_node)
>   {
>   	struct tt_orig_list_entry *tmp_orig_entry;
>   	const struct hlist_head *head;
>   	struct hlist_node *node;
> -	bool found = false;
> +	struct tt_orig_list_entry *orig_entry = NULL;
>
>   	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;
> +			orig_entry = tmp_orig_entry;
>   			break;
>   		}
>   	}
>   	rcu_read_unlock();
> -	return found;
> +	return orig_entry;
>   }
>
>   static void tt_global_add_orig_entry(struct tt_global_entry *tt_global_entry,
> @@ -1262,7 +1262,7 @@ static int tt_global_valid_entry(const void *entry_ptr, const void *data_ptr)
>   	tt_global_entry = container_of(tt_common_entry, struct tt_global_entry,
>   				       common);
>
> -	return tt_global_entry_has_orig(tt_global_entry, orig_node);
> +	return (size_t)tt_global_entry_has_orig(tt_global_entry, orig_node);
>   }
>
>   static struct sk_buff *tt_response_fill_table(uint16_t tt_len, uint8_t ttvn,

Again, I notice the lack of reference counting, but I guess you already are aware of that, and have it on your never ending todo-list :)

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

[B.A.T.M.A.N.] [PATCH 4/5] batman-adv: update ttvn in case of client reannouncement

[Antonio Quartulli]

in various scenarios it would be possible that a node receives an ADD event for
a client it already knows to belong to the advertiser. In this case the node has
to update the global entry ttvn with the one carried by the OGM.

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

diff --git a/translation-table.c b/translation-table.c
index cd6c2dd..ab295ee 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -593,6 +593,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
 		  bool wifi)
 {
 	struct tt_global_entry *tt_global_entry = NULL;
+	struct tt_orig_list_entry *orig_entry;
 	int ret = 0;
 	int hash_added;
 
@@ -640,9 +641,17 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
 			tt_global_entry->roam_at = 0;
 		}
 
-		if (!tt_global_entry_has_orig(tt_global_entry, orig_node))
+		orig_entry = tt_global_entry_has_orig(tt_global_entry,
+						      orig_node);
+		if (!orig_entry)
 			tt_global_add_orig_entry(tt_global_entry, orig_node,
 						 ttvn);
+		else
+			/* if we are "adding" global entry, we may want to
+			 * update the ttvn anyway. Perhaps the global entry is
+			 * here with a wrong ttvn because it was temporary added
+			 * before */
+			orig_entry->ttvn = ttvn;
 	}
 
 	if (wifi)
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH 4/5] batman-adv: update ttvn in case of client reannouncement

[Martin Hundebøll]

Hi,

On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> in various scenarios it would be possible that a node receives an ADD event for
> a client it already knows to belong to the advertiser. In this case the node has
> to update the global entry ttvn with the one carried by the OGM.
>
> Signed-off-by: Antonio Quartulli<ordex@autistici.org>
> ---
>   translation-table.c |   11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/translation-table.c b/translation-table.c
> index cd6c2dd..ab295ee 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -593,6 +593,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
>   		  bool wifi)
>   {
>   	struct tt_global_entry *tt_global_entry = NULL;
> +	struct tt_orig_list_entry *orig_entry;
>   	int ret = 0;
>   	int hash_added;
>
> @@ -640,9 +641,17 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
>   			tt_global_entry->roam_at = 0;
>   		}
>
> -		if (!tt_global_entry_has_orig(tt_global_entry, orig_node))
> +		orig_entry = tt_global_entry_has_orig(tt_global_entry,
> +						      orig_node);
> +		if (!orig_entry)
>   			tt_global_add_orig_entry(tt_global_entry, orig_node,
>   						 ttvn);
> +		else
> +			/* if we are "adding" global entry, we may want to
> +			 * update the ttvn anyway. Perhaps the global entry is
> +			 * here with a wrong ttvn because it was temporary added
> +			 * before */

One more bad-ending comment block :)

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

[B.A.T.M.A.N.] [PATCH 5/5] batman-adv: beautify tt_global_add() argument list

[Antonio Quartulli]

Instead of adding a new bool argument each time it is needed, it is
better (and simpler) to pass an 8bit flag argument which contains all the
needed flags

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 routing.c           |    2 +-
 translation-table.c |   18 ++++++------------
 translation-table.h |    3 +--
 3 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/routing.c b/routing.c
index 2181a91..22d67bb 100644
--- a/routing.c
+++ b/routing.c
@@ -684,7 +684,7 @@ int recv_roam_adv(struct sk_buff *skb, struct hard_iface *recv_if)
 		roam_adv_packet->src, roam_adv_packet->client);
 
 	tt_global_add(bat_priv, orig_node, roam_adv_packet->client,
-		      atomic_read(&orig_node->last_ttvn) + 1, true, false);
+		      TT_CLIENT_ROAM, atomic_read(&orig_node->last_ttvn) + 1);
 
 	/* Roaming phase starts: I have new information but the ttvn has not
 	 * been incremented yet. This flag will make me check all the incoming
diff --git a/translation-table.c b/translation-table.c
index ab295ee..00757ea 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -589,8 +589,7 @@ static void tt_global_add_orig_entry(struct tt_global_entry *tt_global_entry,
 
 /* caller must hold orig_node refcount */
 int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
-		  const unsigned char *tt_addr, uint8_t ttvn, bool roaming,
-		  bool wifi)
+		  const unsigned char *tt_addr, uint8_t flags, uint8_t ttvn)
 {
 	struct tt_global_entry *tt_global_entry = NULL;
 	struct tt_orig_list_entry *orig_entry;
@@ -600,14 +599,12 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
 	tt_global_entry = tt_global_hash_find(bat_priv, tt_addr);
 
 	if (!tt_global_entry) {
-		tt_global_entry = kzalloc(sizeof(*tt_global_entry),
-					  GFP_ATOMIC);
+		tt_global_entry = kzalloc(sizeof(*tt_global_entry), GFP_ATOMIC);
 		if (!tt_global_entry)
 			goto out;
 
 		memcpy(tt_global_entry->common.addr, tt_addr, ETH_ALEN);
-
-		tt_global_entry->common.flags = NO_FLAGS;
+		tt_global_entry->common.flags = flags;
 		tt_global_entry->roam_at = 0;
 		atomic_set(&tt_global_entry->common.refcount, 2);
 
@@ -654,9 +651,6 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
 			orig_entry->ttvn = ttvn;
 	}
 
-	if (wifi)
-		tt_global_entry->common.flags |= TT_CLIENT_WIFI;
-
 	bat_dbg(DBG_TT, bat_priv,
 		"Creating new global tt entry: %pM (via %pM)\n",
 		tt_global_entry->common.addr, orig_node->orig);
@@ -664,7 +658,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
 out_remove:
 	/* remove address from local hash if present */
 	tt_local_remove(bat_priv, tt_global_entry->common.addr,
-			"global tt received", roaming);
+			"global tt received", flags & TT_CLIENT_ROAM);
 	ret = 1;
 out:
 	if (tt_global_entry)
@@ -1678,9 +1672,9 @@ static void _tt_update_changes(struct bat_priv *bat_priv,
 				      (tt_change + i)->flags & TT_CLIENT_ROAM);
 		else
 			if (!tt_global_add(bat_priv, orig_node,
-					   (tt_change + i)->addr, ttvn, false,
+					   (tt_change + i)->addr,
 					   (tt_change + i)->flags &
-							TT_CLIENT_WIFI))
+							TT_CLIENT_WIFI, ttvn))
 				/* In case of problem while storing a
 				 * global_entry, we stop the updating
 				 * procedure without committing the
diff --git a/translation-table.h b/translation-table.h
index c43374d..3239b72 100644
--- a/translation-table.h
+++ b/translation-table.h
@@ -34,8 +34,7 @@ int tt_local_seq_print_text(struct seq_file *seq, void *offset);
 void tt_global_add_orig(struct bat_priv *bat_priv, struct orig_node *orig_node,
 			const unsigned char *tt_buff, int tt_buff_len);
 int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
-		  const unsigned char *addr, uint8_t ttvn, bool roaming,
-		  bool wifi);
+		  const unsigned char *addr, uint8_t flags, uint8_t ttvn);
 int tt_global_seq_print_text(struct seq_file *seq, void *offset);
 void tt_global_del_orig(struct bat_priv *bat_priv,
 			struct orig_node *orig_node, const char *message);
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH 5/5] batman-adv: beautify tt_global_add() argument list

[Martin Hundebøll]

Hi,

On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> Instead of adding a new bool argument each time it is needed, it is
> better (and simpler) to pass an 8bit flag argument which contains all the
> needed flags
>
> Signed-off-by: Antonio Quartulli<ordex@autistici.org>
> ---
>   routing.c           |    2 +-
>   translation-table.c |   18 ++++++------------
>   translation-table.h |    3 +--
>   3 files changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/routing.c b/routing.c
> index 2181a91..22d67bb 100644
> --- a/routing.c
> +++ b/routing.c
> @@ -684,7 +684,7 @@ int recv_roam_adv(struct sk_buff *skb, struct hard_iface *recv_if)
>   		roam_adv_packet->src, roam_adv_packet->client);
>
>   	tt_global_add(bat_priv, orig_node, roam_adv_packet->client,
> -		      atomic_read(&orig_node->last_ttvn) + 1, true, false);
> +		      TT_CLIENT_ROAM, atomic_read(&orig_node->last_ttvn) + 1);
>
>   	/* Roaming phase starts: I have new information but the ttvn has not
>   	 * been incremented yet. This flag will make me check all the incoming
> diff --git a/translation-table.c b/translation-table.c
> index ab295ee..00757ea 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -589,8 +589,7 @@ static void tt_global_add_orig_entry(struct tt_global_entry *tt_global_entry,
>
>   /* caller must hold orig_node refcount */
>   int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
> -		  const unsigned char *tt_addr, uint8_t ttvn, bool roaming,
> -		  bool wifi)
> +		  const unsigned char *tt_addr, uint8_t flags, uint8_t ttvn)
>   {
>   	struct tt_global_entry *tt_global_entry = NULL;
>   	struct tt_orig_list_entry *orig_entry;
> @@ -600,14 +599,12 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
>   	tt_global_entry = tt_global_hash_find(bat_priv, tt_addr);
>
>   	if (!tt_global_entry) {
> -		tt_global_entry = kzalloc(sizeof(*tt_global_entry),
> -					  GFP_ATOMIC);
> +		tt_global_entry = kzalloc(sizeof(*tt_global_entry), GFP_ATOMIC);

This trivial style-fix doesn't really belong in this patch, does it? (Not that I mind...)

>   		if (!tt_global_entry)
>   			goto out;
>
>   		memcpy(tt_global_entry->common.addr, tt_addr, ETH_ALEN);
> -
> -		tt_global_entry->common.flags = NO_FLAGS;
> +		tt_global_entry->common.flags = flags;
>   		tt_global_entry->roam_at = 0;
>   		atomic_set(&tt_global_entry->common.refcount, 2);
>
> @@ -654,9 +651,6 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
>   			orig_entry->ttvn = ttvn;
>   	}
>
> -	if (wifi)
> -		tt_global_entry->common.flags |= TT_CLIENT_WIFI;
> -
>   	bat_dbg(DBG_TT, bat_priv,
>   		"Creating new global tt entry: %pM (via %pM)\n",
>   		tt_global_entry->common.addr, orig_node->orig);
> @@ -664,7 +658,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
>   out_remove:
>   	/* remove address from local hash if present */
>   	tt_local_remove(bat_priv, tt_global_entry->common.addr,
> -			"global tt received", roaming);
> +			"global tt received", flags&  TT_CLIENT_ROAM);
>   	ret = 1;
>   out:
>   	if (tt_global_entry)
> @@ -1678,9 +1672,9 @@ static void _tt_update_changes(struct bat_priv *bat_priv,
>   				      (tt_change + i)->flags&  TT_CLIENT_ROAM);
>   		else
>   			if (!tt_global_add(bat_priv, orig_node,
> -					   (tt_change + i)->addr, ttvn, false,
> +					   (tt_change + i)->addr,
>   					   (tt_change + i)->flags&
> -							TT_CLIENT_WIFI))
> +							TT_CLIENT_WIFI, ttvn))

If you use a temporary variable for "tt_change + i", you can avoid the last indentation and make the code easier to read...

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net