Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [remote protocol] support for disabling packet acknowledgement
@ 2008-07-10 17:23 Pedro Alves
  2008-07-10 18:44 ` Paul Koning
                   ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Pedro Alves @ 2008-07-10 17:23 UTC (permalink / raw)
  To: gdb; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

Hi,

This patch adds support to the remote protocol to disable packet
acknowlegment.  This is useful when the transport being used is itself
reliable, e.g., TCP/IP.  In these cases, by removing the acking
we reduce the amount of roundtrips, and decrease the communication
latency.

While we were discussing implementing this, I noticed that Apple
already had something of the sort, so we ended up reusing Apple's
QStartNoAckMode packet name, and basing our implementation on
their version, modernizing it, adding qSupport support
to it, tweaking the user interface so it is integrated with
the "set remote xxx-packet" set of commands, and documenting it.

The docs changes below explain more in detail the approach taken.

I'm including in the patch GDB changes to support the feature,
and adding the support to gdbserver.  Regtesting against a native
gdbserver on x86_64-unknown-linux-gnu doesn't show any regression.

What do you think?

-- 
Pedro Alves

[-- Attachment #2: noack.diff --]
[-- Type: text/x-diff, Size: 18163 bytes --]

gdb/doc/
2008-07-10  Sandra Loosemore  <sandra@codesourcery.com>

	* gdb.texinfo (Remote Configuration): Document set remote noack-packet.
	(Remote Protocol): Add Packet Acknowledgement to menu.
	(Overview): Mention +/- can be disabled, and point to new section
	where this is discussed in detail.
	(General Query Packets): Document QStartNoAckMode packet, and
	corresponding qSupported reply.
	(Packet Acknowledgement): New section.

gdb/
2008-07-10  Pedro Alves  <pedro@codesourcery.com>

	Add no-ack mode to the remote protocol --- optionally stop ACKing
	packets and responses when we have a reliable communication
	medium.

	Based on Apple's GDB, by Jason Molenda <jmolenda@apple.com>

	* remote.c (struct remote_state): Add noack_mode field.
	(PACKET_QStartNoAckMode): New.
	(remote_start_remote): Don't any outstanding packet here.
	(remote_open_1): Clear noack_mode.  Ack any outstanding packet
	here.  Activate noack mode if requested.
	(remote_protocol_features): Add QStartNoAckMode.
	(remote_open_1):
	(putpkt_binary): Don't send ack in noack mode.
	(read_frame): Don't recompute the checksum in noack mode.
	(getpkt_sane): Skip sending ack if in noack mode.
	(_initialize_remote): Add set/show remote noack mode.

gdb/gdbserver/
2008-07-10  Pedro Alves  <pedro@codesourcery.com>

	* remote-utils.c (noack_mode, transport_is_reliable): New globals.
	(remote_open): Set or clear transport_is_reliable.
	(putpkt_binary): Don't expect acks in noack mode.
	(getpkt): Don't send ack/nac in noack mode.
	* server.c (handle_general_set): Handle QStartNoAckMode.
	(handle_query): If connected by tcp pass QStartNoAckMode+ in
	qSupported.
	(main): Reset noack_mode on every connection.
	* server.h (noack_mode): Declare.

---
 gdb/doc/gdb.texinfo          |   80 ++++++++++++++++++++++++++++++++++++++++++-
 gdb/gdbserver/remote-utils.c |   51 ++++++++++++++++++++++-----
 gdb/gdbserver/server.c       |   16 ++++++++
 gdb/gdbserver/server.h       |    2 +
 gdb/remote.c                 |   72 +++++++++++++++++++++++++++++++++++---
 5 files changed, 204 insertions(+), 17 deletions(-)

Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2008-07-07 19:02:34.000000000 +0100
+++ src/gdb/doc/gdb.texinfo	2008-07-10 17:38:44.000000000 +0100
@@ -13676,6 +13676,10 @@ are:
 @item @code{hostio-unlink-packet}
 @tab @code{vFile:unlink}
 @tab @code{remote delete}
+
+@item @code{noack-packet}
+@tab @code{QStartNoAckMode}
+@tab Packet acknowledgement
 @end multitable
 
 @node Remote Stub
@@ -23703,6 +23707,7 @@ Show the current setting of the target w
 * Tracepoint Packets::
 * Host I/O Packets::
 * Interrupts::
+* Packet Acknowledgement::
 * Examples::
 * File-I/O Remote Protocol Extension::
 * Library List Format::
@@ -23752,7 +23757,6 @@ That @var{sequence-id} was appended to t
 has never output @var{sequence-id}s.  Stubs that handle packets added
 since @value{GDBN} 5.0 must not accept @var{sequence-id}.
 
-@cindex acknowledgment, for @value{GDBN} remote
 When either the host or the target machine receives a packet, the first
 response expected is an acknowledgment: either @samp{+} (to indicate
 the package was received correctly) or @samp{-} (to request
@@ -23764,6 +23768,10 @@ retransmission):
 @end smallexample
 @noindent
 
+The @samp{+}/@samp{-} acknowledgements can be disabled
+once a connection is established.
+@xref{Packet Acknowledgement}, for details.
+
 The host (@value{GDBN}) sends @var{command}s, and the target (the
 debugging stub incorporated in your program) sends a @var{response}.  In
 the case of step and continue @var{command}s, the response is only sent
@@ -24818,6 +24826,23 @@ A badly formed request or an error was e
 An empty reply indicates that @samp{qSearch:memory} is not recognized.
 @end table
 
+@item QStartNoAckMode
+@cindex @samp{QStartNoAckMode} packet
+@anchor{QStartNoAckMode}
+Request that the remote stub disable the normal @samp{+}/@samp{-}
+protocol acknowledgments (@pxref{Packet Acknowledgement}).
+
+Reply:
+@table @samp
+@item OK
+The stub has switched to no-acknowledgement mode.
+@value{GDBN} acknowledges this reponse,
+but neither the stub nor @value{GDBN} shall send or expect further
+@samp{+}/@samp{-} acknowledgements in the current connection.
+@item
+An empty reply indicates that the stub does not support no-acknowledgment mode.
+@end table
+
 @item qSupported @r{[}:@var{gdbfeature} @r{[};@var{gdbfeature}@r{]}@dots{} @r{]}
 @cindex supported packets, remote query
 @cindex features of the remote protocol
@@ -24959,6 +24984,11 @@ These are the currently defined stub fea
 @tab @samp{-}
 @tab Yes
 
+@item @samp{QStartNoAckMode}
+@tab No
+@tab @samp{i}
+@tab Yes
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -25003,6 +25033,10 @@ The remote stub understands the @samp{qX
 The remote stub understands the @samp{QPassSignals} packet
 (@pxref{QPassSignals}).
 
+@item QStartNoAckMode
+The remote stub understands the @samp{QStartNoAckMode} packet and
+prefers to operate in no-acknowledgement mode.  @xref{Packet Acknowledgement}.
+
 @end table
 
 @item qSymbol::
@@ -25547,6 +25581,50 @@ Reply Packets (@pxref{Stop Reply Packets
 of successfully stopping the program.  Interrupts received while the
 program is stopped will be discarded.
 
+@node Packet Acknowledgement
+@section Packet Acknowledgement
+
+@cindex acknowledgment, for @value{GDBN} remote
+@cindex packet acknowledgment, for @value{GDBN} remote
+By default, when either the host or the target machine receives a packet,
+the first response expected is an acknowledgment: either @samp{+} (to indicate
+the package was received correctly) or @samp{-} (to request retransmission).
+This mechanism allows the @value{GDBN} remote protocol to operate over
+unreliable transport mechanisms, such as a serial line.
+
+In cases where the transport mechanism is itself reliable (such as a pipe or
+TCP connection), the @samp{+}/@samp{-} acknowledgements are redundant.
+It may be desirable to disable them in that case to reduce communication
+overhead, or for other reasons.  This can be accomplished by means of the
+@samp{QStartNoAckMode} packet; @pxref{QStartNoAckMode}.
+
+When in no-acknowledgment mode, neither the stub nor GDB shall send or
+expect @samp{+}/@samp{-} protocol acknowledgements.  The packet
+and response format still includes the normal checksum, as described in
+@ref{Overview}, but the checksum may be ignored by the receiver.
+
+If the stub supports @samp{QStartNoAckMode} and prefers to operate in
+no-acknowledgement mode, it should report that to @value{GDBN}
+by including @samp{QStartNoAckMode+} in its response to @samp{qSupported};
+@pxref{qSupported}.
+If @value{GDBN} also supports @samp{QStartNoAckMode} and it has not been
+disabled via the @code{set remote noack-packet off} command
+(@pxref{Remote Configuration}),
+@value{GDBN} may then send a @samp{QStartNoAckMode} packet to the stub.
+Only then may the stub actually turn off packet acknowledgements.
+@value{GDBN} sends a final @samp{+} acknowledgement of the stub's @samp{OK}
+response, which can be safely ignored by the stub.
+
+Note that @code{set remote noack-packet} command only affects negotiation
+between @value{GDBN} and the stub when subsequent connections are made;
+it does not affect the protocol acknowledgement state for any current
+connection.
+Since @samp{+}/@samp{-} acknowledgements are enabled by default when a
+new connection is established,
+there is also no protocol request to re-enable the acknowledgements
+for the current connection, once disabled.
+
+
 @node Examples
 @section Examples
 
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/remote.c	2008-07-10 17:39:29.000000000 +0100
@@ -274,6 +274,11 @@ struct remote_state
      skip calling getpkt.  This flag is set when BUF contains a
      stop reply packet and the target is not waiting.  */
   int cached_wait_status;
+
+  /* True, if in no ack mode.  That is, neither GDB nor the stub will
+     expect acks from each other.  The connection is assumed to be
+     reliable.  */
+  int noack_mode;
 };
 
 /* This data could be associated with a target, but we do not always
@@ -960,6 +965,7 @@ enum {
   PACKET_qSearch_memory,
   PACKET_vAttach,
   PACKET_vRun,
+  PACKET_QStartNoAckMode,
   PACKET_MAX
 };
 
@@ -2291,9 +2297,6 @@ remote_start_remote (struct ui_out *uiou
 
   immediate_quit++;		/* Allow user to interrupt it.  */
 
-  /* Ack any packet which the remote side has already sent.  */
-  serial_write (remote_desc, "+", 1);
-
   /* Check whether the target is running now.  */
   putpkt ("?");
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -2551,6 +2554,8 @@ static struct protocol_feature remote_pr
     PACKET_qXfer_spu_write },
   { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
     PACKET_QPassSignals },
+  { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
+    PACKET_QStartNoAckMode },
 };
 
 static void
@@ -2684,6 +2689,8 @@ static void
 remote_open_1 (char *name, int from_tty, struct target_ops *target, int extended_p)
 {
   struct remote_state *rs = get_remote_state ();
+  struct packet_config *noack_config;
+
   if (name == 0)
     error (_("To open a remote debug connection, you need to specify what\n"
 	   "serial device is attached to the remote system\n"
@@ -2765,6 +2772,7 @@ remote_open_1 (char *name, int from_tty,
      remote_query_supported or as they are needed.  */
   init_all_packet_configs ();
   rs->explicit_packet_size = 0;
+  rs->noack_mode = 0;
 
   general_thread = not_sent_ptid;
   continue_thread = not_sent_ptid;
@@ -2773,11 +2781,39 @@ remote_open_1 (char *name, int from_tty,
   use_threadinfo_query = 1;
   use_threadextra_query = 1;
 
+  /* Ack any packet which the remote side has already sent.  */
+  serial_write (remote_desc, "+", 1);
+
   /* The first packet we send to the target is the optional "supported
      packets" request.  If the target can answer this, it will tell us
      which later probes to skip.  */
   remote_query_supported ();
 
+  /* Next, we possibly activate noack mode.
+
+     If the QStartNoAckMode packet configuration is set to AUTO,
+     enable noack mode if the stub reported a wish for it with
+     qSupported.
+
+     If set to TRUE, then enable noack mode even if the stub didn't
+     report it in qSupported.  If the stub doesn't reply OK, the
+     session ends with an error.
+
+     If FALSE, then don't activate noack mode, regardless of what the
+     stub claimed should be the default with qSupported.  */
+
+  noack_config = &remote_protocol_packets[PACKET_QStartNoAckMode];
+
+  if (noack_config->detect == AUTO_BOOLEAN_TRUE
+      || (noack_config->detect == AUTO_BOOLEAN_AUTO
+	  && noack_config->support == PACKET_ENABLE))
+    {
+      putpkt ("QStartNoAckMode");
+      getpkt (&rs->buf, &rs->buf_size, 0);
+      if (packet_ok (rs->buf, noack_config) == PACKET_OK)
+	rs->noack_mode = 1;
+    }
+
   /* Next, if the target can specify a description, read it.  We do
      this before anything involving memory or registers.  */
   target_find_description ();
@@ -4790,6 +4826,11 @@ putpkt_binary (char *buf, int cnt)
       if (serial_write (remote_desc, buf2, p - buf2))
 	perror_with_name (_("putpkt: write failed"));
 
+      /* If this is a no acks version of the remote protocol, send the
+	 packet and move on.  */
+      if (rs->noack_mode)
+        break;
+
       /* Read until either a timeout occurs (-2) or '+' is read.  */
       while (1)
 	{
@@ -4866,6 +4907,7 @@ putpkt_binary (char *buf, int cnt)
 	}
 #endif
     }
+  return 0;
 }
 
 /* Come here after finding the start of a frame when we expected an
@@ -4921,6 +4963,7 @@ read_frame (char **buf_p,
   long bc;
   int c;
   char *buf = *buf_p;
+  struct remote_state *rs = get_remote_state ();
 
   csum = 0;
   bc = 0;
@@ -4966,6 +5009,12 @@ read_frame (char **buf_p,
 		return -1;
 	      }
 
+	    /* Don't recompute the checksum; with no ack packets we
+	       don't have any way to indicate a packet retransmission
+	       is necessary.  */
+	    if (rs->noack_mode)
+	      return bc;
+
 	    pktcsum = (fromhex (check_0) << 4) | fromhex (check_1);
 	    if (csum == pktcsum)
               return bc;
@@ -5124,20 +5173,28 @@ getpkt_sane (char **buf, long *sizeof_bu
 	      fputstrn_unfiltered (*buf, val, 0, gdb_stdlog);
 	      fprintf_unfiltered (gdb_stdlog, "\n");
 	    }
-	  serial_write (remote_desc, "+", 1);
+
+	  /* Skip the ack char if we're in no-ack mode.  */
+	  if (!rs->noack_mode)
+	    serial_write (remote_desc, "+", 1);
 	  return val;
 	}
 
       /* Try the whole thing again.  */
     retry:
-      serial_write (remote_desc, "-", 1);
+      /* Skip the nack char if we're in no-ack mode.  */
+      if (!rs->noack_mode)
+	serial_write (remote_desc, "-", 1);
     }
 
   /* We have tried hard enough, and just can't receive the packet.
      Give up.  */
 
   printf_unfiltered (_("Ignoring packet error, continuing...\n"));
-  serial_write (remote_desc, "+", 1);
+
+  /* Skip the ack char if we're in no-ack mode.  */
+  if (!rs->noack_mode)
+    serial_write (remote_desc, "+", 1);
   return -1;
 }
 \f
@@ -7532,6 +7589,9 @@ Show the maximum size of the address (in
   add_packet_config_cmd (&remote_protocol_packets[PACKET_vRun],
 			 "vRun", "run", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_QStartNoAckMode],
+			 "QStartNoAckMode", "noack", 0);
+
   /* Keep the old ``set remote Z-packet ...'' working.  Each individual
      Z sub-packet has its own set and show commands, but users may
      have sets to this variable in their .gdbinit files (or in their
Index: src/gdb/gdbserver/remote-utils.c
===================================================================
--- src.orig/gdb/gdbserver/remote-utils.c	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/gdbserver/remote-utils.c	2008-07-10 17:38:08.000000000 +0100
@@ -90,7 +90,7 @@ static struct sym_cache *symbol_cache;
    failures.  */
 int all_symbols_looked_up;
 
-int remote_debug = 0;
+int remote_debug = 1;
 struct ui_file *gdb_stdlog;
 
 static int remote_desc = INVALID_DESCRIPTOR;
@@ -99,6 +99,11 @@ static int remote_desc = INVALID_DESCRIP
 extern int using_threads;
 extern int debug_threads;
 
+/* If true, then GDB has requested noack mode.  */
+int noack_mode = 0;
+/* If true, then we tell GDB to use noack mode by default.  */
+int transport_is_reliable = 0;
+
 #ifdef USE_WIN32API
 # define read(fd, buf, len) recv (fd, (char *) buf, len, 0)
 # define write(fd, buf, len) send (fd, (char *) buf, len, 0)
@@ -181,6 +186,8 @@ remote_open (char *name)
 
       fprintf (stderr, "Remote debugging using %s\n", name);
 #endif /* USE_WIN32API */
+
+      transport_is_reliable = 0;
     }
   else
     {
@@ -267,6 +274,8 @@ remote_open (char *name)
       /* Convert IP address to string.  */
       fprintf (stderr, "Remote debugging from host %s\n", 
          inet_ntoa (sockaddr.sin_addr));
+
+      transport_is_reliable = 1;
     }
 
 #if defined(F_SETFL) && defined (FASYNC)
@@ -551,6 +560,17 @@ putpkt_binary (char *buf, int cnt)
 	  return -1;
 	}
 
+      if (noack_mode)
+	{
+	  /* Don't expect an ack then.  */
+	  if (remote_debug)
+	    {
+	      fprintf (stderr, "putpkt (\"%s\"); [noack mode]\n", buf2);
+	      fflush (stderr);
+	    }
+	  break;
+	}
+
       if (remote_debug)
 	{
 	  fprintf (stderr, "putpkt (\"%s\"); [looking for ack]\n", buf2);
@@ -774,23 +794,34 @@ getpkt (char *buf)
       if (csum == (c1 << 4) + c2)
 	break;
 
+      if (noack_mode)
+	{
+	  fprintf (stderr, "Bad checksum, sentsum=0x%x, csum=0x%x, buf=%s [no-ack-mode, Bad medium?]\n",
+		   (c1 << 4) + c2, csum, buf);
+	  /* Not much we can do, GDB wasn't expecting an ack/nac.  */
+	  break;
+	}
+
       fprintf (stderr, "Bad checksum, sentsum=0x%x, csum=0x%x, buf=%s\n",
 	       (c1 << 4) + c2, csum, buf);
       write (remote_desc, "-", 1);
     }
 
-  if (remote_debug)
+  if (!noack_mode)
     {
-      fprintf (stderr, "getpkt (\"%s\");  [sending ack] \n", buf);
-      fflush (stderr);
-    }
+      if (remote_debug)
+	{
+	  fprintf (stderr, "getpkt (\"%s\");  [sending ack] \n", buf);
+	  fflush (stderr);
+	}
 
-  write (remote_desc, "+", 1);
+      write (remote_desc, "+", 1);
 
-  if (remote_debug)
-    {
-      fprintf (stderr, "[sent ack]\n");
-      fflush (stderr);
+      if (remote_debug)
+	{
+	  fprintf (stderr, "[sent ack]\n");
+	  fflush (stderr);
+	}
     }
 
   return bp - buf;
Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/gdbserver/server.c	2008-07-10 17:38:08.000000000 +0100
@@ -267,6 +267,19 @@ handle_general_set (char *own_buf)
       return;
     }
 
+  if (strcmp (own_buf, "QStartNoAckMode") == 0)
+    {
+      if (remote_debug)
+	{
+	  fprintf (stderr, "[noack mode enabled]\n");
+	  fflush (stderr);
+	}
+
+      noack_mode = 1;
+      write_ok (own_buf);
+      return;
+    }
+
   /* Otherwise we didn't know what packet it was.  Say we didn't
      understand it.  */
   own_buf[0] = 0;
@@ -774,6 +787,8 @@ handle_query (char *own_buf, int packet_
 	 qXfer:feature:read at all, we will never be re-queried.  */
       strcat (own_buf, ";qXfer:features:read+");
 
+      if (transport_is_reliable)
+	strcat (own_buf, ";QStartNoAckMode+");
       return;
     }
 
@@ -1444,6 +1459,7 @@ main (int argc, char *argv[])
 
   while (1)
     {
+      noack_mode = 0;
       remote_open (port);
 
     restart:
Index: src/gdb/gdbserver/server.h
===================================================================
--- src.orig/gdb/gdbserver/server.h	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/gdbserver/server.h	2008-07-10 17:38:08.000000000 +0100
@@ -171,6 +171,8 @@ extern void hostio_last_error_from_errno
 
 extern int remote_debug;
 extern int all_symbols_looked_up;
+extern int noack_mode;
+extern int transport_is_reliable;
 
 int putpkt (char *buf);
 int putpkt_binary (char *buf, int len);

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 17:23 [remote protocol] support for disabling packet acknowledgement Pedro Alves
@ 2008-07-10 18:44 ` Paul Koning
  2008-07-10 19:08   ` Daniel Jacobowitz
  2008-07-25 13:39 ` Eli Zaretskii
  2008-08-11 19:40 ` Daniel Jacobowitz
  2 siblings, 1 reply; 25+ messages in thread
From: Paul Koning @ 2008-07-10 18:44 UTC (permalink / raw)
  To: pedro; +Cc: gdb, gdb-patches

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

 Pedro> Hi, This patch adds support to the remote protocol to disable
 Pedro> packet acknowlegment.  This is useful when the transport being
 Pedro> used is itself reliable, e.g., TCP/IP.  In these cases, by
 Pedro> removing the acking we reduce the amount of roundtrips, and
 Pedro> decrease the communication latency.

I'm not sure this is a good idea.

For one thing, if you want to work on performance, there are much more
dramatic changes to the protocol that could be done that would help
much more.  I can't believe that the cost of acks is significant
compared to all the other bottlenecks.

Also, TCP is reliable delivery at the transport layer.  It doesn't do
reliable delivery at the application layer -- that's what the gdb
remote protocol ACKs do.  The fact that TCP delivered a packet to the
stub doesn't mean the stub acted on it.

     paul


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 18:44 ` Paul Koning
@ 2008-07-10 19:08   ` Daniel Jacobowitz
  0 siblings, 0 replies; 25+ messages in thread
From: Daniel Jacobowitz @ 2008-07-10 19:08 UTC (permalink / raw)
  To: Paul Koning; +Cc: pedro, gdb, gdb-patches

On Thu, Jul 10, 2008 at 02:44:20PM -0400, Paul Koning wrote:
> For one thing, if you want to work on performance, there are much more
> dramatic changes to the protocol that could be done that would help
> much more.  I can't believe that the cost of acks is significant
> compared to all the other bottlenecks.

In addition to what Sandra said, the ack is generally in a separate
TCP packet from the next response or request.  That means there's
double the number of TCP packets.  Especially over high latency links,
I think this will be a big help.  I've had plenty of experience of
cross-country remote protocol sessions and I like anything that helps.

-- 
Daniel Jacobowitz
CodeSourcery


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 17:23 [remote protocol] support for disabling packet acknowledgement Pedro Alves
  2008-07-10 18:44 ` Paul Koning
@ 2008-07-25 13:39 ` Eli Zaretskii
  2008-08-11 19:40 ` Daniel Jacobowitz
  2 siblings, 0 replies; 25+ messages in thread
From: Eli Zaretskii @ 2008-07-25 13:39 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Thu, 10 Jul 2008 18:23:13 +0100
> Cc: gdb-patches@sourceware.org
> 
> 2008-07-10  Sandra Loosemore  <sandra@codesourcery.com>
> 
> 	* gdb.texinfo (Remote Configuration): Document set remote noack-packet.
> 	(Remote Protocol): Add Packet Acknowledgement to menu.
> 	(Overview): Mention +/- can be disabled, and point to new section
> 	where this is discussed in detail.
> 	(General Query Packets): Document QStartNoAckMode packet, and
> 	corresponding qSupported reply.
> 	(Packet Acknowledgement): New section.

This part is okay, provided that you take care of the following:

> +@tab Packet acknowledgement

The correct spelling for the last word is "acknowledgment".  Please
fix that throughout the added text.

> +When in no-acknowledgment mode, neither the stub nor GDB shall send or
                                                        ^^^
@value{GDBN}


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 17:23 [remote protocol] support for disabling packet acknowledgement Pedro Alves
  2008-07-10 18:44 ` Paul Koning
  2008-07-25 13:39 ` Eli Zaretskii
@ 2008-08-11 19:40 ` Daniel Jacobowitz
  2 siblings, 0 replies; 25+ messages in thread
From: Daniel Jacobowitz @ 2008-08-11 19:40 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Thu, Jul 10, 2008 at 06:23:13PM +0100, Pedro Alves wrote:
> Hi,
> 
> This patch adds support to the remote protocol to disable packet
> acknowlegment.  This is useful when the transport being used is itself
> reliable, e.g., TCP/IP.  In these cases, by removing the acking
> we reduce the amount of roundtrips, and decrease the communication
> latency.
> 
> While we were discussing implementing this, I noticed that Apple
> already had something of the sort, so we ended up reusing Apple's
> QStartNoAckMode packet name, and basing our implementation on
> their version, modernizing it, adding qSupport support
> to it, tweaking the user interface so it is integrated with
> the "set remote xxx-packet" set of commands, and documenting it.
> 
> The docs changes below explain more in detail the approach taken.
> 
> I'm including in the patch GDB changes to support the feature,
> and adding the support to gdbserver.  Regtesting against a native
> gdbserver on x86_64-unknown-linux-gnu doesn't show any regression.

This patch is OK, with the two texinfo corrections from Eli, and
without this:

> Index: src/gdb/gdbserver/remote-utils.c
> ===================================================================
> --- src.orig/gdb/gdbserver/remote-utils.c	2008-07-10 17:37:11.000000000 +0100
> +++ src/gdb/gdbserver/remote-utils.c	2008-07-10 17:38:08.000000000 +0100
> @@ -90,7 +90,7 @@ static struct sym_cache *symbol_cache;
>     failures.  */
>  int all_symbols_looked_up;
>  
> -int remote_debug = 0;
> +int remote_debug = 1;
>  struct ui_file *gdb_stdlog;
>  
>  static int remote_desc = INVALID_DESCRIPTOR;

Also, could you please add a NEWS entry (there's a new packet and new
associated commands).

-- 
Daniel Jacobowitz
CodeSourcery


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-25 14:14   ` Sandra Loosemore
@ 2008-07-26  5:54     ` Eli Zaretskii
  0 siblings, 0 replies; 25+ messages in thread
From: Eli Zaretskii @ 2008-07-26  5:54 UTC (permalink / raw)
  To: Sandra Loosemore; +Cc: Paul_Koning, gdb, gdb-patches, pedro

> Date: Fri, 25 Jul 2008 10:12:38 -0400
> From: Sandra Loosemore <sandra@codesourcery.com>
> CC:  Paul_Koning@dell.com,  gdb@sourceware.org,   gdb-patches@sourceware.org,  pedro@codesourcery.com
> 
> >> You'll note the documentation says turning off acks may be desirable to reduce 
> >> communication overhead *or* "for other reasons".  In fact, it is the "other 
> >> reasons" that motivated this patch.  We are working on designing the extensions 
> >> to the remote protocol to support nonstop mode, and we realized that we simply 
> >> cannot do it in combination with using +/- acks on the asynchronous responses.
> > 
> > Then please just say so in the docs.
> 
> As you'll note from subsequent discussion, we decided to use another mechanism 
> for non-stop mode, so it has no dependence on the noack mode patch any more. 
> I'm not sure what else you think the docs for noack mode should say?

Perhaps nothing, now that the decision was to abandon the original
approach.  But the principle remains: if there's some _real_ reason to
including a feature in GDB, let's state that reason in the docs,
instead of hiding it in some "etc."

> Incidentally, I am working on docs for non-stop mode now -- both the user-level 
> changes, and the remote protocol pieces.

Thanks!


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-25 13:41 ` Eli Zaretskii
@ 2008-07-25 14:14   ` Sandra Loosemore
  2008-07-26  5:54     ` Eli Zaretskii
  0 siblings, 1 reply; 25+ messages in thread
From: Sandra Loosemore @ 2008-07-25 14:14 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Paul_Koning, gdb, gdb-patches, pedro

Eli Zaretskii wrote:
>> Date: Thu, 10 Jul 2008 14:57:14 -0400
>> From: Sandra Loosemore <sandra@codesourcery.com>
>> CC:  gdb@sourceware.org,  gdb-patches@sourceware.org,   Pedro Alves <pedro@codesourcery.com>
>>
>> Paul Koning wrote:
>>
>>> I'm not sure this is a good idea.
>>>
>>> For one thing, if you want to work on performance, there are much more
>>> dramatic changes to the protocol that could be done that would help
>>> much more.  I can't believe that the cost of acks is significant
>>> compared to all the other bottlenecks.
>> You'll note the documentation says turning off acks may be desirable to reduce 
>> communication overhead *or* "for other reasons".  In fact, it is the "other 
>> reasons" that motivated this patch.  We are working on designing the extensions 
>> to the remote protocol to support nonstop mode, and we realized that we simply 
>> cannot do it in combination with using +/- acks on the asynchronous responses.
> 
> Then please just say so in the docs.

As you'll note from subsequent discussion, we decided to use another mechanism 
for non-stop mode, so it has no dependence on the noack mode patch any more. 
I'm not sure what else you think the docs for noack mode should say?

Incidentally, I am working on docs for non-stop mode now -- both the user-level 
changes, and the remote protocol pieces.

-Sandra


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 18:59 Sandra Loosemore
  2008-07-10 19:09 ` Paul Koning
@ 2008-07-25 13:41 ` Eli Zaretskii
  2008-07-25 14:14   ` Sandra Loosemore
  1 sibling, 1 reply; 25+ messages in thread
From: Eli Zaretskii @ 2008-07-25 13:41 UTC (permalink / raw)
  To: Sandra Loosemore; +Cc: Paul_Koning, gdb, gdb-patches, pedro

> Date: Thu, 10 Jul 2008 14:57:14 -0400
> From: Sandra Loosemore <sandra@codesourcery.com>
> CC:  gdb@sourceware.org,  gdb-patches@sourceware.org,   Pedro Alves <pedro@codesourcery.com>
> 
> Paul Koning wrote:
> 
> > I'm not sure this is a good idea.
> > 
> > For one thing, if you want to work on performance, there are much more
> > dramatic changes to the protocol that could be done that would help
> > much more.  I can't believe that the cost of acks is significant
> > compared to all the other bottlenecks.
> 
> You'll note the documentation says turning off acks may be desirable to reduce 
> communication overhead *or* "for other reasons".  In fact, it is the "other 
> reasons" that motivated this patch.  We are working on designing the extensions 
> to the remote protocol to support nonstop mode, and we realized that we simply 
> cannot do it in combination with using +/- acks on the asynchronous responses.

Then please just say so in the docs.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11 18:54               ` Paul Koning
@ 2008-07-11 19:10                 ` Pedro Alves
  0 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2008-07-11 19:10 UTC (permalink / raw)
  To: Paul Koning; +Cc: gdb, drow, sandra, gdb-patches

A Friday 11 July 2008 19:54:09, Paul Koning wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

>  Pedro> Not weird at all, and it is safe.  It doesn't matter what you
>  Pedro> have in the middle as long as both ends have tcp.
>
> You're probably thinking about end to end TCP over a datagram cloud.
> That works, of course, that's the Internet.  I was talking about TCP
> from A to B, raw UART B to C, TCP from C to D.  TCP wouldn't be
> helping you detect or correct data loss on the B to C path, and that
> means you'd need application layer acks (as in the current remote GDB
> protocol) for that case.  But that's a topology that makes no sense to
> me and I wouldn't expect ever to see.

Right, I was assuming tcp connection A <-> D, not two separate
tcp connections.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11 17:59             ` Pedro Alves
@ 2008-07-11 18:54               ` Paul Koning
  2008-07-11 19:10                 ` Pedro Alves
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Koning @ 2008-07-11 18:54 UTC (permalink / raw)
  To: pedro; +Cc: gdb, drow, sandra, gdb-patches

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

 Pedro> A Friday 11 July 2008 16:53:46, Paul Koning wrote:
 >> >>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:
 Daniel> On Fri, Jul 11, 2008 at 04:10:47PM +0100, Pedro Alves wrote:
 >> Agreed.  Telnet to a terminal server that feeds a UART based
 >> target stub is common practice.

 Pedro> Ack, we're all in sync.

 >> TCP at both ends with datagrams in between is too weird to
 >> consider.

 Pedro> Not weird at all, and it is safe.  It doesn't matter what you
 Pedro> have in the middle as long as both ends have tcp.

You're probably thinking about end to end TCP over a datagram cloud.
That works, of course, that's the Internet.  I was talking about TCP
from A to B, raw UART B to C, TCP from C to D.  TCP wouldn't be
helping you detect or correct data loss on the B to C path, and that
means you'd need application layer acks (as in the current remote GDB
protocol) for that case.  But that's a topology that makes no sense to
me and I wouldn't expect ever to see.

   paul


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11 15:54           ` Paul Koning
@ 2008-07-11 17:59             ` Pedro Alves
  2008-07-11 18:54               ` Paul Koning
  0 siblings, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2008-07-11 17:59 UTC (permalink / raw)
  To: gdb; +Cc: Paul Koning, drow, sandra, gdb-patches

A Friday 11 July 2008 16:53:46, Paul Koning wrote:
> >>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:
>  Daniel> On Fri, Jul 11, 2008 at 04:10:47PM +0100, Pedro Alves wrote:
> Agreed.  Telnet to a terminal server that feeds a UART based target
> stub is common practice.  

Ack, we're all in sync.

> TCP at both ends with datagrams in between is too weird to consider.

Not weird at all, and it is safe.  It doesn't matter what you have
in the middle as long as both ends have tcp.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11 15:24         ` Daniel Jacobowitz
@ 2008-07-11 15:54           ` Paul Koning
  2008-07-11 17:59             ` Pedro Alves
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Koning @ 2008-07-11 15:54 UTC (permalink / raw)
  To: drow; +Cc: pedro, sandra, gdb, gdb-patches

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

 Daniel> On Fri, Jul 11, 2008 at 04:10:47PM +0100, Pedro Alves wrote:
 >> - GDB also knows it is using a reliable connection (tcp/pipe)
 >> 
 >> That last bit was there until the last version of the patch before
 >> submission, but I ended up removing it from the final patch.  We
 >> can always go with or without that bit for now, and change it
 >> later, as the difference is all on GDB's side.

 Daniel> IMO this isn't necessary - if someone tunnels a reliable
 Daniel> TCP/IP connection from their stub, to gdb, via an unreliable
 Daniel> channel, then they can read the manual and disable this
 Daniel> feature.  I've never encountered this.  At that point you're
 Daniel> not running directly off a target board's UART at system
 Daniel> level, so you have the option of something like PPP.

Agreed.  Telnet to a terminal server that feeds a UART based target
stub is common practice.  TCP at both ends with datagrams in between
is too weird to consider.

   paul


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11 15:11       ` Pedro Alves
@ 2008-07-11 15:24         ` Daniel Jacobowitz
  2008-07-11 15:54           ` Paul Koning
  0 siblings, 1 reply; 25+ messages in thread
From: Daniel Jacobowitz @ 2008-07-11 15:24 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Paul Koning, sandra, gdb, gdb-patches

On Fri, Jul 11, 2008 at 04:10:47PM +0100, Pedro Alves wrote:
>   - GDB also knows it is using a reliable connection (tcp/pipe)
> 
> That last bit was there until the last version of the patch
> before submission, but I ended up removing it from the final
> patch.  We can always go with or without that bit for now, and
> change it later, as the difference is all on GDB's side.

IMO this isn't necessary - if someone tunnels a reliable TCP/IP
connection from their stub, to gdb, via an unreliable channel, then
they can read the manual and disable this feature.  I've never
encountered this.  At that point you're not running directly off a
target board's UART at system level, so you have the option of
something like PPP.

-- 
Daniel Jacobowitz
CodeSourcery


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 20:13     ` Paul Koning
  2008-07-10 22:33       ` Daniel Jacobowitz
       [not found]       ` <20080710223312.GA19058__14539.8706700236$1215729298$gmane$org@caradoc.them.org>
@ 2008-07-11 15:11       ` Pedro Alves
  2008-07-11 15:24         ` Daniel Jacobowitz
  2 siblings, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2008-07-11 15:11 UTC (permalink / raw)
  To: Paul Koning; +Cc: sandra, gdb, gdb-patches

[so, comming back to the original functionality]

On Thursday 10 July 2008 21:13:20, Paul Koning wrote:

> By the way, note that "target remote host:port" does NOT mean that you
> have TCP end to end.  You may have TCP to a terminal server and raw
> UART the rest of the way, so you still need ACKs (of one kind or
> another).

Right, that's why we made it so that only if the stub reports
qSupported:QStartNoAck+, GDB will turn it on by default.  If you
notice the gdbserver patch, it will only report that, if the
connection is coming from TCP.  It is my understanding that
situations like these are not usually seen:

GDB UART <-> something <-> TCP/IP stub

Would it make people happier if GDB would only enable noack mode
*automatically* if:

  - The "set remote noack-packet" setting is set to auto

  - The stub reported support and *preference* to it with qSupported
    (it knows it's using a reliable connection on its end)

  - GDB also knows it is using a reliable connection (tcp/pipe)

That last bit was there until the last version of the patch
before submission, but I ended up removing it from the final
patch.  We can always go with or without that bit for now, and
change it later, as the difference is all on GDB's side.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11 13:43         ` Paul Koning
@ 2008-07-11 14:35           ` Daniel Jacobowitz
  0 siblings, 0 replies; 25+ messages in thread
From: Daniel Jacobowitz @ 2008-07-11 14:35 UTC (permalink / raw)
  To: Paul Koning; +Cc: sandra, gdb, gdb-patches, pedro

On Fri, Jul 11, 2008 at 09:42:37AM -0400, Paul Koning wrote:
> Fair enough.   Ok, thanks.

Thank you, too - I'm glad you got us thinking about this from a
different perspective and we may have something that will satisfy
unreliable links, now.

-- 
Daniel Jacobowitz
CodeSourcery


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 22:33       ` Daniel Jacobowitz
  2008-07-11  0:22         ` Sandra Loosemore
@ 2008-07-11 13:43         ` Paul Koning
  2008-07-11 14:35           ` Daniel Jacobowitz
  1 sibling, 1 reply; 25+ messages in thread
From: Paul Koning @ 2008-07-11 13:43 UTC (permalink / raw)
  To: drow; +Cc: sandra, gdb, gdb-patches, pedro

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

 Daniel> Sandra asked me to take a stab at explaining the mess we're
 Daniel> in. ...

 Daniel> I think that if someone wants to design a more reliable
 Daniel> protocol than the existing one, they are free to do so, and
 Daniel> either layer it under the existing protocol as described
 Daniel> above or contribute it to GDB - we're not leaving anyone out
 Daniel> in the cold and a new feature doesn't have to meet every
 Daniel> possible use case in its first incarnation.

Fair enough.   Ok, thanks.

 Daniel> This isn't the only problem with the existing protocol in my
 Daniel> opinion.  It's pretty crufty, but it gets by.

I'd agree with that.

    paul


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11  3:05         ` Frank Ch. Eigler
@ 2008-07-11  3:25           ` Daniel Jacobowitz
  0 siblings, 0 replies; 25+ messages in thread
From: Daniel Jacobowitz @ 2008-07-11  3:25 UTC (permalink / raw)
  To: Frank Ch. Eigler; +Cc: Paul Koning, sandra, gdb, gdb-patches, pedro

On Thu, Jul 10, 2008 at 10:59:56PM -0400, Frank Ch. Eigler wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > [...]
> > There's a clear solution to this: sequence numbers.  There's a
> > convenient protocol which has them, too...
> 
> You must have meant TCP, but gdb's own remote protocol used to have
> optional sequence-numbered packets.  Have you considered bringing the
> latter back?

Yes, I remember them.  I don't think they help with this problem - the
packets were sequence-numbered, but the acks/naks were not.

-- 
Daniel Jacobowitz
CodeSourcery


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
       [not found]       ` <20080710223312.GA19058__14539.8706700236$1215729298$gmane$org@caradoc.them.org>
@ 2008-07-11  3:05         ` Frank Ch. Eigler
  2008-07-11  3:25           ` Daniel Jacobowitz
  0 siblings, 1 reply; 25+ messages in thread
From: Frank Ch. Eigler @ 2008-07-11  3:05 UTC (permalink / raw)
  To: Paul Koning; +Cc: sandra, gdb, gdb-patches, pedro

Daniel Jacobowitz <drow@false.org> writes:

> [...]
> There's a clear solution to this: sequence numbers.  There's a
> convenient protocol which has them, too...

You must have meant TCP, but gdb's own remote protocol used to have
optional sequence-numbered packets.  Have you considered bringing the
latter back?

- FChE


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-11  0:22         ` Sandra Loosemore
@ 2008-07-11  0:43           ` Daniel Jacobowitz
  0 siblings, 0 replies; 25+ messages in thread
From: Daniel Jacobowitz @ 2008-07-11  0:43 UTC (permalink / raw)
  To: Sandra Loosemore; +Cc: Paul Koning, gdb, gdb-patches, pedro

On Thu, Jul 10, 2008 at 08:19:36PM -0400, Sandra Loosemore wrote:
> My suggestion for dealing with the breakage was for the stub to send an  
> out-of-band ^C back to GDB when it has something to report, rather than 
> sending an actual stop reply asynchronously.  Then GDB could 
> (synchronously) poll the stub with an "eh, what's up?" packet, the stub 
> could reply, the normal +/- acks wouldn't be any more broken than they are 
> now, the stub could resend the ^C without any possibility of confusion if 
> it thought GDB hadn't gotten it the first time, etc.  I still think that's 
> workable, but the reaction here was "let's not go there; let's just assume 
> the connection is reliable".  I think everyone else's brain had exploded 
> by that point as well.  ;-)

Hmm.  I hadn't thought about the "can resend" bit.  You're right.  I
even have a design document and implementation lying around (from Jim
Blandy) for a new type of response which would work.  Let's discuss
that separately.

Assuming it does not become a dependency, Paul, do you have any other
objection?

-- 
Daniel Jacobowitz
CodeSourcery


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 22:33       ` Daniel Jacobowitz
@ 2008-07-11  0:22         ` Sandra Loosemore
  2008-07-11  0:43           ` Daniel Jacobowitz
  2008-07-11 13:43         ` Paul Koning
  1 sibling, 1 reply; 25+ messages in thread
From: Sandra Loosemore @ 2008-07-11  0:22 UTC (permalink / raw)
  To: Paul Koning, gdb, gdb-patches, pedro

Daniel Jacobowitz wrote:
> 
> [snip]
> 
> The result of this is that the acks become ambiguous in the presence
> of an unreliable or antagonistically delayed transport.  For instance,
> if GDB sends a memory write, the stub acks it, the stub replies with
> OK, and then GDB's ack is delayed.  Existing implementations of the
> protocol will resend the OK in this case, assuming the message was
> lost - from stub side that's indistinguishable from ack lost.  GDB's
> long-delayed ACK arrives on the stub at the same time the OK arrives
> at GDB.  GDB must ack again - it doesn't know whether the first ack
> ever made it through, and if it doesn't ack now then the stub might
> keep resending that OK until it gets through.  So now GDB sends an
> ack.  Simultaneously the stub sends a stop reply indicating that some
> other thread has stopped.  When it receives the ack, it thinks GDB saw
> the stop reply and does not resend it.  But GDB hasn't seen it yet,
> and if it is dropped the conversation is now out of sync.  GDB will
> hang around waiting for an event that has already been reported.

Thanks, Dan.  This is about the point at which my brain exploded.  :-P

My suggestion for dealing with the breakage was for the stub to send an 
out-of-band ^C back to GDB when it has something to report, rather than sending 
an actual stop reply asynchronously.  Then GDB could (synchronously) poll the 
stub with an "eh, what's up?" packet, the stub could reply, the normal +/- acks 
wouldn't be any more broken than they are now, the stub could resend the ^C 
without any possibility of confusion if it thought GDB hadn't gotten it the 
first time, etc.  I still think that's workable, but the reaction here was 
"let's not go there; let's just assume the connection is reliable".  I think 
everyone else's brain had exploded by that point as well.  ;-)

In any case, whether or not it ends up being a requirement for non-stop mode in 
the long run, being able to turn off the protocol acks seems useful on its own 
for reducing unnecessary round-trips when debugging over laggy TCP connections. 
  Did anyone have comments on the patch itself?

-Sandra


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 20:13     ` Paul Koning
@ 2008-07-10 22:33       ` Daniel Jacobowitz
  2008-07-11  0:22         ` Sandra Loosemore
  2008-07-11 13:43         ` Paul Koning
       [not found]       ` <20080710223312.GA19058__14539.8706700236$1215729298$gmane$org@caradoc.them.org>
  2008-07-11 15:11       ` Pedro Alves
  2 siblings, 2 replies; 25+ messages in thread
From: Daniel Jacobowitz @ 2008-07-10 22:33 UTC (permalink / raw)
  To: Paul Koning; +Cc: sandra, gdb, gdb-patches, pedro

Sandra asked me to take a stab at explaining the mess we're in.
Some credit also goes to Nathan Sidwell, for hours spent diagramming
this and ramming it through my thick skull.

On Thu, Jul 10, 2008 at 04:13:20PM -0400, Paul Koning wrote:
> Let me see if I understand this right.
> 
> 1. +/- ACKs are fine for the clasis (without non-stop) remote
>    protocol.
> 
> 2. ACKs are needed if the underlying transport isn't a reliable 
>    transport (for example a raw UART).  They aren't needed if the
>    underlying transport is TCP or equivalent.
> 
> 3. +/- ACKs are not good enough for non-stop mode.  (It's not clear to
>    me why -- is it because there may need to be more than one packet
>    in flight?  An explanation of what exactly is wrong would be
>    helpful to understand how to fix the issue.)

The current GDB protocol has very simple state.  At any moment, it is
either GDB's turn or the remote's turn to send events.  Both sides
never simultaneously think they have the token.  Sometimes neither
side thinks they have the token - either when a message is on the
wire, or else when a message has been lost.  Normally a timeout comes
to the rescue.

Non-stop is incompatible with this.  GDB can have the normal protocol
token, for instance if it is about to send a memory read.  At the same
time the debug agent can send packets.  This has to be the case;
otherwise GDB would have to frequently poll for state changes, which
would introduce too much overhead and traffic.

The result of this is that the acks become ambiguous in the presence
of an unreliable or antagonistically delayed transport.  For instance,
if GDB sends a memory write, the stub acks it, the stub replies with
OK, and then GDB's ack is delayed.  Existing implementations of the
protocol will resend the OK in this case, assuming the message was
lost - from stub side that's indistinguishable from ack lost.  GDB's
long-delayed ACK arrives on the stub at the same time the OK arrives
at GDB.  GDB must ack again - it doesn't know whether the first ack
ever made it through, and if it doesn't ack now then the stub might
keep resending that OK until it gets through.  So now GDB sends an
ack.  Simultaneously the stub sends a stop reply indicating that some
other thread has stopped.  When it receives the ack, it thinks GDB saw
the stop reply and does not resend it.  But GDB hasn't seen it yet,
and if it is dropped the conversation is now out of sync.  GDB will
hang around waiting for an event that has already been reported.

There's a clear solution to this: sequence numbers.  There's a
convenient protocol which has them, too...

> The implication is that the non-stop mode design abandons support for
> non-TCP transports.

No, the design abandons support for non-stop operation on lossy
transports.  I've used plenty of serial and UDP links that were in
practice sufficient.  If the link level is not sufficient, then the
implementor still has the option of wrapping a more reliable layer
between the transport and the gdb protocol communication.

> I would argue you need to identify why +/- ACKs aren't good enough,
> and propose a replacement that is good enough.  With that
> replacement you have a way to add the non-stop mode.  If the
> overhead of that replacement is significant in some plausible use
> case, you could then add a way to turn it off for the case where TCP
> is used end to end.

I think that if someone wants to design a more reliable protocol than
the existing one, they are free to do so, and either layer it under
the existing protocol as described above or contribute it to GDB -
we're not leaving anyone out in the cold and a new feature doesn't
have to meet every possible use case in its first incarnation.

This isn't the only problem with the existing protocol in my opinion.
It's pretty crufty, but it gets by.

-- 
Daniel Jacobowitz
CodeSourcery


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 19:59   ` Sandra Loosemore
@ 2008-07-10 20:13     ` Paul Koning
  2008-07-10 22:33       ` Daniel Jacobowitz
                         ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Paul Koning @ 2008-07-10 20:13 UTC (permalink / raw)
  To: sandra; +Cc: gdb, gdb-patches, pedro

>>>>> "Sandra" == Sandra Loosemore <sandra@codesourcery.com> writes:

 Sandra> Paul Koning wrote:
 >>>>>>> "Sandra" == Sandra Loosemore <sandra@codesourcery.com>
 >>>>>>> writes:
 >>
 Sandra> You'll note the documentation says turning off acks may be
 Sandra> desirable to reduce communication overhead *or* "for other
 Sandra> reasons".  In fact, it is the "other reasons" that motivated
 Sandra> this patch.  We are working on designing the extensions to
 Sandra> the remote protocol to support nonstop mode, and we realized
 Sandra> that we simply cannot do it in combination with using +/-
 Sandra> acks on the asynchronous responses.  If we need a reliable
 Sandra> transport layer to support nonstop mode, we might as well
 Sandra> turn the acks off completely instead of dealing with the
 Sandra> extra complexity of trying to design the nonstop protocol
 Sandra> around them.
 >>  Ok, so does that mean the nonstop mode features won't work unless
 >> the remote protocol is layered on TCP?  Given that a lot of the
 >> time the remote link is simply a UART serial link, is there an
 >> issue here?

 Sandra> Probably so, but the +/- acks are not the way to solve it.
 Sandra> :-(

 Sandra> Our internal discussion on that issue was getting more and
 Sandra> more complicated, to the point where I could not even follow
 Sandra> what the exact problem was.  ...

 Sandra> There's current practice (the existing Apple implementation)
 Sandra> to support disabling +/- acks and it seems useful as a
 Sandra> performance hack for TCP connections independently of the
 Sandra> nonstop extensions, so why not formalize it?

Let me see if I understand this right.

1. +/- ACKs are fine for the clasis (without non-stop) remote
   protocol.

2. ACKs are needed if the underlying transport isn't a reliable 
   transport (for example a raw UART).  They aren't needed if the
   underlying transport is TCP or equivalent.

3. +/- ACKs are not good enough for non-stop mode.  (It's not clear to
   me why -- is it because there may need to be more than one packet
   in flight?  An explanation of what exactly is wrong would be
   helpful to understand how to fix the issue.)

and finally

4. The proposal is to have a way to turn off +/-ACKs and this will be 
   used for non-stop mode.

The implication is that the non-stop mode design abandons support for
non-TCP transports.  This doesn't seem like a good thing.  I would
argue you need to identify why +/- ACKs aren't good enough, and
propose a replacement that is good enough.  With that replacement you
have a way to add the non-stop mode.  If the overhead of that
replacement is significant in some plausible use case, you could then
add a way to turn it off for the case where TCP is used end to end.

By the way, note that "target remote host:port" does NOT mean that you
have TCP end to end.  You may have TCP to a terminal server and raw
UART the rest of the way, so you still need ACKs (of one kind or
another). 

	  paul



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 19:09 ` Paul Koning
@ 2008-07-10 19:59   ` Sandra Loosemore
  2008-07-10 20:13     ` Paul Koning
  0 siblings, 1 reply; 25+ messages in thread
From: Sandra Loosemore @ 2008-07-10 19:59 UTC (permalink / raw)
  To: Paul Koning; +Cc: gdb, gdb-patches, pedro

Paul Koning wrote:
>>>>>> "Sandra" == Sandra Loosemore <sandra@codesourcery.com> writes:
> 
>  Sandra> You'll note the documentation says turning off acks may be
>  Sandra> desirable to reduce communication overhead *or* "for other
>  Sandra> reasons".  In fact, it is the "other reasons" that motivated
>  Sandra> this patch.  We are working on designing the extensions to
>  Sandra> the remote protocol to support nonstop mode, and we realized
>  Sandra> that we simply cannot do it in combination with using +/-
>  Sandra> acks on the asynchronous responses.  If we need a reliable
>  Sandra> transport layer to support nonstop mode, we might as well
>  Sandra> turn the acks off completely instead of dealing with the
>  Sandra> extra complexity of trying to design the nonstop protocol
>  Sandra> around them.
> 
> Ok, so does that mean the nonstop mode features won't work unless the
> remote protocol is layered on TCP?  Given that a lot of the time the
> remote link is simply a UART serial link, is there an issue here?  

Probably so, but the +/- acks are not the way to solve it.  :-(

Our internal discussion on that issue was getting more and more complicated, to 
the point where I could not even follow what the exact problem was.  My 
imperfect understanding is this:  In nonstop mode, stop replies are sent 
asynchronously, which breaks the back-and-forth, GDB-talks-stub-responds model. 
  The stub may send a stop reply when GDB is not expecting to read a regular 
packet response.  With some care, we could interleave the protocol acks of these 
responses with the acks of the regular synchronous responses....  except if a 
response or its ACK got lost entirely in transmission.  We got to the point of 
considering whether we needed to add some out-of-band protocol to support the 
asynchronous responses from the stub, mirroring the asynchronous ^C that GDB can 
send to the stub, for instance, before deciding we didn't want to go there right 
now.

There's current practice (the existing Apple implementation) to support 
disabling +/- acks and it seems useful as a performance hack for TCP connections 
independently of the nonstop extensions, so why not formalize it?

-Sandra


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
  2008-07-10 18:59 Sandra Loosemore
@ 2008-07-10 19:09 ` Paul Koning
  2008-07-10 19:59   ` Sandra Loosemore
  2008-07-25 13:41 ` Eli Zaretskii
  1 sibling, 1 reply; 25+ messages in thread
From: Paul Koning @ 2008-07-10 19:09 UTC (permalink / raw)
  To: sandra; +Cc: gdb, gdb-patches, pedro

>>>>> "Sandra" == Sandra Loosemore <sandra@codesourcery.com> writes:

 Sandra> Paul Koning wrote:
 >> I'm not sure this is a good idea.
 >> 
 >> For one thing, if you want to work on performance, there are much
 >> more dramatic changes to the protocol that could be done that
 >> would help much more.  I can't believe that the cost of acks is
 >> significant compared to all the other bottlenecks.

 Sandra> You'll note the documentation says turning off acks may be
 Sandra> desirable to reduce communication overhead *or* "for other
 Sandra> reasons".  In fact, it is the "other reasons" that motivated
 Sandra> this patch.  We are working on designing the extensions to
 Sandra> the remote protocol to support nonstop mode, and we realized
 Sandra> that we simply cannot do it in combination with using +/-
 Sandra> acks on the asynchronous responses.  If we need a reliable
 Sandra> transport layer to support nonstop mode, we might as well
 Sandra> turn the acks off completely instead of dealing with the
 Sandra> extra complexity of trying to design the nonstop protocol
 Sandra> around them.

Ok, so does that mean the nonstop mode features won't work unless the
remote protocol is layered on TCP?  Given that a lot of the time the
remote link is simply a UART serial link, is there an issue here?  

       paul


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [remote protocol] support for disabling packet acknowledgement
@ 2008-07-10 18:59 Sandra Loosemore
  2008-07-10 19:09 ` Paul Koning
  2008-07-25 13:41 ` Eli Zaretskii
  0 siblings, 2 replies; 25+ messages in thread
From: Sandra Loosemore @ 2008-07-10 18:59 UTC (permalink / raw)
  To: Paul Koning; +Cc: gdb, gdb-patches, Pedro Alves

Paul Koning wrote:

> I'm not sure this is a good idea.
> 
> For one thing, if you want to work on performance, there are much more
> dramatic changes to the protocol that could be done that would help
> much more.  I can't believe that the cost of acks is significant
> compared to all the other bottlenecks.

You'll note the documentation says turning off acks may be desirable to reduce 
communication overhead *or* "for other reasons".  In fact, it is the "other 
reasons" that motivated this patch.  We are working on designing the extensions 
to the remote protocol to support nonstop mode, and we realized that we simply 
cannot do it in combination with using +/- acks on the asynchronous responses. 
If we need a reliable transport layer to support nonstop mode, we might as well 
turn the acks off completely instead of dealing with the extra complexity of 
trying to design the nonstop protocol around them.

> Also, TCP is reliable delivery at the transport layer.  It doesn't do
> reliable delivery at the application layer -- that's what the gdb
> remote protocol ACKs do.  The fact that TCP delivered a packet to the
> stub doesn't mean the stub acted on it.

The +/- acks don't mean the stub acted on a packet, either; only that the stub 
received it and that its checksum passed.  The packet responses are what tell 
GDB that the stub has acted (or refused to act, as the case may be).

-Sandra


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2008-08-11 19:40 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-07-10 17:23 [remote protocol] support for disabling packet acknowledgement Pedro Alves
2008-07-10 18:44 ` Paul Koning
2008-07-10 19:08   ` Daniel Jacobowitz
2008-07-25 13:39 ` Eli Zaretskii
2008-08-11 19:40 ` Daniel Jacobowitz
2008-07-10 18:59 Sandra Loosemore
2008-07-10 19:09 ` Paul Koning
2008-07-10 19:59   ` Sandra Loosemore
2008-07-10 20:13     ` Paul Koning
2008-07-10 22:33       ` Daniel Jacobowitz
2008-07-11  0:22         ` Sandra Loosemore
2008-07-11  0:43           ` Daniel Jacobowitz
2008-07-11 13:43         ` Paul Koning
2008-07-11 14:35           ` Daniel Jacobowitz
     [not found]       ` <20080710223312.GA19058__14539.8706700236$1215729298$gmane$org@caradoc.them.org>
2008-07-11  3:05         ` Frank Ch. Eigler
2008-07-11  3:25           ` Daniel Jacobowitz
2008-07-11 15:11       ` Pedro Alves
2008-07-11 15:24         ` Daniel Jacobowitz
2008-07-11 15:54           ` Paul Koning
2008-07-11 17:59             ` Pedro Alves
2008-07-11 18:54               ` Paul Koning
2008-07-11 19:10                 ` Pedro Alves
2008-07-25 13:41 ` Eli Zaretskii
2008-07-25 14:14   ` Sandra Loosemore
2008-07-26  5:54     ` Eli Zaretskii

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox