Conversation
mkeeter
force-pushed
the
mkeeter/hiffy-net
branch
7 times, most recently
from
d116c1e to
54b42af
Compare
|
Marking this a ready-for-review now that oxidecomputer/humility#616 is out |
mkeeter
force-pushed
the
mkeeter/hiffy-net
branch
from
54b42af to
b95a834
Compare
lzrd
left a comment
lzrd
left a comment
There was a problem hiding this comment.
I would fix the repeated literal constants.
It would be nice to see all of the loop timing and retry constants be together.
| sleeps = 0; | ||
| // Exponentially backoff our sleep value, but no more than 250ms | ||
| if sleeps == 10 { | ||
| sleep_ms = core::cmp::min(sleep_ms * 10, 250); |
There was a problem hiding this comment.
Same literal constant repeated. Make it symbolic.
There was a problem hiding this comment.
Can you suggest names for these constants? The first 10 is "number of iterations to run between updates to sleep_ms", the second 10 is "scale factor between updates"; neither of them have a good pithy name.
(This is also unchanged from the previous code, just indented)
| rstack, | ||
| &mut *HIFFY_SCRATCH.borrow_mut(), | ||
| check, | ||
| ); |
There was a problem hiding this comment.
We're in an intentional insecure mode. But, do we at least get an integrity check on the network transport? There isn't one at this layer. This is debug/YOLO mode, but a light check to scope a data corruption error would be good.
Maybe just a comment on data integrity for now.
There was a problem hiding this comment.
We use IPv6 UDP, which has a 16-bit checksum. This is true for all Hubris network activity, so calling out Hiffy as uniquely bad doesn't make sense to me.
mkeeter
force-pushed
the
mkeeter/hiffy-net
branch
2 times, most recently
from
1dc7422 to
9dfe770
Compare
| if notif.check_notification_mask(notifications::SOCKET_MASK) { | ||
| net_state.check_net(); | ||
| } | ||
|
|
||
| if notif.has_timer_fired(notifications::TIMER_MASK) { |
There was a problem hiding this comment.
It feels odd to potentially check the net task here and then also have to check the timer. It's probably fine and I think I'm having a strong reaction to this not being an idol server (hiffy probably pre-dates that?)
There was a problem hiding this comment.
Can you say more about why it feels odd? We're waiting on the combined net + timer notification bits, so it seems reasonable to check which one(s) fired and proceed accordingly.
I did consider making this an Idol server, but decided against it:
hiffydoesn't expose any public IPCs- We'd end up doing the same
notificationchecks in theimpl NotificationHandler, with more indirection / overhead from the Idol runtime
There was a problem hiding this comment.
I was initially concerned about TOCTOU with the net and timer aligning in weird ways but I don't think that's actually a concern on re-review
| // TODO without a safety comment explaining why these are safe, it is | ||
| // not clear if this is sound, do _not_ "fix" this by slapping on an | ||
| // addr_of_mut! without further analysis! | ||
| let text = unsafe { &HIFFY_TEXT }; | ||
| let data = unsafe { &HIFFY_DATA }; | ||
| let rstack = unsafe { &mut HIFFY_RSTACK[0..] }; |
There was a problem hiding this comment.
I don't think we're less unsound but do we need further discussion either her or in another comment about interactions between hiffy over probe and hiffy over the network? I can handwave about soundness okay from one external input source (probe) but two (net and probe) makes things more complicated. Is using both net hiffy and probe hiffy in the category of "don't do that"?
There was a problem hiding this comment.
How about we bring in #2475 first, then I update this PR to use those new primitives (e.g. lifetime-bounded slices)?
There was a problem hiding this comment.
yes I think that would make things easier to reason about
mkeeter
force-pushed
the
mkeeter/hiffy-net
branch
from
9dfe770 to
2a76ffe
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-net
branch
from
2a76ffe to
e262ed1
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-net
branch
from
e262ed1 to
bcb5058
Compare
3 participants
For too long, a network-attached Humility has only been able to run a subset of
hiffyprograms (ones that exclusively use theSendoperations, using theudprpchack). This PR adds anetfeature to thehiffytask, restoring it to full usefulness when network-attached.hiffyaccepts three messages:For simplicity, these messages only write to RAM. Even the
HiffyKickmessage doesn't actually start execution itself; it just sets theHIFFY_KICKvariable, which is checked the main loop. This mimics the behavior of the debugger.Interestingly, we don't need any new infrastructure to read the results; we can use the existing RAM-reading infrastructure.
Unfortunately, Github is butchering the diff here. The existing code (look for kick, run program) is unchanged but for indentation, because we have two notifications (timer and
netactivity). If the newnetnotification fires, then we handle the packet in a newnet::Stateobject.I'm opening this as a draft while I polish up the Humility side, but it's ready for review now.