[ovs-dev] [PATCH] ofproto: Resubmit statistics improperly account during failover.

[Ben Pfaff]

That helps.

So, looking through ofproto.c, let me see if I understand.  Is the
following correct:

    ofproto tracks facet stats that have already been accounted into
    OpenFlow rule using dp_packet_count and dp_byte_count.  When we
    query the database, only packets or bytes in excess of those
    values should be considered.  The 'stat's parameter to
    facet_update_stats() should only be passed those values in excess.
    Some of the callers, however, were not doing this:
    facet_revalidate() failed to subtract already-accounted packets
    and bytes, and so did facet_uninstall().

If I'm right about that, then your patch is correct but insufficient:
we should make a similar change to facet_uninstall(), perhaps
factoring out the common code into a function.

It'd be good to document what facet_put__() now stores into '*stats',
e.g.:

/* Creates or modifies 'facet''s flow in 'ofproto->dpif', setting its
   actions to the 'actions_len' bytes of actions in 'actions'.  If
   'stats' is nonnull, zeros any existing statistics for the flow in
   the dpif and stores the statistics in excess of those already
   recorded as 'facet->dp_packet_count' and 'facet->dp_byte_count'
   into '*stats'.  Returns 0 if successful, otherwise a positive
   errno value. */

Thank you!

On Tue, May 03, 2011 at 01:03:00PM -0700, Ethan Jackson wrote:
> I added the following paragraph to the commit message.
> 
>     Generally speaking, when a facet is facet_put__() into the
>     datapath, the kernel returns the old flow's statistics so they may
>     be accounted for in user space.  These statistics are generally
>     pushed down to the relevant facet's resubmit children.  Before this
>     patch, facet_put__() did not compensate for the fact that many of
>     the statistics in the datapath may have been already pushed.
>     Thus the entire packet count stored in the datapath would be pushed
>     to its children instead of simply the packets which have accrued
>     since the last accounting.  This patch fixes the behavior by
>     subtracting already accounted for packets from the statistics
>     returned by the datapath.
> 
> On Tue, May 3, 2011 at 12:42, Ben Pfaff <blp at nicira.com> wrote:
> > On Mon, May 02, 2011 at 01:38:50PM -0700, Ethan Jackson wrote:
> >> In some cases, when a facet's actions change because it resubmits
> >> into a different rule. ?It will account all packets it as accrued
> >> in the datapath to the new rule. ?Due to the algorithm we are
> >> using, it is acceptable for a facet to miscount at most 1 second
> >> worth of packets in this manor. ?This patch implements the proper
> >> behavior.
> >
> > The above commit message does a good job explaining the "why", but not
> > the observable effect of the change. ?I'm feeling really slow today,
> > so would you mind walking me through what happens differently with
> > this commit applied?
> >
> > Thanks,
> >
> > Ben.
> >