[v4,5/5] batman-adv: Don't keep redundant TT change events
Commit Message
When adding a local TT twice within the same OGM interval (e.g. happens
when flag get updated), the flags of the first TT change entry is updated
with the second one and both change events is added to the change list.
This leads to having the same ADD change entry twice. Similarly a
DEL+DEL scenario is also creating twice the same event.
Deduplicate ADD+ADD or DEL+DEL scenarios to reduce the TT change events
that need to be sent in both OGM and TT response.
Co-developped-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
net/batman-adv/translation-table.c | 43 +++++++++++++-----------------
1 file changed, 19 insertions(+), 24 deletions(-)
Comments
On Friday, 22 November 2024 16:52:52 CET Remi Pommarel wrote:
> - del_op_entry = entry->change.flags & BATADV_TT_CLIENT_DEL;
>
This line must not be dropped. Just checked my PoC change and it seems like I
already dropped it. I thought I've spotted and fixed this before. But most
likely, I've just changed it in the editor and then forgot to copy it back to
the mail client.
It is also called "Co-Developed-by: " and needs a SoB directly after that
(which I didn't give at that specific point - but now did in batadv/net-next).
I have now changed this directly the batadv/net-next branch. But in case there
needs to be a v6, please also change this locally on your end.
We will still wait for Antonio before I consider it really as accepted (and
then try to send it to the netdev maintainers).
Kind regards,
Sven
On Sun, Nov 24, 2024 at 11:00:40AM +0100, Sven Eckelmann wrote:
> On Friday, 22 November 2024 16:52:52 CET Remi Pommarel wrote:
> > - del_op_entry = entry->change.flags & BATADV_TT_CLIENT_DEL;
> >
>
> This line must not be dropped. Just checked my PoC change and it seems like I
> already dropped it. I thought I've spotted and fixed this before. But most
> likely, I've just changed it in the editor and then forgot to copy it back to
> the mail client.
Outch, sorry for not having noticed that. I am a bit surprised that gcc
does not complain.
>
> It is also called "Co-Developed-by: " and needs a SoB directly after that
> (which I didn't give at that specific point - but now did in batadv/net-next).
Actually according to checkpatch the preferred tag seems to be
"Co-developed-by", which was still not what I used. Also I have mixed
feeling about this tag, I have been asked before to add the
Co-developped-by tag but I don't really feel confortable giving
Signed-off-by in other people name....
Don't know if you noticed, but I also messed up indentation at line
505, my bad.
>
> I have now changed this directly the batadv/net-next branch. But in case there
> needs to be a v6, please also change this locally on your end.
>
> We will still wait for Antonio before I consider it really as accepted (and
> then try to send it to the netdev maintainers).
Thanks.
Regards,
@@ -478,7 +478,7 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv,
del_op_requested = flags & BATADV_TT_CLIENT_DEL;
- /* check for ADD+DEL or DEL+ADD events */
+ /* check for ADD+DEL, DEL+ADD, ADD+ADD or DEL+DEL events */
spin_lock_bh(&bat_priv->tt.changes_list_lock);
changes = READ_ONCE(bat_priv->tt.local_changes);
list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list,
@@ -486,30 +486,25 @@ 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 in the same originator interval. It
- * means that flags have been changed: update them!
- */
- if (!del_op_requested && !del_op_entry)
- entry->change.flags = flags;
+ if (del_op_requested != del_op_entry) {
+ /* 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);
+ changes--;
+ } else {
+ /* 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
+ */
+ entry->change.flags = flags;
+ }
- continue;
-del:
- list_del(&entry->list);
- kmem_cache_free(batadv_tt_change_cache, entry);
- changes--;
kmem_cache_free(batadv_tt_change_cache, tt_change_node);
goto update_changes;
}