The first three patches are actual fixes.
The first two try to avoid sending uninitialized data that could be interpreted as invalid TT change events in both TT change response and OGM. Following invalid entries could be seen when that happen with batctl o:
* 00:00:00:00:00:00 -1 [....] ( 0) 88:12:4e:ad:7e:ba (179) (0x45845380) * 00:00:00:00:78:79 4092 [.W..] ( 0) 88:12:4e:ad:7e:3c (145) (0x8ebadb8b)
The third one fixes an issue that happened when a TT change event list is too big for the MTU, the list was never actually sent nor free and continued to grow indefinitely from this point. That also caused the OGM TTVN to increase at each OGM interval without any changes being ever visible to other nodes. This ever growing TT change event list could be observed by looking at /sys/kernel/slab/batadv_tt_change_cache/objects that sometimes showed unusal high value even after issuing a memcache shrink.
The next two patches are more cleanup / potential slight improvements. While patch 4 is mainly cosmetic (having negative tt.local_changes values is not exactly an issue), patch 5 is here to keep the TT changes list as short as possible (reducing network overhead).
V3: - Fix commit message wording - Update outdated comments
V2: - This has been tested enough to not be in RFC state anymore - Add one more uninitialize TT change fix for full table TT responses
Remi Pommarel (5): batman-adv: Do not send uninitialized TT changes batman-adv: Remove uninitialized data in full table TT response batman-adv: Do not let TT changes list grows indefinitely batman-adv: Remove atomic usage for tt.local_changes batman-adv: Don't keep redundant TT change events
net/batman-adv/soft-interface.c | 2 +- net/batman-adv/translation-table.c | 92 ++++++++++++++++++------------ net/batman-adv/types.h | 4 +- 3 files changed, 60 insertions(+), 38 deletions(-)
The number of TT changes can be less than initially expected in batadv_tt_tvlv_container_update() (changes can be removed by batadv_tt_local_event() in ADD+DEL sequence between reading tt_diff_entries_num and actually iterating the change list under lock).
Thus tt_diff_len could be bigger than the actual changes size that need to be sent. Because batadv_send_my_tt_response sends the whole packet, uninitialized data can be interpreted as TT changes on other nodes leading to weird TT global entries on those nodes such as:
* 00:00:00:00:00:00 -1 [....] ( 0) 88:12:4e:ad:7e:ba (179) (0x45845380) * 00:00:00:00:78:79 4092 [.W..] ( 0) 88:12:4e:ad:7e:3c (145) (0x8ebadb8b)
All of the above also applies to OGM tvlv container buffer's tvlv_len.
Remove the extra allocated space to avoid sending uninitialized TT changes in batadv_send_my_tt_response() and batadv_v_ogm_send_softif().
Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs") Signed-off-by: Remi Pommarel repk@triplefau.lt --- net/batman-adv/translation-table.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 2243cec18ecc..f0590f9bc2b1 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0; int tt_diff_entries_count = 0; + size_t tt_extra_len = 0; u16 tvlv_len;
tt_diff_entries_num = atomic_read(&bat_priv->tt.local_changes); @@ -1027,6 +1028,9 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) } spin_unlock_bh(&bat_priv->tt.changes_list_lock);
+ tt_extra_len = batadv_tt_len(tt_diff_entries_num - + tt_diff_entries_count); + /* Keep the buffer for possible tt_request */ spin_lock_bh(&bat_priv->tt.last_changeset_lock); kfree(bat_priv->tt.last_changeset); @@ -1035,6 +1039,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) tt_change_len = batadv_tt_len(tt_diff_entries_count); /* check whether this new OGM has no changes due to size problems */ if (tt_diff_entries_count > 0) { + tt_diff_len -= tt_extra_len; /* if kmalloc() fails we will reply with the full table * instead of providing the diff */ @@ -1047,6 +1052,8 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) } spin_unlock_bh(&bat_priv->tt.last_changeset_lock);
+ /* Remove extra packet space for OGM */ + tvlv_len -= tt_extra_len; container_register: batadv_tvlv_container_register(bat_priv, BATADV_TVLV_TT, 1, tt_data, tvlv_len);
On 20/11/2024 18:47, Remi Pommarel wrote:
The number of TT changes can be less than initially expected in batadv_tt_tvlv_container_update() (changes can be removed by batadv_tt_local_event() in ADD+DEL sequence between reading tt_diff_entries_num and actually iterating the change list under lock).
Thus tt_diff_len could be bigger than the actual changes size that need to be sent. Because batadv_send_my_tt_response sends the whole packet, uninitialized data can be interpreted as TT changes on other nodes leading to weird TT global entries on those nodes such as:
- 00:00:00:00:00:00 -1 [....] ( 0) 88:12:4e:ad:7e:ba (179) (0x45845380)
- 00:00:00:00:78:79 4092 [.W..] ( 0) 88:12:4e:ad:7e:3c (145) (0x8ebadb8b)
All of the above also applies to OGM tvlv container buffer's tvlv_len.
Remove the extra allocated space to avoid sending uninitialized TT changes in batadv_send_my_tt_response() and batadv_v_ogm_send_softif().
Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs") Signed-off-by: Remi Pommarel repk@triplefau.lt
net/batman-adv/translation-table.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 2243cec18ecc..f0590f9bc2b1 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0; int tt_diff_entries_count = 0;
size_t tt_extra_len = 0; u16 tvlv_len;
tt_diff_entries_num = atomic_read(&bat_priv->tt.local_changes);
@@ -1027,6 +1028,9 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) } spin_unlock_bh(&bat_priv->tt.changes_list_lock);
what speaks against acquiring tt.changes_list_lock before reading tt.local_changes? (and making sure the writer does the update under lock too) Any reason for not pursuing that path?
That should guarantee consistency across the counter and the list.
We could also have the situation where an ADD only comes between reading the counter and iterating over the list. In this case we don't send random memory, but we still send a somewhat inconsistent changeset.
Regards,
On Thu, Nov 21, 2024 at 02:05:58PM +0100, Antonio Quartulli wrote:
On 20/11/2024 18:47, Remi Pommarel wrote:
The number of TT changes can be less than initially expected in batadv_tt_tvlv_container_update() (changes can be removed by batadv_tt_local_event() in ADD+DEL sequence between reading tt_diff_entries_num and actually iterating the change list under lock).
Thus tt_diff_len could be bigger than the actual changes size that need to be sent. Because batadv_send_my_tt_response sends the whole packet, uninitialized data can be interpreted as TT changes on other nodes leading to weird TT global entries on those nodes such as:
- 00:00:00:00:00:00 -1 [....] ( 0) 88:12:4e:ad:7e:ba (179) (0x45845380)
- 00:00:00:00:78:79 4092 [.W..] ( 0) 88:12:4e:ad:7e:3c (145) (0x8ebadb8b)
All of the above also applies to OGM tvlv container buffer's tvlv_len.
Remove the extra allocated space to avoid sending uninitialized TT changes in batadv_send_my_tt_response() and batadv_v_ogm_send_softif().
Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs") Signed-off-by: Remi Pommarel repk@triplefau.lt
net/batman-adv/translation-table.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 2243cec18ecc..f0590f9bc2b1 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0; int tt_diff_entries_count = 0;
- size_t tt_extra_len = 0; u16 tvlv_len; tt_diff_entries_num = atomic_read(&bat_priv->tt.local_changes);
@@ -1027,6 +1028,9 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) } spin_unlock_bh(&bat_priv->tt.changes_list_lock);
what speaks against acquiring tt.changes_list_lock before reading tt.local_changes? (and making sure the writer does the update under lock too) Any reason for not pursuing that path?
Nothing against, just tried to follow old behavior in case this was that way for performance reasons. That would mean batadv_tt_local_commit_changes_nolock() to take the lock; because it is only called once per OGM interval I think that would be ok.
And now that I've looked into batadv_tt_tvlv_container_update() a bit deeper I realize that it calls batadv_tt_prepare_tvlv_local_data() every time, traversing the softif_vlan_list and building the tvlv container over and over even if no changes occured (I mean even after the two OGM that repeat last changes in case the first one has been lost).
Using lock for consistency maybe I can try to avoid that also ?
Thanks,
On Thu, Nov 21, 2024 at 02:56:17PM +0100, Remi Pommarel wrote:
On Thu, Nov 21, 2024 at 02:05:58PM +0100, Antonio Quartulli wrote:
On 20/11/2024 18:47, Remi Pommarel wrote:
The number of TT changes can be less than initially expected in batadv_tt_tvlv_container_update() (changes can be removed by batadv_tt_local_event() in ADD+DEL sequence between reading tt_diff_entries_num and actually iterating the change list under lock).
Thus tt_diff_len could be bigger than the actual changes size that need to be sent. Because batadv_send_my_tt_response sends the whole packet, uninitialized data can be interpreted as TT changes on other nodes leading to weird TT global entries on those nodes such as:
- 00:00:00:00:00:00 -1 [....] ( 0) 88:12:4e:ad:7e:ba (179) (0x45845380)
- 00:00:00:00:78:79 4092 [.W..] ( 0) 88:12:4e:ad:7e:3c (145) (0x8ebadb8b)
All of the above also applies to OGM tvlv container buffer's tvlv_len.
Remove the extra allocated space to avoid sending uninitialized TT changes in batadv_send_my_tt_response() and batadv_v_ogm_send_softif().
Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs") Signed-off-by: Remi Pommarel repk@triplefau.lt
net/batman-adv/translation-table.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 2243cec18ecc..f0590f9bc2b1 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0; int tt_diff_entries_count = 0;
- size_t tt_extra_len = 0; u16 tvlv_len; tt_diff_entries_num = atomic_read(&bat_priv->tt.local_changes);
@@ -1027,6 +1028,9 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) } spin_unlock_bh(&bat_priv->tt.changes_list_lock);
what speaks against acquiring tt.changes_list_lock before reading tt.local_changes? (and making sure the writer does the update under lock too) Any reason for not pursuing that path?
Nothing against, just tried to follow old behavior in case this was that way for performance reasons. That would mean batadv_tt_local_commit_changes_nolock() to take the lock; because it is only called once per OGM interval I think that would be ok.
Actually I initialy though that holding this lock in batadv_tt_local_commit_changes_nolock() would be fine, but because it purges client and updates crc (two fairly intensive operations), that could be a bad idea to hold the lock that long.
So batadv_tt_local_commit_changes_nolock() would still need to get tt.local_changes without the lock (needing READ_ONCE), hence updates would need WRITE_ONCE to avoid store tearing as discussed with Sven.
So the patch would be quite similar, only tt->tt.changes_list_lock will be taken sooner in batadv_tt_tvlv_container_update().
That will fix the ADD between two read situation as you described though.
Do you still prefer this option ?
Thanks
On Thursday, 21 November 2024 16:07:24 CET Remi Pommarel wrote:
So the patch would be quite similar, only tt->tt.changes_list_lock will be taken sooner in batadv_tt_tvlv_container_update().
That will fix the ADD between two read situation as you described though.
Do you still prefer this option ?
I can't speak for Antonio but I think I would prefer for the fix the current version. The locking one would end up again with nested spinlocks and maybe more refactoring. And it might be nicer for the stable backports to have less noise in the patch.
Btw. just noticed that the code (not in your change - but overall) for the filling of diff entries could have been something like:
--- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -980,6 +980,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_entries_count = 0; bool drop_changes = false; size_t tt_extra_len = 0; + LIST_HEAD(tt_diffs); u16 tvlv_len;
tt_diff_entries_num = READ_ONCE(bat_priv->tt.local_changes); @@ -1011,9 +1012,10 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv)
spin_lock_bh(&bat_priv->tt.changes_list_lock); bat_priv->tt.local_changes = 0; + list_splice_init(&bat_priv->tt.changes_list, &tt_diffs); + spin_unlock_bh(&bat_priv->tt.changes_list_lock);
- list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list, - list) { + list_for_each_entry_safe(entry, safe, &tt_diffs, list) { if (tt_diff_entries_count < tt_diff_entries_num) { memcpy(tt_change + tt_diff_entries_count, &entry->change, @@ -1023,7 +1025,6 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) list_del(&entry->list); kmem_cache_free(batadv_tt_change_cache, entry); } - spin_unlock_bh(&bat_priv->tt.changes_list_lock);
tt_extra_len = batadv_tt_len(tt_diff_entries_num - tt_diff_entries_count);
And then you can also move this before "tt_diff_entries_num = ..." and save the corresponding bat_priv->tt.local_changes for the spliced list to the inside the lock also in a local variable. And then operate on this variable for the other decisions. Of course, you must still clean the local list in case of an error. Which of course would slightly change the behavior in case of an allocation error in batadv_tt_prepare_tvlv_local_data (which would previously kept the list as it was).
But if it would be done like this then we could also remove the READ_ONCE and not introduce the WRITE_ONCE - just because local_changes is only touched inside a locked area (see changes_list_lock).
Please double check these statements - this was just a simple brain dump.
Kind regards, Sven
The number of entries filled by batadv_tt_tvlv_generate() can be less than initially expected in batadv_tt_prepare_tvlv_{global,local}_data() (changes can be removed by batadv_tt_local_event() in ADD+DEL sequence in the meantime as the lock held during the whole tvlv global/local data generation).
Thus tvlv_len could be bigger than the actual TT entry size that need to be sent so full table TT_RESPONSE could hold invalid TT entries such as below.
* 00:00:00:00:00:00 -1 [....] ( 0) 88:12:4e:ad:7e:ba (179) (0x45845380) * 00:00:00:00:78:79 4092 [.W..] ( 0) 88:12:4e:ad:7e:3c (145) (0x8ebadb8b)
Remove the extra allocated space to avoid sending uninitialized entries for full table TT_RESPONSE in both batadv_send_other_tt_response() and batadv_send_my_tt_response().
Fixes: 7ea7b4a14275 ("batman-adv: make the TT CRC logic VLAN specific") Signed-off-by: Remi Pommarel repk@triplefau.lt --- net/batman-adv/translation-table.c | 37 ++++++++++++++++++------------ 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index f0590f9bc2b1..bbab7491c83f 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -2754,14 +2754,16 @@ static bool batadv_tt_global_valid(const void *entry_ptr, * * Fills the tvlv buff with the tt entries from the specified hash. If valid_cb * is not provided then this becomes a no-op. + * + * Return: Remaining unused length in tvlv_buff. */ -static void batadv_tt_tvlv_generate(struct batadv_priv *bat_priv, - struct batadv_hashtable *hash, - void *tvlv_buff, u16 tt_len, - bool (*valid_cb)(const void *, - const void *, - u8 *flags), - void *cb_data) +static u16 batadv_tt_tvlv_generate(struct batadv_priv *bat_priv, + struct batadv_hashtable *hash, + void *tvlv_buff, u16 tt_len, + bool (*valid_cb)(const void *, + const void *, + u8 *flags), + void *cb_data) { struct batadv_tt_common_entry *tt_common_entry; struct batadv_tvlv_tt_change *tt_change; @@ -2775,7 +2777,7 @@ static void batadv_tt_tvlv_generate(struct batadv_priv *bat_priv, tt_change = tvlv_buff;
if (!valid_cb) - return; + return tt_len;
rcu_read_lock(); for (i = 0; i < hash->size; i++) { @@ -2801,6 +2803,8 @@ static void batadv_tt_tvlv_generate(struct batadv_priv *bat_priv, } } rcu_read_unlock(); + + return batadv_tt_len(tt_tot - tt_num_entries); }
/** @@ -3076,10 +3080,11 @@ static bool batadv_send_other_tt_response(struct batadv_priv *bat_priv, goto out;
/* fill the rest of the tvlv with the real TT entries */ - batadv_tt_tvlv_generate(bat_priv, bat_priv->tt.global_hash, - tt_change, tt_len, - batadv_tt_global_valid, - req_dst_orig_node); + tvlv_len -= batadv_tt_tvlv_generate(bat_priv, + bat_priv->tt.global_hash, + tt_change, tt_len, + batadv_tt_global_valid, + req_dst_orig_node); }
/* Don't send the response, if larger than fragmented packet. */ @@ -3203,9 +3208,11 @@ static bool batadv_send_my_tt_response(struct batadv_priv *bat_priv, goto out;
/* fill the rest of the tvlv with the real TT entries */ - batadv_tt_tvlv_generate(bat_priv, bat_priv->tt.local_hash, - tt_change, tt_len, - batadv_tt_local_valid, NULL); + tvlv_len -= batadv_tt_tvlv_generate(bat_priv, + bat_priv->tt.local_hash, + tt_change, tt_len, + batadv_tt_local_valid, + NULL); }
tvlv_tt_data->flags = BATADV_TT_RESPONSE;
On 20/11/2024 18:47, Remi Pommarel wrote:
The number of entries filled by batadv_tt_tvlv_generate() can be less than initially expected in batadv_tt_prepare_tvlv_{global,local}_data() (changes can be removed by batadv_tt_local_event() in ADD+DEL sequence in the meantime as the lock held during the whole tvlv global/local data generation).
Thus tvlv_len could be bigger than the actual TT entry size that need to be sent so full table TT_RESPONSE could hold invalid TT entries such as below.
- 00:00:00:00:00:00 -1 [....] ( 0) 88:12:4e:ad:7e:ba (179) (0x45845380)
- 00:00:00:00:78:79 4092 [.W..] ( 0) 88:12:4e:ad:7e:3c (145) (0x8ebadb8b)
Remove the extra allocated space to avoid sending uninitialized entries for full table TT_RESPONSE in both batadv_send_other_tt_response() and batadv_send_my_tt_response().
Fixes: 7ea7b4a14275 ("batman-adv: make the TT CRC logic VLAN specific") Signed-off-by: Remi Pommarel repk@triplefau.lt
I am wondering if acquiring a lock around the entire section can make sense. Also in this case it would cover the case where we get an ADD in between.
What do you think?
Regards,
On Thursday, 21 November 2024 14:14:13 CET Antonio Quartulli wrote:
I am wondering if acquiring a lock around the entire section can make sense. Also in this case it would cover the case where we get an ADD in between.
What do you think?
So you would change the hash with its per-bucket spinlocks to a global spinlock? Just imagine that I am now locking angry at you. Especially because I still hear the people asking for a migration from the batadv hash the rhashtable.
Kind regards, Sven
When TT changes list is too big to fit in packet due to MTU size, an empty OGM is sent expected other node to send TT request to get the changes. The issue is that tt.last_changeset was not built thus the originator was responding with previous changes to those TT requests (see batadv_send_my_tt_response). Also the changes list was never cleaned up effectively never ending growing from this point onwards, repeatedly sending the same TT response changes over and over, and a creating a new empty OGM every OGM interval expecting for the local changes to be purged.
When there is more TT changes that can fit in packet, drop all changes, send empty OGM and wait for TT request so we can respond with a full table instead.
Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs") Signed-off-by: Remi Pommarel repk@triplefau.lt --- net/batman-adv/translation-table.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index bbab7491c83f..d7b43868b624 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0; int tt_diff_entries_count = 0; + bool drop_changes = false; size_t tt_extra_len = 0; u16 tvlv_len;
@@ -997,21 +998,29 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) tt_diff_len = batadv_tt_len(tt_diff_entries_num);
/* if we have too many changes for one packet don't send any - * and wait for the tt table request which will be fragmented + * and wait for the tt table request so we can reply with the full + * table. + * + * The local change history should still be cleaned up or it will only + * grow from this point onwards. Also tt.last_changeset should be set + * to NULL so tt response could send the full table instead of diff. */ - if (tt_diff_len > bat_priv->soft_iface->mtu) + if (tt_diff_len > bat_priv->soft_iface->mtu) { tt_diff_len = 0; + tt_diff_entries_num = 0; + drop_changes = true; + }
tvlv_len = batadv_tt_prepare_tvlv_local_data(bat_priv, &tt_data, &tt_change, &tt_diff_len); if (!tvlv_len) return;
- tt_data->flags = BATADV_TT_OGM_DIFF; - - if (tt_diff_len == 0) + if (!drop_changes && tt_diff_len == 0) goto container_register;
+ tt_data->flags = BATADV_TT_OGM_DIFF; + spin_lock_bh(&bat_priv->tt.changes_list_lock); atomic_set(&bat_priv->tt.local_changes, 0);
On 20/11/2024 18:47, Remi Pommarel wrote:
When TT changes list is too big to fit in packet due to MTU size, an empty OGM is sent expected other node to send TT request to get the changes. The issue is that tt.last_changeset was not built thus the originator was responding with previous changes to those TT requests (see batadv_send_my_tt_response). Also the changes list was never cleaned up effectively never ending growing from this point onwards, repeatedly sending the same TT response changes over and over, and a creating a new empty OGM every OGM interval expecting for the local changes to be purged.
nice catch!
When there is more TT changes that can fit in packet, drop all changes, send empty OGM and wait for TT request so we can respond with a full table instead.
Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs") Signed-off-by: Remi Pommarel repk@triplefau.lt
net/batman-adv/translation-table.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index bbab7491c83f..d7b43868b624 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0; int tt_diff_entries_count = 0;
- bool drop_changes = false; size_t tt_extra_len = 0; u16 tvlv_len;
@@ -997,21 +998,29 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) tt_diff_len = batadv_tt_len(tt_diff_entries_num);
/* if we have too many changes for one packet don't send any
* and wait for the tt table request which will be fragmented
* and wait for the tt table request so we can reply with the full* table.
I'd still say "(fragmented) table", in order to give the reader a clue about how we're going to handle this.
** The local change history should still be cleaned up or it will only* grow from this point onwards. Also tt.last_changeset should be set* to NULL so tt response could send the full table instead of diff.
Personally I'd not add these details. I'd just say that the history "should still be cleaned up as we get ready for the next TT round". Or something along those lines. The rest is just a consequence of the "preparation".
*/
- if (tt_diff_len > bat_priv->soft_iface->mtu)
if (tt_diff_len > bat_priv->soft_iface->mtu) { tt_diff_len = 0;
tt_diff_entries_num = 0;drop_changes = true;}
tvlv_len = batadv_tt_prepare_tvlv_local_data(bat_priv, &tt_data, &tt_change, &tt_diff_len); if (!tvlv_len) return;
- tt_data->flags = BATADV_TT_OGM_DIFF;
- if (tt_diff_len == 0)
if (!drop_changes && tt_diff_len == 0) goto container_register;
tt_data->flags = BATADV_TT_OGM_DIFF;
hmm there is no mention in the commit message as to why we're moving this assignment. Why is that?
[And I just saw that this flag is never used.......]
Regards,
- spin_lock_bh(&bat_priv->tt.changes_list_lock); atomic_set(&bat_priv->tt.local_changes, 0);
On Thu, Nov 21, 2024 at 02:50:18PM +0100, Antonio Quartulli wrote:
On 20/11/2024 18:47, Remi Pommarel wrote:
When TT changes list is too big to fit in packet due to MTU size, an empty OGM is sent expected other node to send TT request to get the changes. The issue is that tt.last_changeset was not built thus the originator was responding with previous changes to those TT requests (see batadv_send_my_tt_response). Also the changes list was never cleaned up effectively never ending growing from this point onwards, repeatedly sending the same TT response changes over and over, and a creating a new empty OGM every OGM interval expecting for the local changes to be purged.
nice catch!
When there is more TT changes that can fit in packet, drop all changes, send empty OGM and wait for TT request so we can respond with a full table instead.
Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs") Signed-off-by: Remi Pommarel repk@triplefau.lt
net/batman-adv/translation-table.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index bbab7491c83f..d7b43868b624 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0; int tt_diff_entries_count = 0;
- bool drop_changes = false; size_t tt_extra_len = 0; u16 tvlv_len;
@@ -997,21 +998,29 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) tt_diff_len = batadv_tt_len(tt_diff_entries_num); /* if we have too many changes for one packet don't send any
* and wait for the tt table request which will be fragmented
* and wait for the tt table request so we can reply with the full* table.I'd still say "(fragmented) table", in order to give the reader a clue about how we're going to handle this.
** The local change history should still be cleaned up or it will only* grow from this point onwards. Also tt.last_changeset should be set* to NULL so tt response could send the full table instead of diff.Personally I'd not add these details. I'd just say that the history "should still be cleaned up as we get ready for the next TT round". Or something along those lines. The rest is just a consequence of the "preparation".
*/
- if (tt_diff_len > bat_priv->soft_iface->mtu)
- if (tt_diff_len > bat_priv->soft_iface->mtu) { tt_diff_len = 0;
tt_diff_entries_num = 0;drop_changes = true;- } tvlv_len = batadv_tt_prepare_tvlv_local_data(bat_priv, &tt_data, &tt_change, &tt_diff_len); if (!tvlv_len) return;
- tt_data->flags = BATADV_TT_OGM_DIFF;
- if (tt_diff_len == 0)
- if (!drop_changes && tt_diff_len == 0) goto container_register;
- tt_data->flags = BATADV_TT_OGM_DIFF;
hmm there is no mention in the commit message as to why we're moving this assignment. Why is that?
No reason, the if is supposed to be after this flag, thanks.
[And I just saw that this flag is never used.......]
Thanks for the review.
The tt.local_changes atomic is either written with tt.changes_list_lock or close to it (see batadv_tt_local_event()). Thus the performance gain using an atomic was limited (or because of atomic_read() impact even negative). Using atomic also comes with the need to be wary of potential negative tt.local_changes value.
Simplify the tt.local_changes usage by removing the atomic property and modifying it only with tt.changes_list_lock held.
Signed-off-by: Remi Pommarel repk@triplefau.lt --- net/batman-adv/soft-interface.c | 2 +- net/batman-adv/translation-table.c | 17 ++++++----------- net/batman-adv/types.h | 4 ++-- 3 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index 2758aba47a2f..2575f13992d2 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -783,13 +783,13 @@ static int batadv_softif_init_late(struct net_device *dev) atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE); atomic_set(&bat_priv->bcast_seqno, 1); atomic_set(&bat_priv->tt.vn, 0); - atomic_set(&bat_priv->tt.local_changes, 0); atomic_set(&bat_priv->tt.ogm_append_cnt, 0); #ifdef CONFIG_BATMAN_ADV_BLA atomic_set(&bat_priv->bla.num_requests, 0); #endif atomic_set(&bat_priv->tp_num, 0);
+ bat_priv->tt.local_changes = 0; bat_priv->tt.last_changeset = NULL; bat_priv->tt.last_changeset_len = 0; bat_priv->isolation_mark = 0; diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index d7b43868b624..39704af81169 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -463,7 +463,6 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv, struct batadv_tt_change_node *tt_change_node, *entry, *safe; struct batadv_tt_common_entry *common = &tt_local_entry->common; u8 flags = common->flags | event_flags; - bool event_removed = false; bool del_op_requested, del_op_entry;
tt_change_node = kmem_cache_alloc(batadv_tt_change_cache, GFP_ATOMIC); @@ -508,21 +507,17 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv, del: list_del(&entry->list); kmem_cache_free(batadv_tt_change_cache, entry); + bat_priv->tt.local_changes--; kmem_cache_free(batadv_tt_change_cache, tt_change_node); - event_removed = true; goto unlock; }
/* track the change in the OGMinterval list */ list_add_tail(&tt_change_node->list, &bat_priv->tt.changes_list); + bat_priv->tt.local_changes++;
unlock: spin_unlock_bh(&bat_priv->tt.changes_list_lock); - - if (event_removed) - atomic_dec(&bat_priv->tt.local_changes); - else - atomic_inc(&bat_priv->tt.local_changes); }
/** @@ -994,7 +989,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) size_t tt_extra_len = 0; u16 tvlv_len;
- tt_diff_entries_num = atomic_read(&bat_priv->tt.local_changes); + tt_diff_entries_num = READ_ONCE(bat_priv->tt.local_changes); tt_diff_len = batadv_tt_len(tt_diff_entries_num);
/* if we have too many changes for one packet don't send any @@ -1022,7 +1017,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) tt_data->flags = BATADV_TT_OGM_DIFF;
spin_lock_bh(&bat_priv->tt.changes_list_lock); - atomic_set(&bat_priv->tt.local_changes, 0); + bat_priv->tt.local_changes = 0;
list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list, list) { @@ -1438,7 +1433,7 @@ static void batadv_tt_changes_list_free(struct batadv_priv *bat_priv) kmem_cache_free(batadv_tt_change_cache, entry); }
- atomic_set(&bat_priv->tt.local_changes, 0); + bat_priv->tt.local_changes = 0; spin_unlock_bh(&bat_priv->tt.changes_list_lock); }
@@ -3708,7 +3703,7 @@ static void batadv_tt_local_commit_changes_nolock(struct batadv_priv *bat_priv) { lockdep_assert_held(&bat_priv->tt.commit_lock);
- if (atomic_read(&bat_priv->tt.local_changes) < 1) { + if (READ_ONCE(bat_priv->tt.local_changes) < 1) { if (!batadv_atomic_dec_not_zero(&bat_priv->tt.ogm_append_cnt)) batadv_tt_tvlv_container_update(bat_priv); return; diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 04f6398b3a40..f491bff8c51b 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -1022,7 +1022,7 @@ struct batadv_priv_tt { atomic_t ogm_append_cnt;
/** @local_changes: changes registered in an originator interval */ - atomic_t local_changes; + size_t local_changes;
/** * @changes_list: tracks tt local changes within an originator interval @@ -1044,7 +1044,7 @@ struct batadv_priv_tt { */ struct list_head roam_list;
- /** @changes_list_lock: lock protecting changes_list */ + /** @changes_list_lock: lock protecting changes_list & local_changes */ spinlock_t changes_list_lock;
/** @req_list_lock: lock protecting req_list */
On Wednesday, 20 November 2024 18:47:17 CET Remi Pommarel wrote:
The tt.local_changes atomic is either written with tt.changes_list_lock or close to it (see batadv_tt_local_event()). Thus the performance gain using an atomic was limited (or because of atomic_read() impact even negative). Using atomic also comes with the need to be wary of potential negative tt.local_changes value.
Simplify the tt.local_changes usage by removing the atomic property and modifying it only with tt.changes_list_lock held.
The overall change assumes that the compiler never splits writes (store tearing) [1]. Of course, writes are protected against each other using locks. But the reader is no longer protected from partial writes. I haven't checked whether store fusing might be a problem.
Kind regards, Sven
[1] https://www.kernel.org/doc/html/latest/core-api/wrappers/memory-barriers.htm...
[...]
@@ -783,13 +783,13 @@ static int batadv_softif_init_late(struct net_device *dev) atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE); atomic_set(&bat_priv->bcast_seqno, 1); atomic_set(&bat_priv->tt.vn, 0);
- atomic_set(&bat_priv->tt.local_changes, 0); atomic_set(&bat_priv->tt.ogm_append_cnt, 0);
#ifdef CONFIG_BATMAN_ADV_BLA atomic_set(&bat_priv->bla.num_requests, 0); #endif atomic_set(&bat_priv->tp_num, 0);
- bat_priv->tt.local_changes = 0;
Would need WRITE_ONCE (just to be consistent)
[...]
@@ -508,21 +507,17 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv, del: list_del(&entry->list); kmem_cache_free(batadv_tt_change_cache, entry);
bat_priv->tt.local_changes--;
event_removed = true;goto unlock; }
/* track the change in the OGMinterval list */ list_add_tail(&tt_change_node->list, &bat_priv->tt.changes_list);
- bat_priv->tt.local_changes++;
Needs more complex constructs with WRITE_ONCE or __sync_add_and_fetch/__sync_sub_and_fetch (which were handled before inside atomic_inc). The latter are not used that often in the kernel, so I wouldn't want to introduce them in the batman-adv module.
@@ -1022,7 +1017,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) tt_data->flags = BATADV_TT_OGM_DIFF;
spin_lock_bh(&bat_priv->tt.changes_list_lock);
- atomic_set(&bat_priv->tt.local_changes, 0);
bat_priv->tt.local_changes = 0;
list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list, list) {
Would need WRITE_ONCE
@@ -1438,7 +1433,7 @@ static void batadv_tt_changes_list_free(struct batadv_priv *bat_priv) kmem_cache_free(batadv_tt_change_cache, entry); }
- atomic_set(&bat_priv->tt.local_changes, 0);
- bat_priv->tt.local_changes = 0; spin_unlock_bh(&bat_priv->tt.changes_list_lock);
}
Would need WRITE_ONCE
Kind regards, Sven
On Thu, Nov 21, 2024 at 10:04:15AM +0100, Sven Eckelmann wrote:
On Wednesday, 20 November 2024 18:47:17 CET Remi Pommarel wrote:
The tt.local_changes atomic is either written with tt.changes_list_lock or close to it (see batadv_tt_local_event()). Thus the performance gain using an atomic was limited (or because of atomic_read() impact even negative). Using atomic also comes with the need to be wary of potential negative tt.local_changes value.
Simplify the tt.local_changes usage by removing the atomic property and modifying it only with tt.changes_list_lock held.
The overall change assumes that the compiler never splits writes (store tearing) [1]. Of course, writes are protected against each other using locks. But the reader is no longer protected from partial writes. I haven't checked whether store fusing might be a problem.
Kind regards, Sven
[1] https://www.kernel.org/doc/html/latest/core-api/wrappers/memory-barriers.htm...
Good catch thanks.
[...]
@@ -783,13 +783,13 @@ static int batadv_softif_init_late(struct net_device *dev) atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE); atomic_set(&bat_priv->bcast_seqno, 1); atomic_set(&bat_priv->tt.vn, 0);
- atomic_set(&bat_priv->tt.local_changes, 0); atomic_set(&bat_priv->tt.ogm_append_cnt, 0);
#ifdef CONFIG_BATMAN_ADV_BLA atomic_set(&bat_priv->bla.num_requests, 0); #endif atomic_set(&bat_priv->tp_num, 0);
- bat_priv->tt.local_changes = 0;
Would need WRITE_ONCE (just to be consistent)
[...]
@@ -508,21 +507,17 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv, del: list_del(&entry->list); kmem_cache_free(batadv_tt_change_cache, entry);
bat_priv->tt.local_changes--;
event_removed = true;goto unlock; }
/* track the change in the OGMinterval list */ list_add_tail(&tt_change_node->list, &bat_priv->tt.changes_list);
- bat_priv->tt.local_changes++;
Needs more complex constructs with WRITE_ONCE or __sync_add_and_fetch/__sync_sub_and_fetch (which were handled before inside atomic_inc). The latter are not used that often in the kernel, so I wouldn't want to introduce them in the batman-adv module.
What about using something in the line: WRITE_ONCE(&bat_priv->tt.local_changes, READ_ONCE(&bat_priv->tt.local_changes) + 1);
@@ -1022,7 +1017,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) tt_data->flags = BATADV_TT_OGM_DIFF;
spin_lock_bh(&bat_priv->tt.changes_list_lock);
- atomic_set(&bat_priv->tt.local_changes, 0);
bat_priv->tt.local_changes = 0;
list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list, list) {
Would need WRITE_ONCE
@@ -1438,7 +1433,7 @@ static void batadv_tt_changes_list_free(struct batadv_priv *bat_priv) kmem_cache_free(batadv_tt_change_cache, entry); }
- atomic_set(&bat_priv->tt.local_changes, 0);
- bat_priv->tt.local_changes = 0; spin_unlock_bh(&bat_priv->tt.changes_list_lock);
}
Would need WRITE_ONCE
Yes.
Thanks for the review.
On Thursday, 21 November 2024 10:28:06 CET Remi Pommarel wrote:
Needs more complex constructs with WRITE_ONCE or __sync_add_and_fetch/__sync_sub_and_fetch (which were handled before inside atomic_inc). The latter are not used that often in the kernel, so I wouldn't want to introduce them in the batman-adv module.
What about using something in the line: WRITE_ONCE(&bat_priv->tt.local_changes, READ_ONCE(&bat_priv->tt.local_changes) + 1);
This is what I meant with "more complex construct" :)
Kind regards, Sven
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.
Signed-off-by: Remi Pommarel repk@triplefau.lt --- net/batman-adv/translation-table.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 39704af81169..4cc69a930b2c 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -477,7 +477,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); list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list, list) { @@ -497,17 +497,21 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv, 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! + /* 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; 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; }
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
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*/
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;
On Thursday, 21 November 2024 10:13:15 CET Remi Pommarel wrote:
Looks right to me, I would even simplify that more for readability with:
[...]
-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;
The "discard" is unused. But the rest looks good.
Kind regards, Sven
On Thursday, 21 November 2024 10:23:33 CET Sven Eckelmann wrote:
On Thursday, 21 November 2024 10:13:15 CET Remi Pommarel wrote:
Looks right to me, I would even simplify that more for readability with:
[...]
-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;
The "discard" is unused. But the rest looks good.
No, it doesn't. You've accidental removed "entry->change.flags = flags;". So it should look more like this:
--- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -484,34 +484,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 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) { + /* 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--; + } 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; - goto discard; }
- 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; }
Kind regards, Sven
On Thu, Nov 21, 2024 at 10:30:37AM +0100, Sven Eckelmann wrote:
On Thursday, 21 November 2024 10:23:33 CET Sven Eckelmann wrote:
On Thursday, 21 November 2024 10:13:15 CET Remi Pommarel wrote:
Looks right to me, I would even simplify that more for readability with:
[...]
-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;
The "discard" is unused. But the rest looks good.
No, it doesn't. You've accidental removed "entry->change.flags = flags;". So it should look more like this:
Yes sorry did that too quickly. I'll wait for Antonio point of view on double DEL before respinning that.
Thanks,
On Wednesday, 20 November 2024 18:47:13 CET Remi Pommarel wrote:
The first three patches are actual fixes.
[...]
V3:
- Fix commit message wording
- Update outdated comments
Antonio, can you please review the patches? It is important that we get the first three out early (to avoid merge conflicts when sending the second part). So maybe you can have at least a look at the first three.
Kind regards, Sven
On Wed, Nov 20, 2024 at 08:46:12PM +0100, Sven Eckelmann wrote:
On Wednesday, 20 November 2024 18:47:13 CET Remi Pommarel wrote:
The first three patches are actual fixes.
[...]
V3:
- Fix commit message wording
- Update outdated comments
Antonio, can you please review the patches? It is important that we get the first three out early (to avoid merge conflicts when sending the second part). So maybe you can have at least a look at the first three.
If it is more convenient for you, I sure can split the serie in two ?
Also just saw that I forgot to remove the "a" from "a creating" in the commit log, sorry about that. Do you need re-spin for that ?
Thanks,
On 20/11/2024 20:54, Remi Pommarel wrote:
On Wed, Nov 20, 2024 at 08:46:12PM +0100, Sven Eckelmann wrote:
On Wednesday, 20 November 2024 18:47:13 CET Remi Pommarel wrote:
The first three patches are actual fixes.
[...]
V3:
- Fix commit message wording
- Update outdated comments
Antonio, can you please review the patches? It is important that we get the first three out early (to avoid merge conflicts when sending the second part). So maybe you can have at least a look at the first three.
If it is more convenient for you, I sure can split the serie in two ?
No problem for me. I'll allocate some hours tomorrow morning. Thanks for the ping, Sven.
Regards,
Also just saw that I forgot to remove the "a" from "a creating" in the commit log, sorry about that. Do you need re-spin for that ?
Thanks,
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Remi Pommarel -
Sven Eckelmann