Skip to content

Commit

Permalink
ames: minimum viable decrement underflow hotfix (#375)
Browse files Browse the repository at this point in the history
We were incrementing the queue size counter conditionally, only for
packets we want to forward. But for all packets, we were issuing a scry,
and the callback would unconditionally decrement the queue size counter,
leading to underflows. That, in turn, would result in vere thinking the
queue was full when it wasn't.

Here, we make the least invasive change that resolves that particular
bug. There may or may not still be other things wrong with the
logic/factoring around the packet queue and lane scries.

f9e8542 changed the factoring here, making normal packet sending and
forwarding take the same code path. ~~But also, even before that change,
we were always sending the head of the packet queue to whatever lane
scry result we got, which feels like it might not be right, _especially
given that we don't even put the packet in queue when not forwarding_.~~
(edit: i think that was an incorrect reading of the code, we pass a
pointer to the specific packet, not the whole queue.) I suspect we just
don't want to scry for the lane in the normal sending codepath. We
didn't do that before recent changes either. (edit: as currently
written, we keep packets we want to send in memory until the scry gets
back to us, without tracking them anywhere.) But I don't know the full
context of the recent changes, so I'm only solving the specific
symptom/bug here.

Since this bug prevents forwarding from working reliable under certain
circumstances, this should go out as a hotfix.
  • Loading branch information
joemfb authored May 10, 2023
2 parents 4bfddbf + 2fb1b0b commit 72f693b
Showing 1 changed file with 24 additions and 9 deletions.
33 changes: 24 additions & 9 deletions pkg/vere/io/ames.c
Original file line number Diff line number Diff line change
Expand Up @@ -1393,8 +1393,6 @@ _ames_lane_scry_cb(void* vod_p, u3_noun nun)
u3_ames* sam_u = pac_u->sam_u;
u3_weak las = u3r_at(7, nun);

sam_u->sat_u.foq_d--;

// if scry fails, remember we can't scry, and just inject the packet
//
if ( u3_none == las ) {
Expand Down Expand Up @@ -1425,6 +1423,19 @@ _ames_lane_scry_cb(void* vod_p, u3_noun nun)
u3z(nun);
}

/* _ames_lane_scry_forward_cb(): learn lanes to forward packet on
*/
static void
_ames_lane_scry_forward_cb(void *vod_p, u3_noun nun)
{
u3_panc *pan_u = vod_p;
u3_ames *sam_u = pan_u->pac_u->sam_u;

sam_u->sat_u.foq_d--;

_ames_lane_scry_cb(vod_p, nun);
}

/* _ames_try_send(): try to send a packet to a ship and its sponsors
*/
static void
Expand Down Expand Up @@ -1488,7 +1499,9 @@ _ames_try_send(u3_pact* pac_u, c3_o for_o)
pan_u->pac_u = pac_u;
pan_u->for_o = for_o;

// if forwarding, enqueue
u3_noun pax = _lane_scry_path(u3i_chubs(2, pac_u->pre_u.rec_d));

// if forwarding, enqueue the packet and scry for the lane
//
if ( c3y == for_o ) {
if ( 0 != sam_u->pan_u ) {
Expand All @@ -1497,14 +1510,16 @@ _ames_try_send(u3_pact* pac_u, c3_o for_o)
}
sam_u->pan_u = pan_u;
sam_u->sat_u.foq_d++;
}

// scry the lane out of ames
u3_pier_peek_last(sam_u->pir_u, u3_nul, c3__ax,
u3_nul, pax, pan_u, _ames_lane_scry_forward_cb);
}
// otherwise, just scry for the lane
//
u3_noun pax = _lane_scry_path(u3i_chubs(2, pac_u->pre_u.rec_d));

u3_pier_peek_last(sam_u->pir_u, u3_nul, c3__ax,
u3_nul, pax, pan_u, _ames_lane_scry_cb);
else {
u3_pier_peek_last(sam_u->pir_u, u3_nul, c3__ax,
u3_nul, pax, pan_u, _ames_lane_scry_cb);
}
}
}

Expand Down

0 comments on commit 72f693b

Please sign in to comment.