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.