[PATCH] mt set per-command remote-packets on|off

[PATCH] mt set per-command remote-packets on|off

[Doug Evans]

Hi.

If there are no objections, I plan to check this in.
It adds "mt set per-command packet on|off", and is helpful
in examining remote serial protocol issues.

Tested on amd64-linux.

2013-11-27  Doug Evans  <dje@google.com>

	* NEWS: Mention new command "mt set per-command remote-packet on|off".
	* maint.c: #include "remote.h".
	(per_command_packet): New static global.
	(struct cmd_stats): New members packet_enabled, start_packets_sent,
	start_packets_rcvd, start_packet_bytes_sent, start_packet_bytes_rcvd.
	(report_command_stats): Print packet stats if asked.
	(make_command_stats_cleanup): Collect packet stats if asked.
	(_initialize_maint_cmds): New command "mt set per-command remote-packet
	on|off".
	* remote.c (struct remote_state): New members packets_sent,
	packets_received, bytes_sent, bytes_received.
	(get_remote_packet_stats): New function.
	(putpkt_binary, getpkt_or_notif_sane_1): Update stats.
	* remote.h (get_remote_packet_stats): Declare.

	doc/
	* gdb.texinfo (Maintenance Commands): Add docs for
	"mt set per-command remote-packet {on,off}".

diff --git a/gdb/NEWS b/gdb/NEWS
index 5110b27..d393b4c 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -74,6 +74,7 @@ maint set|show per-command
 maint set|show per-command space
 maint set|show per-command time
 maint set|show per-command symtab
+maint set|show per-command remote-packets
   Enable display of per-command gdb resource usage.
 
 remove-symbol-file FILENAME
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 004c376..d613da5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -37611,6 +37611,23 @@ number of primary symbol tables
 @item
 number of blocks in the blockvector
 @end enumerate
+
+@item maint set per-command remote-packets [on|off]
+@itemx maint show per-command remote-packets
+Enable or disable the printing of remote protocol packet statistics
+for each command.
+If enabled, @value{GDBN} will display the following information:
+
+@enumerate a
+@item
+number of packets sent
+@item
+number of packets received
+@item
+number of bytes sent
+@item
+number of bytes received
+@end enumerate
 @end table
 
 @kindex maint space
diff --git a/gdb/maint.c b/gdb/maint.c
index 71c4b85..cb8decc 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -42,7 +42,7 @@
 #include "top.h"
 #include "timeval-utils.h"
 #include "maint.h"
-
+#include "remote.h"
 #include "cli/cli-decode.h"
 #include "cli/cli-utils.h"
 #include "cli/cli-setshow.h"
@@ -746,6 +746,10 @@ static int per_command_space;
 
 static int per_command_symtab;
 
+/* If nonzero, display basic remote-protocol packet stats for each command.  */
+
+static int per_command_remote_packets;
+
 /* mt per-command commands.  */
 
 static struct cmd_list_element *per_command_setlist;
@@ -765,6 +769,7 @@ struct cmd_stats
   int time_enabled : 1;
   int space_enabled : 1;
   int symtab_enabled : 1;
+  int remote_packets_enabled : 1;
   long start_cpu_time;
   struct timeval start_wall_time;
   long start_space;
@@ -774,6 +779,10 @@ struct cmd_stats
   int start_nr_primary_symtabs;
   /* Total number of blocks.  */
   int start_nr_blocks;
+  /* Total number of packets sent/received.  */
+  long start_packets_sent, start_packets_rcvd;
+  /* Total number of packet bytes sent/received.  */
+  long start_packet_bytes_sent, start_packet_bytes_rcvd;
 };
 
 /* Set whether to display time statistics to NEW_VALUE
@@ -885,6 +894,30 @@ report_command_stats (void *arg)
 			 nr_blocks,
 			 nr_blocks - start_stats->start_nr_blocks);
     }
+
+  if (start_stats->remote_packets_enabled)
+    {
+      long packets_sent, packets_rcvd, bytes_sent, bytes_rcvd;
+
+      get_remote_packet_stats (&packets_sent, &packets_rcvd,
+			       &bytes_sent, &bytes_rcvd);
+      if (packets_sent > 0)
+	{
+	  printf_unfiltered (_("#pkts sent: %ld (+%ld),"
+			       " #pkts rcvd: %ld (+%ld)\n"
+			       "#bytes sent: %ld (+%ld),"
+			       " #bytes rcvd: %ld (+%ld)\n"),
+			     packets_sent,
+			     packets_sent - start_stats->start_packets_sent,
+			     packets_rcvd,
+			     packets_rcvd - start_stats->start_packets_rcvd,
+			     bytes_sent,
+			     bytes_sent - start_stats->start_packet_bytes_sent,
+			     bytes_rcvd,
+			     (bytes_rcvd
+			      - start_stats->start_packet_bytes_rcvd));
+	}
+    }
 }
 
 /* Create a cleanup that reports time and space used since its creation.
@@ -933,6 +966,19 @@ make_command_stats_cleanup (int msg_type)
       new_stat->symtab_enabled = 1;
     }
 
+  if (per_command_remote_packets)
+    {
+      long packets_sent, packets_rcvd, bytes_sent, bytes_rcvd;
+
+      get_remote_packet_stats (&packets_sent, &packets_rcvd,
+			       &bytes_sent, &bytes_rcvd);
+      new_stat->start_packets_sent = packets_sent;
+      new_stat->start_packets_rcvd = packets_rcvd;
+      new_stat->start_packet_bytes_sent = bytes_sent;
+      new_stat->start_packet_bytes_rcvd = bytes_rcvd;
+      new_stat->remote_packets_enabled = 1;
+    }
+
   /* Initalize timer to keep track of how long we waited for the user.  */
   reset_prompt_for_continue_wait_time ();
 
@@ -1086,6 +1132,16 @@ displayed following the command's output."),
 			   NULL, NULL,
 			   &per_command_setlist, &per_command_showlist);
 
+  add_setshow_boolean_cmd ("remote-packets", class_maintenance,
+			   &per_command_remote_packets, _("\
+Set whether to display per-command remote-protocol packet statistics."), _("\
+Show whether to display per-command remote-protocol packet statistics."),
+			   _("\
+If enabled, the remote protocol packet statistics for each command will be\n\
+displayed following the command's output."),
+			   NULL, NULL,
+			   &per_command_setlist, &per_command_showlist);
+
   /* This is equivalent to "mt set per-command time on".
      Kept because some people are used to typing "mt time 1".  */
   add_cmd ("time", class_maintenance, maintenance_time_display, _("\
diff --git a/gdb/remote.c b/gdb/remote.c
index 186c058..429a49c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -426,6 +426,14 @@ struct remote_state
 
   /* The state of remote notification.  */
   struct remote_notif_state *notif_state;
+
+  /* Statistics for measuring protocol efficiency.
+     Note: These record data at the protocol level, so to speak.
+     If we're communicating over a flaky channel and have to resent packets
+     that is not accounted for here.  It's recommended to track resends, etc.
+     separately.  */
+  long packets_sent, packets_received;
+  long bytes_sent, bytes_received;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -629,6 +637,20 @@ get_remote_state (void)
   return get_remote_state_raw ();
 }
 
+/* Fetch the current packet statistics.  */
+
+void
+get_remote_packet_stats (long *packets_sent, long *packets_received,
+			 long *bytes_sent, long *bytes_received)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  *packets_sent = rs->packets_sent;
+  *packets_received = rs->packets_received;
+  *bytes_sent = rs->bytes_sent;
+  *bytes_received = rs->bytes_received;
+}
+
 static int
 compare_pnums (const void *lhs_, const void *rhs_)
 {
@@ -7305,6 +7327,10 @@ putpkt_binary (char *buf, int cnt)
   *p++ = tohex ((csum >> 4) & 0xf);
   *p++ = tohex (csum & 0xf);
 
+  /* Update the stats counters.  */
+  ++rs->packets_sent;
+  rs->bytes_sent += p - buf2;
+
   /* Send it over and over until we get a positive ack.  */
 
   while (1)
@@ -7748,6 +7774,10 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
 	  return -1;
 	}
 
+      /* Update the stats counters.  */
+      ++rs->packets_received;
+      rs->bytes_received += val;
+
       /* If we got an ordinary packet, return that to our caller.  */
       if (c == '$')
 	{
diff --git a/gdb/remote.h b/gdb/remote.h
index b6ea547..1b4c482 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -71,4 +71,10 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
 					      int *poffset);
 
 extern void remote_notif_get_pending_events (struct notif_client *np);
+
+extern void get_remote_packet_stats (long *packets_sent,
+				     long *packets_received,
+				     long *bytes_sent,
+				     long *bytes_received);
+
 #endif

Re: [PATCH] mt set per-command remote-packets on|off

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> It adds "mt set per-command packet on|off", and is helpful
Doug> in examining remote serial protocol issues.

I'm curious what happens in target-async mode with a background command.
I suppose the stats are associated with the subsequent command?

Also, not your problem I guess, but it this seems like this will do the
wrong thing when there are multiple remote targets, since the accounting
is per-remote.  If you really want it to be per-command it seems that
you might as well just make the stats be globals, not in remote_state.

Doug>+     If we're communicating over a flaky channel and have to resent packets

Typo, "resend".

Tom

Re: [PATCH] mt set per-command remote-packets on|off

[Doug Evans]

On Wed, Nov 27, 2013 at 3:58 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> It adds "mt set per-command packet on|off", and is helpful
> Doug> in examining remote serial protocol issues.
>
> I'm curious what happens in target-async mode with a background command.
> I suppose the stats are associated with the subsequent command?

Depends on when the transactions happen.
I played with it in target-async mode - as you know I'm seeing some
poor gdbserver performance in target-async+non-stop.

The absolute counts are ok, it's just the deltas that can be off.
It's good enough for my intended usage.

> Also, not your problem I guess, but it this seems like this will do the
> wrong thing when there are multiple remote targets, since the accounting
> is per-remote.  If you really want it to be per-command it seems that
> you might as well just make the stats be globals, not in remote_state.

Thought about that too.
I'd like having per-remote stats so I kept them in remote_state.
The per-command stats will just report one target (and get the deltas
wrong if the target ever switches during the command), but it's a
pretty rare event at the moment.
And if/when we start having multiple remote targets be more common I
was thinking of having a command to allow dumping all the stats
(should such a need ever arise - it may not).

I was thinking about at least printing ??? instead of negative values
for the deltas should they occur.
I may do that now.

> Doug>+     If we're communicating over a flaky channel and have to resent packets
>
> Typo, "resend".

Righto.

Re: [PATCH] mt set per-command remote-packets on|off

[Tom Tromey]

Doug> I'd like having per-remote stats so I kept them in remote_state.
Doug> The per-command stats will just report one target (and get the deltas
Doug> wrong if the target ever switches during the command), but it's a
Doug> pretty rare event at the moment.
Doug> And if/when we start having multiple remote targets be more common I
Doug> was thinking of having a command to allow dumping all the stats
Doug> (should such a need ever arise - it may not).

Doug> I was thinking about at least printing ??? instead of negative values
Doug> for the deltas should they occur.
Doug> I may do that now.

Perhaps the printing ought to zero the stats in the remote_state.
Or perhaps the printing ought to be delegated to remote.c.

Tom

Re: [PATCH] mt set per-command remote-packets on|off

[Eli Zaretskii]

> From: Doug Evans <dje@google.com>
> Date: Wed, 27 Nov 2013 14:43:49 -0800
> 
> If there are no objections, I plan to check this in.
> It adds "mt set per-command packet on|off", and is helpful
> in examining remote serial protocol issues.

OK for the documentation parts.

Thanks.

Re: [PATCH] mt set per-command remote-packets on|off

[Yao Qi]

On 11/28/2013 06:43 AM, Doug Evans wrote:
> +
> +  if (start_stats->remote_packets_enabled)
> +    {
> +      long packets_sent, packets_rcvd, bytes_sent, bytes_rcvd;
> +
> +      get_remote_packet_stats (&packets_sent, &packets_rcvd,
> +			       &bytes_sent, &bytes_rcvd);
> +      if (packets_sent > 0)
> +	{
> +	  printf_unfiltered (_("#pkts sent: %ld (+%ld),"
> +			       " #pkts rcvd: %ld (+%ld)\n"
> +			       "#bytes sent: %ld (+%ld),"
> +			       " #bytes rcvd: %ld (+%ld)\n"),
> +			     packets_sent,
> +			     packets_sent - start_stats->start_packets_sent,
> +			     packets_rcvd,
> +			     packets_rcvd - start_stats->start_packets_rcvd,
> +			     bytes_sent,
> +			     bytes_sent - start_stats->start_packet_bytes_sent,
> +			     bytes_rcvd,
> +			     (bytes_rcvd
> +			      - start_stats->start_packet_bytes_rcvd));
> +	}
> +    }

It might not be good to bypass target interface to get the data about
remote target in target independent code.  We want to print some 
statistics of target after each command.  For "remote" target, we print 
statistics on packet, and for other targets, we may want to print 
something else.  IWBN to add a new interface target_print_statistics, 
and leave each target to decide what data to print.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index 186c058..429a49c 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -426,6 +426,14 @@ struct remote_state
>
>     /* The state of remote notification.  */
>     struct remote_notif_state *notif_state;
> +
> +  /* Statistics for measuring protocol efficiency.
> +     Note: These record data at the protocol level, so to speak.
> +     If we're communicating over a flaky channel and have to resent packets
> +     that is not accounted for here.  It's recommended to track resends, etc.
> +     separately.  */
> +  long packets_sent, packets_received;
> +  long bytes_sent, bytes_received;

Use "unsigned long"?

IWBN to do statistics in a fine granularity, to count for each 
frequently-used type of packet.  For example, we can count the sent 'm', 
'x', and 'g' packet, then we'll get more clues from the the statistics 
messages.

Probably we can create a new structure, say "remote_packet_statistics", 
for these fields....

>   };
>
>   /* Private data that we'll store in (struct thread_info)->private.  */
> @@ -629,6 +637,20 @@ get_remote_state (void)
>     return get_remote_state_raw ();
>   }
>
> +/* Fetch the current packet statistics.  */
> +
> +void
> +get_remote_packet_stats (long *packets_sent, long *packets_received,
> +			 long *bytes_sent, long *bytes_received)
> +{

... In this way, the number of arguments of function can be reduced.

> +  struct remote_state *rs = get_remote_state ();
> +
> +  *packets_sent = rs->packets_sent;
> +  *packets_received = rs->packets_received;
> +  *bytes_sent = rs->bytes_sent;
> +  *bytes_received = rs->bytes_received;
> +}
> +

I am trying to add a python binding to get the statistics of rsp 
packets, which can be used as a new measurement in perf testsuite.
If one change causes the number of packets increased dramatically, the 
change may cause some performance regressions in remote debugging.
Looks I can base my work on top of this patch.

-- 
Yao (齐尧)

Re: [PATCH] mt set per-command remote-packets on|off

[Doug Evans]

blech, gmail again.

On Wed, Nov 27, 2013 at 10:08 PM, Yao Qi <yao@codesourcery.com> wrote:
>
> On 11/28/2013 06:43 AM, Doug Evans wrote:
>>
>> +
>> +  if (start_stats->remote_packets_enabled)
>> +    {
>> +      long packets_sent, packets_rcvd, bytes_sent, bytes_rcvd;
>> +
>> +      get_remote_packet_stats (&packets_sent, &packets_rcvd,
>> +                              &bytes_sent, &bytes_rcvd);
>> +      if (packets_sent > 0)
>> +       {
>> +         printf_unfiltered (_("#pkts sent: %ld (+%ld),"
>> +                              " #pkts rcvd: %ld (+%ld)\n"
>> +                              "#bytes sent: %ld (+%ld),"
>> +                              " #bytes rcvd: %ld (+%ld)\n"),
>> +                            packets_sent,
>> +                            packets_sent - start_stats->start_packets_sent,
>> +                            packets_rcvd,
>> +                            packets_rcvd - start_stats->start_packets_rcvd,
>> +                            bytes_sent,
>> +                            bytes_sent - start_stats->start_packet_bytes_sent,
>> +                            bytes_rcvd,
>> +                            (bytes_rcvd
>> +                             - start_stats->start_packet_bytes_rcvd));
>> +       }
>> +    }
>
>
> It might not be good to bypass target interface to get the data about
> remote target in target independent code.  We want to print some statistics of target after each command.  For "remote" target, we print statistics on packet, and for other targets, we may want to print something else.



Good point.  OTOH going that route brings more complication that isn't
needed at the moment (rsp perf data *is* needed now).
[I finally have some(!) time to return to this.]


>
> IWBN to add a new interface target_print_statistics, and leave each target to decide what data to print.


From a high level perspective the target collects the statistics, but
consumers will want to print them their own way.
I didn't want to go "whole hog" yet because it's not clear yet what
the different consumers are.
[And remote protocol performance is in real need of improvement so I
wanted to start solving that problem now.]


>
>
>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 186c058..429a49c 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -426,6 +426,14 @@ struct remote_state
>>
>>     /* The state of remote notification.  */
>>     struct remote_notif_state *notif_state;
>> +
>> +  /* Statistics for measuring protocol efficiency.
>> +     Note: These record data at the protocol level, so to speak.
>> +     If we're communicating over a flaky channel and have to resent packets
>> +     that is not accounted for here.  It's recommended to track resends, etc.
>> +     separately.  */
>> +  long packets_sent, packets_received;
>> +  long bytes_sent, bytes_received;
>
>
> Use "unsigned long"?


How about int?

>
> IWBN to do statistics in a fine granularity, to count for each frequently-used type of packet.  For example, we can count the sent 'm', 'x', and 'g' packet, then we'll get more clues from the the statistics messages.


Sure, but that's a finer grained (and vastly more verbose) metric than
I'm looking for in the output of "mt set per on".

>
> Probably we can create a new structure, say "remote_packet_statistics", for these fields....
>
>
>>   };
>>
>>   /* Private data that we'll store in (struct thread_info)->private.  */
>> @@ -629,6 +637,20 @@ get_remote_state (void)
>>     return get_remote_state_raw ();
>>   }
>>
>> +/* Fetch the current packet statistics.  */
>> +
>> +void
>> +get_remote_packet_stats (long *packets_sent, long *packets_received,
>> +                        long *bytes_sent, long *bytes_received)
>> +{
>
>
> ... In this way, the number of arguments of function can be reduced.


It was done this way to separate the collector of the data from the
printer of the data.
If we later collect finer grained stats, callers that want a gross
number can still call this, and this function could sum up the pieces.

>
>
>> +  struct remote_state *rs = get_remote_state ();
>> +
>> +  *packets_sent = rs->packets_sent;
>> +  *packets_received = rs->packets_received;
>> +  *bytes_sent = rs->bytes_sent;
>> +  *bytes_received = rs->bytes_received;
>> +}
>> +
>
>
> I am trying to add a python binding to get the statistics of rsp packets, which can be used as a new measurement in perf testsuite.
> If one change causes the number of packets increased dramatically, the change may cause some performance regressions in remote debugging.
> Looks I can base my work on top of this patch.


Indeed, adding rsp stats for the perf testsuite is something I want to
see happen.
Another consumer of the data.

Re: [PATCH] mt set per-command remote-packets on|off

[Yao Qi]

On 12/11/2013 05:12 AM, Doug Evans wrote:
> 
> 
> Good point.  OTOH going that route brings more complication that isn't
> needed at the moment (rsp perf data*is*  needed now).
> [I finally have some(!) time to return to this.]

That is good! :)

> 
> 
>> >
>> >IWBN to add a new interface target_print_statistics, and leave each target to decide what data to print.
> 
>  From a high level perspective the target collects the statistics, but
> consumers will want to print them their own way.
> I didn't want to go "whole hog" yet because it's not clear yet what
> the different consumers are.
> [And remote protocol performance is in real need of improvement so I
> wanted to start solving that problem now.]
> 

That is OK to me.  I am eager to see collect rsp perf data through
this and potential improvement, rather than designing an ideal
interface.

> 
>> >
>> >
>> >
>>> >>diff --git a/gdb/remote.c b/gdb/remote.c
>>> >>index 186c058..429a49c 100644
>>> >>--- a/gdb/remote.c
>>> >>+++ b/gdb/remote.c
>>> >>@@ -426,6 +426,14 @@ struct remote_state
>>> >>
>>> >>     /* The state of remote notification.  */
>>> >>     struct remote_notif_state *notif_state;
>>> >>+
>>> >>+  /* Statistics for measuring protocol efficiency.
>>> >>+     Note: These record data at the protocol level, so to speak.
>>> >>+     If we're communicating over a flaky channel and have to resent packets
>>> >>+     that is not accounted for here.  It's recommended to track resends, etc.
>>> >>+     separately.  */
>>> >>+  long packets_sent, packets_received;
>>> >>+  long bytes_sent, bytes_received;
>> >
>> >
>> >Use "unsigned long"?
> 
> How about int?
>

If you prefer signed type here, "long" is fine.

>>> >>
>>> >>+/* Fetch the current packet statistics.  */
>>> >>+
>>> >>+void
>>> >>+get_remote_packet_stats (long *packets_sent, long *packets_received,
>>> >>+                        long *bytes_sent, long *bytes_received)
>>> >>+{
>> >
>> >
>> >... In this way, the number of arguments of function can be reduced.
> 
> It was done this way to separate the collector of the data from the
> printer of the data.
> If we later collect finer grained stats, callers that want a gross
> number can still call this, and this function could sum up the pieces.
> 

Even we just count these four, it is better to put them in a
structure, and associate the structure instance with remote_state.

-- 
Yao (齐尧)