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

[Remi Pommarel <repk@triplefau.lt>]

On Thu, Nov 21, 2024 at 09:43:01AM +0100, Sven Eckelmann wrote:
> 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--;
> 

Looks right to me, I would even simplify that more for readability with:


 		 * 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
-		 * add): update them
-		 */
-		if (del_op_requested == del_op_entry) {
-			entry->change.flags = flags;
-			goto discard;
+		if (del_op_requested != del_op_entry) {
+			list_del(&entry->list);
+			kmem_cache_free(batadv_tt_change_cache, entry);
+			bat_priv->tt.local_changes--;
 		}
-
-		continue;
-del:
-		list_del(&entry->list);
-		kmem_cache_free(batadv_tt_change_cache, entry);
-		bat_priv->tt.local_changes--;
 discard:
 		kmem_cache_free(batadv_tt_change_cache, tt_change_node);
 		goto unlock;

-- 
Remi