[ovs-dev] [PATCH RFC] ofproto-dpif-upcall: Increment dump_seq before instead of after dump.

[Ben Pfaff]

On Tue, Jul 07, 2015 at 08:13:58PM -0700, Joe Stringer wrote:
> On 6 July 2015 at 22:20, Ben Pfaff <blp at nicira.com> wrote:
> > When process_upcall() passes a miss upcall to upcall_xlate(), the new
> > ukey's dump_seq member is initialized from the current dump_seq.  Later,
> > when udpif_revalidator() calls revalidate(), any dumped flow for which
> > ukey->dump_seq equals the current dump_seq is skipped.  However, until now
> > the current dump_seq was only incremented *after* a revalidation run is
> > completed.  That means that, if a ukey added is added between revalidation
> > runs, it will be skipped on the subsequent revalidation run.  This commit
> > fixes the problem by incrementing dump_seq just before a revalidation run
> > instead of just after.
> 
> I think this change makes sense. In cases with few flows (or generally
> low revalidator workload), it makes stats and forwarding behaviour
> more accurate, then in cases with high revalidator workload there
> should be no effect (the end of one round melds into the start of the
> next).
> 
> The few areas that I can think of which are relevant are:
> - ofproto-dpif's run() function uses udpif_dump_seq() to determine
> when all the stats have just finished being updated, indicating it's a
> good time to do periodic work like timing out openflow flows and
> rebalancing bonds. It seems like this would be best performed between
> revalidation rounds, but I don't have a good way of quantifying how
> much this matters.
> - In the testsuite we use 'ovs-appctl revalidator/wait' (which is
> based on dump_seq) to determine when the flows have been dumped and
> stats updated, which will be triggered at the end of revalidation
> currently. This change would make it more likely that the testsuite
> calls revalidator/wait while the revalidators are sleeping, and
> returns before the flows are actually dumped. This may or may not be a
> problem in practice, unfortunately the testuite only shows up this
> kind of issue as intermittent failures on various tests. Anyway, this
> could be trivially solved if it shows to be a problem.
> - The race conditions mentioned in ukey_install() for the userspace
> datapath are more likely, due to there being a smaller window between
> dump_seq being incremented and revalidators reaching the GC stage. In
> practice I doubt it's a problem.

Thanks for all the thoughts!

Do you think that it's best to address all of these by experimenting and
fixing any observed problems (how?), or by adding comments that describe
the design and the possible shortcomings, or by adding to the commit
message, or by doing something else?