Ondrej Zajicek <[email protected]> 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. > - 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. -Toke
