Hi Pravin,

It looks like a previous fix you made, 190aa3e77880 ("openvswitch: Fix 
Frame-size larger than 1024 bytes warning."), is breaking stuff. With this 
change, the actual flow lookup, ovs_flow_tbl_lookup(), is done using a masked 
key, where it should be an unmasked key. This is maybe more clear if you take a 
look at the diff for the ufid addition, 74ed7ab9264c ("openvswitch: Add support 
for unique flow IDs.").

Just reverting the change gets rid of the problem, but it will re-introduce the 
larger stack size. It looks like we either have it on the stack or dynamically 
allocate it each time. Let me know if you have any other clever fix ;)

We found this after debugging some customer-specific issue. More details are in 
the following OVS patch, 
https://patchwork.ozlabs.org/project/openvswitch/list/?series=328315

Cheers,

Eelco


FYI the working revers:


   Revert "openvswitch: Fix Frame-size larger than 1024 bytes warning."

    This reverts commit 190aa3e77880a05332ea1ccb382a51285d57adb5.

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 861dfb8daf4a..660d5fdd9b28 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -948,6 +948,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
        struct sw_flow_mask mask;
        struct sk_buff *reply;
        struct datapath *dp;
+       struct sw_flow_key key;
        struct sw_flow_actions *acts;
        struct sw_flow_match match;
        u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
@@ -975,24 +976,20 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
        }

        /* Extract key. */
-       ovs_match_init(&match, &new_flow->key, false, &mask);
+       ovs_match_init(&match, &key, true, &mask);
        error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
                                  a[OVS_FLOW_ATTR_MASK], log);
        if (error)
                goto err_kfree_flow;

+       ovs_flow_mask_key(&new_flow->key, &key, true, &mask);
+
        /* Extract flow identifier. */
        error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
-                                      &new_flow->key, log);
+                                      &key, log);
        if (error)
                goto err_kfree_flow;

-       /* unmasked key is needed to match when ufid is not used. */
-       if (ovs_identifier_is_key(&new_flow->id))
-               match.key = new_flow->id.unmasked_key;
-
-       ovs_flow_mask_key(&new_flow->key, &new_flow->key, true, &mask);
-
        /* Validate actions. */
        error = ovs_nla_copy_actions(net, a[OVS_FLOW_ATTR_ACTIONS],
                                     &new_flow->key, &acts, log);
@@ -1019,7 +1016,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
        if (ovs_identifier_is_ufid(&new_flow->id))
                flow = ovs_flow_tbl_lookup_ufid(&dp->table, &new_flow->id);
        if (!flow)
-               flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
+               flow = ovs_flow_tbl_lookup(&dp->table, &key);
        if (likely(!flow)) {
                rcu_assign_pointer(new_flow->sf_acts, acts);

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to