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 <[email protected]>
> > ---
> > 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.
--
Remi
