gdb(4): Implement support for NoAckMode
ClosedPublic
Actions

Authored by cem on Sep 22 2019, 10:11 PM.

Details

Reviewers

markj
emaste
jhb

Commits

rS353702: gdb(4): Implement support for NoAckMode

Summary

When the underlying debugport transport is reliable, GDB's additional
checksums and acknowledgements come at some mild CPU cost, and more
importantly, additional RTTs. NoAckMode eliminates the redundant RTTs and
allows us to skip checking RX checksums. The GDB packet framing does not
change, so checksums are still included as message trailers.

The gdb(4) stub in FreeBSD advertises support for the feature in response to
the client's 'qSupported' request IFF the current debugport has the
gdb_dbfeatures flag GDB_DBGP_FEAT_RELIABLE set. Currently, only netgdb(4)
supports this feature.

If the remote GDB client supports the feature and does not have it disabled
via a GDB configuration knob, it may instruct our gdb(4) stub to enter
NoAckMode. Unless and until it issues that command, we must continue to
transmit acks as usual (and for now, we continue to wait until we receive
them as well, even if we know the debugport is on a reliable transport).

In the kernel sources, the sense of the flag representing the state of the
feature is reversed from that of the GDB command. (I.e., it is
'gdb_ackmode', not 'gdb_noackmode.') This is to avoid confusing double-
negative conditions.

For reference, see:

Test Plan

DEBUG output from the Python NetGDB proxy:

Waiting for connection from GDB client on port 8000.
Use 'target remote <ip>:8000' from gdb to connect.
Connection from GDB client received.
...
2019-09-22 15:02:10,846 - DEBUG - Received tcp data: +$qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a
2019-09-22 15:02:10,846 - DEBUG - Sending packet + to ...:20026.
2019-09-22 15:02:10,847 - DEBUG - Sending packet $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a to ...:20026.
2019-09-22 15:02:10,848 - DEBUG - Received udp data packet: +
2019-09-22 15:02:10,849 - DEBUG - Received udp data packet: $PacketSize=1003;qXfer:threads:read+;QStartNoAckMode+#71
2019-09-22 15:02:10,955 - DEBUG - Received tcp data: +$vMustReplyEmpty#3a
2019-09-22 15:02:10,955 - DEBUG - Sending packet + to ...:20026.
2019-09-22 15:02:10,955 - DEBUG - Sending packet $vMustReplyEmpty#3a to ...:20026.
2019-09-22 15:02:10,957 - DEBUG - Received udp data packet: +
2019-09-22 15:02:10,958 - DEBUG - Received udp data packet: $#00
2019-09-22 15:02:11,063 - DEBUG - Received tcp data: +$QStartNoAckMode#b0
2019-09-22 15:02:11,063 - DEBUG - Sending packet + to ...:20026.
2019-09-22 15:02:11,063 - DEBUG - Sending packet $QStartNoAckMode#b0 to ...:20026.
2019-09-22 15:02:11,065 - DEBUG - Received udp data packet: +
2019-09-22 15:02:11,066 - DEBUG - Received udp data packet: $OK#9a
// Below is the final ACK ("+") GDB client sends, in response to NetGDB "OK", in response to GDB's "QStackNoAckMode."
2019-09-22 15:02:11,171 - DEBUG - Received tcp data: +$Hg0#df
2019-09-22 15:02:11,171 - DEBUG - Sending packet + to ...:20026.
2019-09-22 15:02:11,171 - DEBUG - Sending packet $Hg0#df to ...:20026.
// Below is first NetGDB client response without preceeding ACK
2019-09-22 15:02:11,172 - DEBUG - Received udp data packet: $OK#9a
// Followed by first GDB server command without preceeding ACK
2019-09-22 15:02:11,173 - DEBUG - Received tcp data: $qTStatus#49
// Followed by no more redundant ACKs.
...

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 26872
Build 25190: arc lint + arc unit

Event Timeline

cem created this revision.Sep 22 2019, 10:11 PM

Harbormaster completed remote builds in B26633: Diff 62446.Sep 22 2019, 10:11 PM

cem added a parent revision: D21568: Implement NetGDB(4).Sep 22 2019, 10:12 PM

Rebase on parent change. No functional change.

Harbormaster completed remote builds in B26636: Diff 62450.Sep 23 2019, 12:42 AM

emaste added a subscriber: jhb.Sep 27 2019, 1:02 PM

Strictly speaking the helpers are kind of redundant as all but one call are already under a 'if (gdb_ackmode)' check.

This revision is now accepted and ready to land.Sep 27 2019, 7:34 PM

In D21761#476464, @jhb wrote:

Strictly speaking the helpers are kind of redundant as all but one call are already under a 'if (gdb_ackmode)' check.

Yeah, that's very true. I added them before I had quantified how few places actually responded with acks. I would be happy to remove that change if you prefer.

markj accepted this revision.Sep 30 2019, 7:38 PM

markj added inline comments.

sys/gdb/gdb_main.c
773	Add braces to keep consistency?
780	Wrong style for multi-line comments.

Thanks!

sys/gdb/gdb_main.c
773	I didn't really add this line, `diff` just ... borrowed it instead of showing the identical `tx_empty`/`break` lines I added below, at l. 787-788. The logical diff starts at `case 'Q':` and ends at `break;`. (If anything, I'd remove the excess braces elsewhere in the compound statement. But since I only added the `Q` case in this diff I'd rather leave `q` alone.)
780	Will fix, not sure what I was thinking.