* Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct.
@ 2022-11-15 16:16 Eelco Chaudron
2022-11-23 10:56 ` Eelco Chaudron
2022-11-25 14:53 ` Ilya Maximets
0 siblings, 2 replies; 4+ messages in thread
From: Eelco Chaudron @ 2022-11-15 16:16 UTC (permalink / raw)
To: Pravin B Shelar, netdev; +Cc: Ilya Maximets, Aaron Conole, dev
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);
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct.
2022-11-15 16:16 Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct Eelco Chaudron
@ 2022-11-23 10:56 ` Eelco Chaudron
2022-11-25 14:53 ` Ilya Maximets
1 sibling, 0 replies; 4+ messages in thread
From: Eelco Chaudron @ 2022-11-23 10:56 UTC (permalink / raw)
To: Pravin B Shelar, netdev; +Cc: Ilya Maximets, Aaron Conole, dev
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);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct.
2022-11-15 16:16 Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct Eelco Chaudron
2022-11-23 10:56 ` Eelco Chaudron
@ 2022-11-25 14:53 ` Ilya Maximets
2022-11-29 14:05 ` Aaron Conole
1 sibling, 1 reply; 4+ messages in thread
From: Ilya Maximets @ 2022-11-25 14:53 UTC (permalink / raw)
To: Eelco Chaudron, Pravin B Shelar, netdev; +Cc: i.maximets, Aaron Conole, dev
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);
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct.
2022-11-25 14:53 ` Ilya Maximets
@ 2022-11-29 14:05 ` Aaron Conole
0 siblings, 0 replies; 4+ messages in thread
From: Aaron Conole @ 2022-11-29 14:05 UTC (permalink / raw)
To: Ilya Maximets; +Cc: Eelco Chaudron, Pravin B Shelar, netdev, dev
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);
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-29 14:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-15 16:16 Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct Eelco Chaudron
2022-11-23 10:56 ` Eelco Chaudron
2022-11-25 14:53 ` Ilya Maximets
2022-11-29 14:05 ` Aaron Conole
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).