| Message ID | 9FD6F4CD-4F41-4350-B217-4EFE40E347E2@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev] Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | fail | test: fail |
Hi Pravin, Any update feedback on this? //Eelco On 15 Nov 2022, at 17:16, Eelco Chaudron wrote: > 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);
On 11/15/22 17:16, Eelco Chaudron wrote: > 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 ;) I'd say that dynamic allocation should be fine. We do alloc other things in the same function, and I don't immediately see another simple way to fix the problem without heavily re-working the logic. My 2c. Best regards, Ilya Maximets. > > 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); >
Ilya Maximets <i.maximets@ovn.org> writes: > On 11/15/22 17:16, Eelco Chaudron wrote: >> 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 ;) > > I'd say that dynamic allocation should be fine. > We do alloc other things in the same function, and > I don't immediately see another simple way to fix > the problem without heavily re-working the logic. +1 > My 2c. > Best regards, Ilya Maximets. > >> >> 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); >>
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);
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.