On 7 July 2015 at 21:42, Ben Pfaff <[email protected]> wrote: > On Tue, Jul 07, 2015 at 08:13:58PM -0700, Joe Stringer wrote: >> On 6 July 2015 at 22:20, Ben Pfaff <[email protected]> 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?
In practice, the cases above require the intersection of multiple corner cases before they present themselves. This makes them harder to address. I'm also aware that having spent lots of time in this code I'm prone to overstate the probability of some of these issues :-) One general approach would be to divide 'dump_seq' into 'dump_start_seq' and 'dump_end_seq'. The first two cases above can use the end_seq, while the ukey logic can use the start_seq. I expect that this will fix the test case you mention with a smaller change to the current logic. This addresses the first two cases above. Case-by-case: - The run() issue is most likely to present itself when we have openflow flows with a small idle time, eg 1 second. We might be able to produce a case where we install such a flow, then send some traffic to hit that flow, and the flow is timed out despite the packets arriving. However, with 1-second revalidation cycles we're always going to have poor accuracy in this kind of case, so this is probably best addressed through documentation. If we don't already document what can be expected about the stats collection, then we probably should. - The revalidator/wait issue is most likely to show up as intermittent test failures for tests like "ofproto-dpif - ofproto/trace from dpctl output", where stats are wrong for a test because revalidators didn't run, or we're filtering flow dump messages and expecting them to be there (but they aren't, because revalidators didn't run to dump the flows yet). If such tests fail intermittently on travis, we can modify the logic to, eg, perform revalidator/wait twice, or modify the udpif_run() to only respond to unix connections after a couple of revalidation rounds. - In the worst case, the ukey_install() issue will show up as extraneous warnings about "failed to flow_del", because the flow is marked as "existing in the datapath", but it wasn't yet installed. Ultimately the solution to this problem is to refactor the upcall_cb() interface to split flow installation into two phases, like it happens for the linux datapath. This might not even be much work, I just didn't consider it worthwhile due to the low probability of hitting the issue. Most of these cases will come up more frequently on single-core boxes with high parallelism, due to more crazy scheduling of OVS threads. The limited resources in the travis-ci environment can pick up on some of these issues. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
