[ovs-dev] [resubmit 3/4] ofproto: Batch statistics updates.

Ethan Jackson ethan at nicira.com
Tue Feb 15 20:34:17 UTC 2011


Your way is a bit clearer so I will go ahead and change it.

Ethan

On Tue, Feb 15, 2011 at 12:30 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Feb 14, 2011 at 02:06:00PM -0800, Ethan Jackson wrote:
>> Facet statistics are updated once per second during
>> ofproto_expire() instead of upon request.  This will greatly
>> simplify implementation of future patches. This commit also changes
>> each facet's packet and byte counters to include the statistics
>> stored in the datapath.
>
> This looks good.  Thank you!
>
> The new code in ofproto_update_used() struck me as a little odd in form,
> because it has duplicate assignments of the form "facet->dp_something =
> stats->something".  I think that I would write the following:
>
>    if (stats->n_packets < facet->dp_packet_count) {
>        VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
>        facet->dp_packet_count = stats->n_packets;
>    }
>
>    if (stats->n_bytes < facet->dp_byte_count) {
>        VLOG_WARN_RL(&rl, "unexpected byte count from datapath");
>        facet->dp_byte_count = stats->n_bytes;
>    }
>
>    facet->packet_count += stats->n_packets - facet->dp_packet_count;
>    facet->byte_count += stats->n_bytes - facet->dp_byte_count;
>
>    facet->dp_packet_count = stats->n_packets;
>    facet->dp_byte_count = stats->n_bytes;
>
> more like this:
>
>    /* Update packet count. */
>    if (stats->n_packets >= facet->dp_packet_count) {
>        facet->packet_count += stats->n_packets - facet->dp_packet_count;
>    } else {
>        VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
>    }
>    facet->dp_packet_count = stats->n_packets;
>
>    /* Update byte count. */
>    if (stats->n_bytes >= facet->dp_byte_count) {
>        facet->byte_count += stats->n_bytes - facet->dp_byte_count;
>    } else {
>        VLOG_WARN_RL(&rl, "unexpected byte count from the datapath");
>    }
>    facet->dp_byte_count = stats->n_bytes;
>
> I'm just quibbling.  The code as you wrote it looks correct to em.
>
> Thanks,
>
> Ben.
>




More information about the dev mailing list