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

Doug Evans
2013-12-10T21:12:51Z

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.