On Mon, Feb 13, 2012 at 01:59:30PM -0800, Ethan Jackson wrote: > One of the NEWS entries uses a TAB instead of spaces.
Thanks, fixed. > Following tradition, the nx_action_fin_timeout structure should have > an OFP_ASSERT of its size. Thanks, added. > > netflow_flow_clear(&facet->nf_flow); > > + facet->tcp_flags = 0; > > Could you comment briefly on this line of code? I'm having trouble > convincing myself that it's either correct or incorrect. I'm worried > about the case where facet_flush_stats() is called due to the actions > changing on the facet. If this facet has a fin_timeout in its new > actions, and it's already seen a fin, I would expect the fin_timeout > action to apply immediately? I'm not sure though. This is in facet_flush_stats(), which gets called from two contexts: facet_remove() and facet_revalidate(). It shouldn't really matter in facet_remove() since the facet is going away anyway, so the facet_revalidate() case is the interesting one. That case comes up when the facet's actions change. Now, I can see the point of view that "we saw a FIN once, so we'll make that sticky" but the point of view I've always taken is slightly different. My point of view is that the ideal that Open vSwitch is trying to reach is that every packet that goes through a flow executes the actions. That means that, when a packet with a FIN goes through the flow, it only changes the timeout that one time. If the FIN goes through when the actions are different, it has no effect, and there's no "stickiness". In reality, though, we only find out about TCP flags periodically, so we can only approximate the ideal behavior. By resetting tcp_flags when the actions change, we "forget" that the FIN was seen, which seems correct to me because in fact it hasn't been seen with the new actions. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
