[ovs-dev] [PATCH] ofproto-dpif: Init ukey->dump_seq to zero

Joe Stringer joe at ovn.org
Sun Apr 8 20:07:41 UTC 2018

On 6 April 2018 at 10:44, Ben Pfaff <blp at ovn.org> wrote:
> [also CCing Joe on the chance that he wants to comment]
> On Fri, Apr 06, 2018 at 09:34:38AM +0000, Jan Scheurich wrote:
>> > -----Original Message-----
>> > From: Ben Pfaff [mailto:blp at ovn.org]
>> > Sent: Wednesday, 04 April, 2018 22:28
>> >
>> > Oh, that's weird.  It's as if I didn't read the patch.  Maybe I just
>> > read some preliminary version in another thread.
>> >
>> > Anyway, you're totally right.  I applied this to master.  If you're
>> > seeing problems in another branch, let me know and I will backport.
>> Thanks!
>> I think the issue was introduced into OVS by the following commit a long time ago.
>> commit 23597df052262dec961fd86eb7c54d10984a1ec0
>> Author: Joe Stringer <joestringer at nicira.com>
>> Date:   Fri Jul 25 13:54:24 2014 +1200
>> It's a temporary glitch that can cause unexpected behavior only within
>> the first few hundred milliseconds after datapath flow creation. It is
>> most likely to affect "reactive" controller use cases (MAC learning,
>> ARP handling), like the OVN test case that now failed with a small
>> change of timing. So it is possible that one could notice short packet
>> drops or duplicate PACKET_INs in real SDN deployments when looking
>> close enough.

I see, so there's two seqs used in this code, dump_seq and reval_seq.
The purpose of the dump_seq is just to handle the case where a thread
dumps the exact same flow from the kernel datapath twice in quick
succession. This may happen because the kernel API doesn't provide a
guarantee about dumping an exact snapshot of the entire table, so if
one set of flows (~10-15) are dumped from the kernel, then a flow is
inserted into the bucket that is being dumped right now, then a
subsequent dump will begin from the same bucket/index as where the
previous dump ended, and perhaps rather than starting from the very
next flow, it starts with the last flow of the previous dump. Hence we
see the same flow twice.

>From this perspective, setting the dump_seq during upcall processing
does not make sense, so I agree with the basis of the patch. However,
I note that you also set it to 0 in ukey_create_from_dpif_flow(),
which is actually a dump so the same flow could be revalidated twice.
This doesn't quite seem right according to the above reasoning, but
that said looking over the code I can't see anything problematic with
handling it this way. Creating ukeys from dpif_flows is very much a
corner case and it seems unlikely to have a large impact even if there
is some logical mismatch in this case. (Equally, because it's such a
corner case, people are far less likely to notice a problem here..)

If I follow the order of relevant executions is something like:

* Openflow has set of rules R1
* dump_seq is set to N1
* new datapath flow arrives, creates upcall with N1 and associated
ukey with same dump_seq
* datapath flow is forwarded to controller, controller pushes new
openflow flow, rules are now R2
* during the same dump, the revalidator dumps the newly installed
flow, finds the ukey, sees the dump_seq N1 and skips revalidating it
although there are new rules R2
* traffic continues to be forwarded according to R1 until the next dump
* Because of rule transition R1->R2, another flow dump / revalidation
round is immediately triggered via reval_seq, with minimum bound of
starting in T+5ms
* dump_seq is set to N2
* During this second revalidation round, the datapath flow is actually
checked, determined to be forwarding per out-of-date behaviour, and
updated / evicted.

I would think that if you have a small number of flows, a new
revalidation round should be triggered fairly quickly due to reval_seq
(scheduled to happen in minimum ~5ms, as per updif_revalidator()). In
this window the flow will not be revalidated so you would still see
blips like this. On the other hand, if you have a large number of
flows, statistically you may not end up dumping the newly installed
flow during the same dump, so it may still take hundreds of
milliseconds for the dump to finish, to trigger revalidation due to
the R1->R2 rule change, then for the flow to eventually be revalidated
to implement the new behaviour. I don't know that it would make a big
difference there. That said, for the case where the flow /is/ dumped
during the same round, its actions may be updated so OVS would be more
responsive to OpenFlow changes in that case.

If you think there's a slightly different timeline, I'd be curious of
your thoughts on it though I won't provide any guarantee I'll be able
to provide further insight into it :-)

I believe that you observe an improvement in behaviour with the patch,
so I have no objections. Logically it seems the right direction. That
said, I doubt that it's able to make the datapath more responsive in
all cases as it's really quite difficult to synchronize the state
between OpenFlow and the datapath within a short period. The general
rule we tend to observe in this area of the code is "Try to be less
than 1 second behind the current OpenFlow layer's forwarding rules".


More information about the dev mailing list