[PATCH v3] babel: Rework seqno request handling

[Ondrej Zajicek]

On Tue, Dec 20, 2022 at 03:22:32PM +0100, Toke Høiland-Jørgensen wrote:
> Ondrej Zajicek <santiago at crfreenet.org> writes:
> 
> > On Sat, Dec 17, 2022 at 09:45:59PM +0100, Toke Høiland-Jørgensen via Bird-users wrote:
> >> The seqno request retransmission handling was tracking the destination that
> >> a forwarded request was being sent to and always retransmitting to that
> >> same destination. This is unnecessary because we only need to retransmit
> >> requests we originate ourselves, not those we forward on behalf of
> >> others; in fact retransmitting on behalf of others can lead to exponential
> >> multiplication of requests, which would be bad.
> >
> > Hi
> >
> > Thanks for investigating the issue and the patch. It took me some time to
> > process it, so answering now. I have some notes / suggestions:
> >
> >  - In babel_expire_requests() the check for sr->forwarded is inside
> >  sr->count check, so BABEL_SEQNO_REQUEST_RETRY is applied for forwarded
> >  info, which does not make sense, perhaps it should be like:
> >  if (!sr->forwarded && (sr->count < BABEL_SEQNO_REQUEST_RETRY)) ...
> 
> This was actually on purpose: we want to keep the request object around
> for longer, so we can use it to trigger an update when it's satisfied.
> So I figured it would be simpler to just keep the expiry logic without
> actually transmitting anything. There's a check in
> babel_add_seqno_request() which uses sr->count time-limit the duplicate
> check to one update interval (which is also shorter for forwarded
> requests).
> 
> I'll update the comment to explain this.

OK, i thought that forwarding would be supressed for retransmited seqno requests,
but now i see there is added condition in babel_add_seqno_request() for it:

 (!sr->forwarded || (fwd_from && !sr->count))


But i still think this modification is worse than keeping entries for the
short time (without accounting for replies), for these reasons:

1) For variant with the short time, if the entry in the forwarding router
expires, then originator re-send the request, forwarding router re-forward
and re-create the entry, so satisfying logic still detect the reply and
trigger an update.

2) This change effectively eliminates 'multiple forward check' once
sr->count > 0. It would make sense to have 'one forward per one sr->count
increase' limit. But then it is pretty much equivalent to the 'short time
variant'

3) It makes more sense to have one interval for both duplicate detection
and reply detection.


> >  - There is no need to check hop_count in babel_handle_seqno_request(),
> >  that is already done in babel_read_seqno_request().
> 
> Lol. I went looking for that and somehow missed it; will remove.
> 
> >  - Seems to me that as the focus of the patch changed completely from v1
> >  to v3, some changes thate were initially introduced now makes less sense.
> >  Namely i would keep meaning of the last argument of babel_add_seqno_request()
> >  to be the neighbor we forward the request to, and keep seqno request routing
> >  (i.e. babel_find_seqno_request_tgt()) outside. Either keep old code in
> >  babel_handle_seqno_request(), or call babel_find_seqno_request_tgt() from
> >  that. That makes more sense to me as it is part of section 3.8.1.2
> >  implemented there.
> 
> Right, can do; I was on the fence about whether I should move it back or
> not, but I agree it makes more sense that way.

Thanks.

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."