[PATCH v3] babel: Rework seqno request handling
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. So rework the seqno request tracking so that instead of storing the destination of a request, we just track whether it was a request that we forwarded on behalf of another node, or if it was a request we originated ourselves. Forwarded requests are not retransmitted, they are only used for duplicate suppression, and for triggering an update when satisfied. If we end up originating a request that we previously forwarded, we "upgrade" the old request and restart the retransmit counter. One complication with this is that requests sent in response to unfeasible updates (section 3.8.2.2 of the RFC) have to be sent as unicast to a particular peer. However, we don't really need to retransmit those as there's no starvation when sending such a request; so we just change such requests to be one-off unicast requests that are not subject to retransmission or duplicate suppression. This is the same behaviour as babeld has for such requests. We also weren't discarding requests with a hop count of zero, so let's add such a check (they are forbidden by the RFC). Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- v3: - Rework the dupliate suppression logic to avoid suppressing resends v2: - Get rid of the neighbour pointer in the request state tracking - Change the resend behaviour to only resend requests we originated ourselves. proto/babel/babel.c | 155 +++++++++++++++++++++++++++++--------------- proto/babel/babel.h | 3 +- 2 files changed, 102 insertions(+), 56 deletions(-) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index ecfd2763d737..325b30aa2f58 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -53,7 +53,7 @@ static void babel_expire_requests(struct babel_proto *p, struct babel_entry *e); static void babel_select_route(struct babel_proto *p, struct babel_entry *e, struct babel_route *mod); static inline void babel_announce_retraction(struct babel_proto *p, struct babel_entry *e); static void babel_send_route_request(struct babel_proto *p, struct babel_entry *e, struct babel_neighbor *n); -static void babel_send_seqno_request(struct babel_proto *p, struct babel_entry *e, struct babel_seqno_request *sr); +static void babel_send_seqno_request(struct babel_proto *p, struct babel_entry *e, struct babel_seqno_request *sr, struct babel_neighbor *tgt); static void babel_update_cost(struct babel_neighbor *n); static inline void babel_kick_timer(struct babel_proto *p); static inline void babel_iface_kick_timer(struct babel_iface *ifa); @@ -288,8 +288,31 @@ babel_expire_routes(struct babel_proto *p) babel_expire_routes_(p, &p->ip6_rtable); } -static inline int seqno_request_valid(struct babel_seqno_request *sr) -{ return !sr->nbr || sr->nbr->ifa; } +static struct babel_neighbor * +babel_find_seqno_request_tgt(struct babel_entry *e, struct babel_neighbor *skip) +{ + struct babel_route *r, *best_feasible = NULL, *best_any = NULL; + + WALK_LIST(r, e->routes) + { + if (r->neigh == skip) + continue; + + if (r->feasible && (!best_feasible || r->metric < best_feasible->metric)) + best_feasible = r; + + if (!best_any || r->metric < best_any->metric) + best_any = r; + } + + if (best_feasible) + return best_feasible->neigh; + + if (best_any) + return best_any->neigh; + + return NULL; +} /* * Add seqno request to the table of pending requests (RFC 6216 3.2.6) and send @@ -299,50 +322,85 @@ static inline int seqno_request_valid(struct babel_seqno_request *sr) static void babel_add_seqno_request(struct babel_proto *p, struct babel_entry *e, u64 router_id, u16 seqno, u8 hop_count, - struct babel_neighbor *nbr) + struct babel_neighbor *fwd_from) { + struct babel_neighbor *tgt = NULL; struct babel_seqno_request *sr; + if (fwd_from) + { + tgt = babel_find_seqno_request_tgt(e, fwd_from); + if (!tgt) + { + TRACE(D_PACKETS, "Couldn't find a neighbour to forward seqno request for %N router-id %lR seqno %d to", + e->n.addr, router_id, seqno); + return; + } + } + WALK_LIST(sr, e->requests) if (sr->router_id == router_id) { - /* Found matching or newer */ - if (ge_mod64k(sr->seqno, seqno) && seqno_request_valid(sr)) + /* + * To suppress duplicates, check if we already have a newer (higher seqno) + * outstanding request. If we do, suppress this request if the outstanding + * request is one we originated ourselves. If the outstanding request is + * forwarded, supress only if this request is also one we're forwarding + * *and* we're within the first expiry period of that request. The latter + * is to ensure we don't suppress retransmissions of the request from the + * same source. + */ + if (ge_mod64k(sr->seqno, seqno) && + (!sr->forwarded || (fwd_from && !sr->count))) return; - /* Found older */ rem_node(NODE sr); - if (sr->nbr) - rem_node(&sr->nbr_node); + /* Allow upgrading from forwarded to non-forwarded */ + if (!fwd_from) + sr->forwarded = 0; goto found; } /* No entries found */ sr = sl_allocz(p->seqno_slab); + sr->forwarded = !!fwd_from; found: sr->router_id = router_id; sr->seqno = seqno; - sr->hop_count = hop_count; + sr->hop_count = hop_count ?: BABEL_INITIAL_HOP_COUNT; sr->count = 0; - sr->expires = current_time() + BABEL_SEQNO_REQUEST_EXPIRY; - if (sr->nbr = nbr) - add_tail(&nbr->requests, &sr->nbr_node); + /* + * Set the initial expiry time of forwarded requests to half the usual expiry + * time to be reasonably sure we expire it before it is resent by the source. + */ + sr->expires = current_time() + (sr->forwarded ? + (BABEL_SEQNO_REQUEST_EXPIRY/2) : + BABEL_SEQNO_REQUEST_EXPIRY); add_tail(&e->requests, NODE sr); + babel_send_seqno_request(p, e, sr, tgt); +} + +static void +babel_generate_seqno_request(struct babel_proto *p, struct babel_entry *e, + u64 router_id, u16 seqno, struct babel_neighbor *tgt) +{ + struct babel_seqno_request req = { + .router_id = router_id, + .seqno = seqno, + .hop_count = BABEL_INITIAL_HOP_COUNT, + }; - babel_send_seqno_request(p, e, sr); + babel_send_seqno_request(p, e, &req, tgt); } static void babel_remove_seqno_request(struct babel_proto *p UNUSED, struct babel_seqno_request *sr) { - if (sr->nbr) - rem_node(&sr->nbr_node); - rem_node(NODE sr); sl_free(sr); } @@ -372,13 +430,6 @@ babel_expire_requests(struct babel_proto *p, struct babel_entry *e) WALK_LIST_DELSAFE(sr, srx, e->requests) { - /* Remove seqno requests sent to dead neighbors */ - if (!seqno_request_valid(sr)) - { - babel_remove_seqno_request(p, sr); - continue; - } - /* Handle expired requests - resend or remove */ if (sr->expires && sr->expires <= now_) { @@ -386,7 +437,10 @@ babel_expire_requests(struct babel_proto *p, struct babel_entry *e) { sr->count++; sr->expires += (BABEL_SEQNO_REQUEST_EXPIRY << sr->count); - babel_send_seqno_request(p, e, sr); + + /* Forwarded requests will be resent by their originator */ + if (!sr->forwarded) + babel_send_seqno_request(p, e, sr, NULL); } else { @@ -431,7 +485,6 @@ babel_get_neighbor(struct babel_iface *ifa, ip_addr addr) nbr->cost = BABEL_INFINITY; nbr->init_expiry = current_time() + BABEL_INITIAL_NEIGHBOR_TIMEOUT; init_list(&nbr->routes); - init_list(&nbr->requests); add_tail(&ifa->neigh_list, NODE nbr); return nbr; @@ -452,10 +505,6 @@ babel_flush_neighbor(struct babel_proto *p, struct babel_neighbor *nbr) babel_flush_route(p, r); } - struct babel_seqno_request *sr; - WALK_LIST_FIRST2(sr, nbr_node, nbr->requests) - babel_remove_seqno_request(p, sr); - nbr->ifa = NULL; rem_node(NODE nbr); mb_free(nbr); @@ -903,22 +952,23 @@ babel_send_wildcard_request(struct babel_iface *ifa) } static void -babel_send_seqno_request(struct babel_proto *p, struct babel_entry *e, struct babel_seqno_request *sr) +babel_send_seqno_request(struct babel_proto *p, struct babel_entry *e, + struct babel_seqno_request *sr, struct babel_neighbor *tgt) { union babel_msg msg = {}; msg.type = BABEL_TLV_SEQNO_REQUEST; - msg.seqno_request.hop_count = sr->hop_count ?: BABEL_INITIAL_HOP_COUNT; + msg.seqno_request.hop_count = sr->hop_count; msg.seqno_request.seqno = sr->seqno; msg.seqno_request.router_id = sr->router_id; net_copy(&msg.seqno_request.net, e->n.addr); - if (sr->nbr) + if (tgt) { TRACE(D_PACKETS, "Sending seqno request for %N router-id %lR seqno %d to %I on %s", - e->n.addr, sr->router_id, sr->seqno, sr->nbr->addr, sr->nbr->ifa->ifname); + e->n.addr, sr->router_id, sr->seqno, tgt->addr, tgt->ifa->ifname); - babel_send_unicast(&msg, sr->nbr->ifa, sr->nbr->addr); + babel_send_unicast(&msg, tgt->ifa, tgt->addr); } else { @@ -1285,10 +1335,13 @@ babel_handle_update(union babel_msg *m, struct babel_iface *ifa) metric = babel_compute_metric(nbr, msg->metric); best = e->selected; - /* RFC section 3.8.2.2 - Dealing with unfeasible updates */ + /* + * RFC section 3.8.2.2 - Dealing with unfeasible updates. Generate a one-off + * (not retransmitted) unicast seqno request to the originator of this update + */ if (!feasible && (metric != BABEL_INFINITY) && (!best || (r == best) || (metric < best->metric))) - babel_add_seqno_request(p, e, s->router_id, s->seqno + 1, 0, nbr); + babel_generate_seqno_request(p, e, s->router_id, s->seqno + 1, nbr); /* Special case - ignore unfeasible update to best route */ if (r == best && !feasible && (msg->router_id == r->router_id)) @@ -1361,6 +1414,13 @@ babel_handle_seqno_request(union babel_msg *m, struct babel_iface *ifa) /* RFC 6126 3.8.1.2 */ + if (!msg->hop_count) + { + TRACE(D_PACKETS, "Discarding seqno request for %N router-id %lR with hop count 0 from %I", + &msg->net, msg->router_id, msg->sender); + return; + } + TRACE(D_PACKETS, "Handling seqno request for %N router-id %lR seqno %d hop count %d", &msg->net, msg->router_id, msg->seqno, msg->hop_count); @@ -1389,26 +1449,13 @@ babel_handle_seqno_request(union babel_msg *m, struct babel_iface *ifa) { /* Not ours; forward if TTL allows it */ - /* Find best admissible route */ - struct babel_route *r, *best1 = NULL, *best2 = NULL; - WALK_LIST(r, e->routes) - if ((r->router_id == msg->router_id) && !ipa_equal(r->neigh->addr, msg->sender)) - { - /* Find best feasible route */ - if ((!best1 || r->metric < best1->metric) && r->feasible) - best1 = r; - - /* Find best not necessary feasible route */ - if (!best2 || r->metric < best2->metric) - best2 = r; - } + struct babel_neighbor *nbr; - /* If no route is found, do nothing */ - r = best1 ?: best2; - if (!r) + nbr = babel_find_neighbor(ifa, msg->sender); + if (!nbr) return; - babel_add_seqno_request(p, e, msg->router_id, msg->seqno, msg->hop_count-1, r->neigh); + babel_add_seqno_request(p, e, msg->router_id, msg->seqno, msg->hop_count-1, nbr); } } diff --git a/proto/babel/babel.h b/proto/babel/babel.h index 8b6da3c8ce91..3e6673e574fe 100644 --- a/proto/babel/babel.h +++ b/proto/babel/babel.h @@ -240,7 +240,6 @@ struct babel_neighbor { btime init_expiry; list routes; /* Routes this neighbour has sent us (struct babel_route) */ - list requests; /* Seqno requests bound to this neighbor */ }; struct babel_source { @@ -276,7 +275,7 @@ struct babel_seqno_request { u8 hop_count; u8 count; btime expires; - struct babel_neighbor *nbr; + u8 forwarded; }; struct babel_entry { -- 2.38.1
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)) ... - There is no need to check hop_count in babel_handle_seqno_request(), that is already done in babel_read_seqno_request(). - 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. -- 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."
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.
- 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
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."
Ondrej Zajicek <santiago@crfreenet.org> writes:
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.
Not if the update arrives between the time we expire the entry, and the time we receive the retransmit. Which can easily happen if there's a slight timing difference, or if we're talking to a different implementation which has a different value for the expiry timer. So to be reasonably sure we always do the right thing and trigger an update, we really do need to keep the entry alive for longer. At the same time, we can't just keep a long timeout, because then we'll suppress actual retransmits.
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'
It doesn't quite eliminate it: if sr->count is >0 when a second forwarded request arrives, the object will get recycled and sr->count will get reset to 0. So there will be a new (short) period of duplicate suppression each time a new retransmit is performed. This would be identical if we removed the entry entirely after the first timeout.
3) It makes more sense to have one interval for both duplicate detection and reply detection.
But they serve different purposes: The duplicate suppression deals with the case where lots of neighbours all lose a prefix at the same time and send identical seqno requests for it. To deal with this we want a *short* window of time where we suppress duplicates. Whereas a reply may take longer to get to us, so we need a longer record to trigger the update, as described above. Reusing the expiry counter mechanism is maybe not the clearest way of expressing this; if you prefer I can rework it to have an explicit separate timer for duplicate suppression? Something like (in babel_add_seqno_request()): if (sr->forwarded) { sr->expires = current_time() + BABEL_SEQNO_FORWARD_EXPIRY; /* longish - 10 S? */; sr->dup_suppress = current_time() + BABEL_SEQNO_DUPLICATE_SUPPRESS_TIME; /* 1 S */; } else { sr->expires = current_time() + BABEL_SEQNO_REQUEST_EXPIRY; /* current value */ } Then we can do what you suggest and just remove the entry the first time we hit the expiry time if sr->forwarded (and we'd change the check on sr->count to look at sr->dup_suppress in add_seqno_request() of course). WDYT? -Toke
On Tue, Dec 20, 2022 at 05:39:19PM +0100, Toke Høiland-Jørgensen wrote:
Ondrej Zajicek <santiago@crfreenet.org> writes:
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.
Not if the update arrives between the time we expire the entry, and the time we receive the retransmit. Which can easily happen if there's a slight timing difference, or if we're talking to a different implementation which has a different value for the expiry timer. So to be reasonably sure we always do the right thing and trigger an update, we really do need to keep the entry alive for longer. At the same time, we can't just keep a long timeout, because then we'll suppress actual retransmits.
OK, i agree that keeping satisfy-detection for longer than duplicate-suppression makes sense, to avoid issues with timing differences. (although there is IMHO no need to keep it mixed with re-sent counter and make it as long as full timeout of originating router)
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'
It doesn't quite eliminate it: if sr->count is >0 when a second forwarded request arrives, the object will get recycled and sr->count will get reset to 0. So there will be a new (short) period of duplicate suppression each time a new retransmit is performed. This would be identical if we removed the entry entirely after the first timeout.
You are right, i missed that.
3) It makes more sense to have one interval for both duplicate detection and reply detection.
But they serve different purposes: The duplicate suppression deals with the case where lots of neighbours all lose a prefix at the same time and send identical seqno requests for it. To deal with this we want a *short* window of time where we suppress duplicates. Whereas a reply may take longer to get to us, so we need a longer record to trigger the update, as described above.
Reusing the expiry counter mechanism is maybe not the clearest way of expressing this; if you prefer I can rework it to have an explicit separate timer for duplicate suppression?
Yes, that would be much better, and would clearly communicate the purpose.
Something like (in babel_add_seqno_request()):
if (sr->forwarded) { sr->expires = current_time() + BABEL_SEQNO_FORWARD_EXPIRY; /* longish - 10 S? */; sr->dup_suppress = current_time() + BABEL_SEQNO_DUPLICATE_SUPPRESS_TIME; /* 1 S */; } else { sr->expires = current_time() + BABEL_SEQNO_REQUEST_EXPIRY; /* current value */ }
Then we can do what you suggest and just remove the entry the first time we hit the expiry time if sr->forwarded (and we'd change the check on sr->count to look at sr->dup_suppress in add_seqno_request() of course).
WDYT?
That would be great. -- 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."
participants (2)
-
Ondrej Zajicek -
Toke Høiland-Jørgensen