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

[Ben Pfaff]

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.