Re: [PATCH v3 5/5] batman-adv: Don't keep redundant TT change events

[Sven Eckelmann <sven@narfation.org>]

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

On Wednesday, 20 November 2024 18:47:18 CET Remi Pommarel wrote:
> -               /* this is a second add in the same originator interval. It
> -                * means that flags have been changed: update them!
> +               /* this is a second add or del in the same originator interval.
> +                * It could mean that flags have been changed (e.g. double
> +                * add): update them
>                  */
> -               if (!del_op_requested && !del_op_entry)
> +               if (del_op_requested == del_op_entry) {
>                         entry->change.flags = flags;
> +                       goto discard;
> +               }
>  
>                 continue;

>From logic perspective, the check would be irrelevant - and the continue never happens.


    if (!del_op_requested && del_op_entry)
    	goto del;
    if (del_op_requested && !del_op_entry)
    	goto del;


Implies that del_op_requested == del_op_entry. So the check wouldn't make too 
much sense. Maybe we should clean up this logic further:


diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 437c4edd..b349851b 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -484,18 +484,7 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv,
 		if (!batadv_compare_eth(entry->change.addr, common->addr))
 			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
-		 */
 		del_op_entry = entry->change.flags & BATADV_TT_CLIENT_DEL;
-		if (!del_op_requested && del_op_entry)
-			goto del;
-		if (del_op_requested && !del_op_entry)
-			goto del;
 
 		/* this is a second add or del in the same originator interval.
 		 * It could mean that flags have been changed (e.g. double
@@ -506,8 +495,13 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv,
 			goto discard;
 		}
 
-		continue;
-del:
+		/* 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);
 		kmem_cache_free(batadv_tt_change_cache, entry);
 		bat_priv->tt.local_changes--;

Kind regards,
	Sven

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