On 6 April 2018 at 10:44, Ben Pfaff <b...@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:b...@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 <joestrin...@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". Cheers, Joe _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
