On 7/28/23 23:13, Mike Pattrick wrote:
> On Fri, Jul 28, 2023 at 7:48 AM Ilya Maximets <[email protected]
> <mailto:[email protected]>> 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.
The 'modified' time of a learned rule is updated in this function, because
the packet hit the learn() rule. If the learned rule didn't exist, we
would create it at this moment. Instead of re-creating, we just update
the 'modified' time. If packets will stop hitting the learn() rule, but
will continue to flow through the learned one, the learned one will be
removed when the hard timeout fires up. That is a correct behavior.
>
>
> 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.
Not really. If the learned flow is removed, we need a packet to hit a
learn() rule, which is a different rule, in order for traffic to continue
to flow. Let's say we implement a firewall that allows connections only
from A to B. So, A can initiate connection to B, but B can't initiate
connection to A. Once A sends tcp SYN, we learn a rule that allows traffic
from B to A on these particular ports. Now B can send traffic back.
Once the learned flow is removed, A will have to establish a new connection
in order to re-learn the B->A rule. So, not just an upcall. May not be a
best example, but should work as an illustration.
> The alternative
> is we re-install a flow that may be different then the flow that the user
> actually
> wants installed.
If the flow is modified, it means that there was a traffic on a learn()
action and we have to learn that specific rule. If it was already removed,
we have to reinstate it. In general, the logic in the current OVS code
seems correct in this particular place.
>
> 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.
The problem here is that we can't define which one is 'correct'.
It's a race where both values are correct, but you like one better
for some reason. New rules are only learned if the parent datapath
flow that has a learn() action seen packets after the previous stats
check.
What you're describing is that you have a single rule with a learn
action that is capable of re-writing the same learned rule depending
on what kind of packet actually hits it. Is that correct?
I would consider this kind of setup as fairly fragile and would not
recommend using it in general.
However, the scenario is likely like this:
1. Upcall on a rule with learn() action.
New datapath flow (1) is installed. This flow has a match on
all the fields used in a learn action and ukey contains the
translation cache with a rule that it produces.
The new learned rule is installed from this.
2. Upcall with a different packet that doesn't match the first
datapath flow (1), but on the same rule with a learn() action.
New datapath flow (2) is installed. This flow has a match on
all the fields used in a learn action (they are different from
the first one, since we have an upcall) and ukey contains the
translation cache with a rule that it produces. Let's say the
learned rule has the same match, but different actions.
This overwrites the previously installed learned rule.
3. Both datapath flows (1) and (2) get some traffic.
4. Revalidation starts.
5. Revalidator A dumps datapath flow (1).
Revalidator B dumps datapath flow (2).
6. Revalidators A and B are trying to revalidate these datapath flows
at the same time. Both datapath flows had traffic since the
last revalidation. Both revalidators are trying to update the
learned rule, which is the same for both of them.
7. One of them does that first, the other one ends up with a rule in a
REMOVED state, since it was just replaced by the other revalidator.
There is no correct or incorrect rule in this situation. Both
of them are correct, because both datapath flows have seen packets
since the previous revalidation. We can't guarantee which one will
end up being written the last and therefore will take effect.
if we refuse to reinstate the REMOVED rule, we still can't say which
one will be active in the end.
What can resolve the race is the next revalidation cycle.
Once one of the datapath flows will stop receiving traffic, there
should no longer be a race condition and the next revalidation will
set the "correct" rule, i.e. the rule that corresponds to a learning
datapath flow that seen some packets last. But until both flows have
traffic, there is no "correct" rule and the result will be random,
regardless of what we choose to do.
One issue that I see in the current code is that it looks like the table
version is not getting updated on learned flow refresh and hence the next
revalidation may not actually be triggered. In this case the random
result may stay for much longer than we would like.
The suspicious code (and the potential fix) is the following:
@@ -5548,22 +5558,15 @@ ofproto_flow_mod_learn_revert(struct ofproto_flow_mod
*ofm)
}
enum ofperr
-ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm,
- struct ofproto *orig_ofproto)
+ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm)
OVS_REQUIRES(ofproto_mutex)
{
struct rule *rule = rule_collection_rules(&ofm->new_rules)[0];
enum ofperr error = 0;
- /* If learning on a different bridge, must bump its version
- * number and flush connmgr afterwards. */
- if (rule->ofproto != orig_ofproto) {
- ofproto_bump_tables_version(rule->ofproto);
- }
+ ofproto_bump_tables_version(rule->ofproto);
error = ofproto_flow_mod_finish(rule->ofproto, ofm, NULL);
- if (rule->ofproto != orig_ofproto) {
- ofmonitor_flush(rule->ofproto->connmgr);
- }
+ ofmonitor_flush(rule->ofproto->connmgr);
return error;
}
---
We should actually bump the version and request a revalidation in
any case, not only when the change is in a different bridge.
Could you try that in your setup?
The issue appears to come from the commit:
1f4a89336682 ("ofproto: Refactor packet_out handling.")
There is a slight chance of launching ourselves into an infinite
revlidation loop, I guess. Especially if both learning datapath
flows will continue to receive the traffic, but there is a minimal
revalidation interval, IIRC. Not sure if it applies in this case.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
