-
Notifications
You must be signed in to change notification settings - Fork 45
MGS: Add usdt probes for receiving messages #799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
# `gateway-sp-comms` | ||
|
||
Communicate with SPs across the management network. | ||
|
||
## DTrace Probes | ||
|
||
This crate currently provides three DTrace probes: receiving raw packets, | ||
receiving responses to requests, receiving serial console messages. | ||
|
||
Raw packets: | ||
|
||
``` | ||
% sudo dtrace -n 'gateway_sp_comms*:::recv_packet { printf("%s, %s", copyinstr(arg0), copyinstr(arg1)); tracemem(copyin(arg2, arg3), 128, arg3); }' | ||
dtrace: description 'gateway_sp_comms*:::recv_packet ' matched 1 probe | ||
CPU ID FUNCTION:NAME | ||
6 2961 _ZN16gateway_sp_comms17management_switch9recv_task28_$u7b$$u7b$closure$u7d$$u7d$17h2bdb4a0aa5dcced5E:recv_packet {"ok":"127.0.0.1:23457"}, {"ok":1} | ||
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef | ||
0: 01 00 00 00 01 73 70 33 00 00 00 00 00 00 00 00 .....sp3........ | ||
10: 00 00 00 00 00 24 00 00 00 00 00 00 00 06 00 68 .....$.........h | ||
20: 65 6c 6c 6f 0a ello. | ||
``` | ||
|
||
Responses: | ||
|
||
``` | ||
% sudo dtrace -n 'gateway_sp_comms*:::recv_response { printf("%s, %u, %s", copyinstr(arg0), arg1, copyinstr(arg2)); }' | ||
dtrace: description 'gateway_sp_comms*:::recv_response ' matched 1 probe | ||
CPU ID FUNCTION:NAME | ||
5 2962 _ZN16gateway_sp_comms12recv_handler11RecvHandler15handle_response17h28a3ce4546c4e1bdE:recv_response {"ok":0}, 0, {"ok":{"Ok":{"IgnitionState":{"id":17,"flags":{"bits":3}}}}} | ||
2 2962 _ZN16gateway_sp_comms12recv_handler11RecvHandler15handle_response17h28a3ce4546c4e1bdE:recv_response {"ok":1}, 1, {"ok":{"Ok":{"SpState":{"serial_number":[0,0,0,0,1,1,1,1,2,2,2,2,3,3,3,3]}}}} | ||
``` | ||
|
||
Serial console messages: | ||
|
||
``` | ||
% sudo dtrace -n 'gateway_sp_comms*:::recv_serial_console { printf("%u, %s, %u", arg0, copyinstr(arg0), arg2); tracemem(copyin(arg3, arg4), 128, arg4); }' | ||
dtrace: description 'gateway_sp_comms*:::recv_serial_console ' matched 1 probe | ||
CPU ID FUNCTION:NAME | ||
2 2963 _ZN16gateway_sp_comms12recv_handler11RecvHandler21handle_serial_console17hba2bd6ac4422e295E:recv_serial_console 105553180037200, {"ok":1}, 42 | ||
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef | ||
0: 68 65 6c 6c 6f 21 0a hello!. | ||
``` | ||
|
||
TODO EXPAND |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,13 @@ pub enum SpType { | |
Power, | ||
} | ||
|
||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
// We derive `Serialize` to be able to send `SwitchPort`s to usdt probes, but | ||
// critically we do _not_ implement `Deserialize` - the only way to construct a | ||
// `SwitchPort` should be to receive one from a `ManagementSwitch`. | ||
#[derive( | ||
Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, | ||
)] | ||
#[serde(transparent)] | ||
pub(crate) struct SwitchPort(usize); | ||
|
||
impl SwitchPort { | ||
|
@@ -359,6 +365,13 @@ where | |
|
||
match sock.try_recv_from(&mut buf) { | ||
Ok((n, addr)) => { | ||
let buf = &buf[..n]; | ||
probes::recv_packet!(|| ( | ||
&addr, | ||
&port, | ||
buf.as_ptr() as usize as u64, | ||
buf.len() as u64 | ||
)); | ||
if Some(addr) != inner.sp_socket(port).map(|s| s.addr) { | ||
// TODO-security: we received a packet from an address that | ||
// doesn't match what we believe is the SP's address. for | ||
|
@@ -371,7 +384,7 @@ where | |
); | ||
} else { | ||
debug!(inner.log, "received {} bytes", n; "port" => ?port); | ||
callback(port, &buf[..n]); | ||
callback(port, buf); | ||
} | ||
} | ||
// spurious wakeup; no need to log, just continue | ||
|
@@ -582,3 +595,14 @@ mod tests { | |
} | ||
} | ||
} | ||
|
||
#[usdt::provider(provider = "gateway_sp_comms")] | ||
mod probes { | ||
fn recv_packet( | ||
_source: &SocketAddr, | ||
_port: &SwitchPort, | ||
_data: u64, // TODO actually a `*const u8`, but that isn't allowed by usdt | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right, I didn't see what DTrace actions you were using. What you're doing seems like the best approach. How valuable do you feel raw pointers would be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can definitely see raw pointers as being a nice type to accept. Would that be reasonable for us to add? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you mean a reasonable amount of work, then yes I think so. We currently accept the builtin D primitive types, so I think extending it to support pointers to those types as well would not be a big lift. Converting a pointer to an integer as is done here "works", though, and that will actually still be done under the hood. So I think it would only serve as documentation (which is valuable!). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I filed oxidecomputer/usdt#55; I'll merge this PR with the |
||
_len: u64, | ||
) { | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that feature(asm) was no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to be required still; taking it out gives
for me on macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we'll still need
feature(asm)
until we use bothusdt
v0.3.2 or later and a recent-enough toolchain. And to build on macOS, we'll need nightly forfeature(asm_sym)
until that's stabilized. That might be sometime this year, but I think we've learned not to hold our breath :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised we aren't using a more recent toolchain I guess..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall how we settled on a particular toolchain, only that we had to pick a nightly for
usdt
. I'd love to get us on stable, but of course I'm not in the process of pulling inopte
, which has its own reliance on nightly :)