On Tue, Dec 20, 2022 at 03:22:32PM +0100, Toke Høiland-Jørgensen wrote:
Ondrej Zajicek <santiago@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@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."