On Fri, Jul 28, 2023 at 7:48 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
> On 7/20/23 17:06, Mike Pattrick wrote: > > When the a revalidator thread is updating statistics for an XC_LEARN > > xcache entry in xlate_push_stats_entry it uses ofproto_flow_mod_learn. > > The revalidator will update stats for rules even if they are in a > > removed state or marked as invisible. However, ofproto_flow_mod_learn > > will detect if a flow has been removed and re-add it in that case. This > > can result in an old learn action replacing the new learn action that > > had replaced it in the first place. > > > > This change adds a new parameter to ofproto_flow_mod_learn allowing the > > caller to specify an action to take if temp_rule is removed. If it is > > removed and the caller is xlate_push_stats_entry, then a new rule will > > not be replaced. > > This is a slightly unrelated point, but while reviewing some code and test results I think I found a different issue. rule_expire() considers rule modified time for hard_timeout and the man page says hard_timeout "Causes the flow to expire after the given number of seconds, regardless of activity", however, modified time is updated in ofproto_flow_mod_learn_refresh() even if the rule isn't changed. Due to the current behaviour, I don't think hard_timeout could actually ever trigger on a learned action if an idle timeout is set or if traffic is ongoing. > Hi, Mike. That's an interesting issue, though I'm not sure that is a > correct way to fix it. > > It is important to re-install removed learned flows during stats update. > The logic is the following, AFAIU: > > 1. Packet hits the OF rule (1) with a learning action. > 2. Handler translates OF and creates a datapath flow for it, creating > a learned rule (2) in the process and filling the translation cache. > 3. Datapath flow for the rule (1) is installed into the datapath. > 4. Time goes by and timeout on the learned rule (2) hits. Learned rule > is removed now. > 5. A packet hits the datapath flow for rule (1). > 6. Revalidator updates flow stats for the rule (1). > 7. Since the packet hit the learning rule (1), the rule (2) has to be > added back. > > IIUC, the step 7 will not happen with the change proposed in this patch. > Flow will not be re-translated, so the only way to re-install it is > to execute side effects when we see stats update on a parent datapath > flow in the revalidator. > > One potential way to avoid breaking this scenario is to check the > removal reason, but it's not bulletproof either as we do still need > to reinstate the flow even if it was manually deleted or evicted. > I added a log to check the removal reason in the XC_LEARN stats update, and the reason always seems to be eviction. I wasn't able to catch an idle or hard removal in that function, though I'm sure I just wasn't winning the race. But in this example if we hit 7, and don't re-add the rule, what is the real penalty? The next packet will have to upcall and we would process the learn action from scratch, but that may be what the user wants as indicated by setting a timeout. The alternative is we re-install a flow that may be different then the flow that the user actually wants installed. Isn't there a risk of different revalidator threads going out of sync with different versions of a learned rule in their xcache? > It also seems like we may hold the reference to this deleted rule for a > long time in the cache until the parent datapath flow is deleted. > > > What is triggering the deletion of the learned rule in your case? > Is it deletion or modification of the parent rule? And does it have > 'delete_learned' flag? > In the case that I was debugging, the parent learn action was not deleted. Instead, new traffic from a different port was supposed to cause the learn action to modify the existing rule. But instead we see the rule modified with the correct information, and then immediately reverted with old stale information. > > Best regards, Ilya Maximets. > > Thank you, Mike _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev