| Message ID | 20191016083714.104447-1-roid@mellanox.com |
|---|---|
| State | Accepted |
| Commit | d0fbb09f907b9a5c02cf8f9b04ed03bee3516504 |
| Headers | show |
| Series | [ovs-dev] tc: Limit the max action number to 16 | expand |
Bleep bloop. Greetings Roi Dayan, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 87 characters long (recommended limit is 79)
#57 FILE: lib/tc.c:1481:
VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM);
Lines checked: 85, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: > From: Chris Mi <chrism@mellanox.com> > > Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. > But net sched api has a limit of 4K message size which is not enough > for 32 actions when echo flag is set. > > After a lot of testing, we find that 16 actions is a reasonable number. > So in this commit, we introduced a new define to limit the max actions. > > Fixes: 0c70132cd288("tc: Make the actions order consistent") > Signed-off-by: Chris Mi <chrism@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> Hi Chris, I'm unclear on what problem is this patch solving. > --- > lib/netdev-offload-tc.c | 4 ++-- > lib/tc.c | 6 +++--- > lib/tc.h | 4 +++- > 3 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 6814390eab2f..f6d1abb2e695 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -1360,8 +1360,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, > } > > NL_ATTR_FOR_EACH(nla, left, actions, actions_len) { > - if (flower.action_count >= TCA_ACT_MAX_PRIO) { > - VLOG_DBG_RL(&rl, "Can only support %d actions", flower.action_count); > + if (flower.action_count >= TCA_ACT_MAX_NUM) { > + VLOG_DBG_RL(&rl, "Can only support %d actions", TCA_ACT_MAX_NUM); > return EOPNOTSUPP; > } > action = &flower.actions[flower.action_count]; > diff --git a/lib/tc.c b/lib/tc.c > index 316a0ee5dc7c..d5487097d8ec 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1458,7 +1458,7 @@ static int > nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) > { > const struct nlattr *actions = attrs[TCA_FLOWER_ACT]; > - static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO + 1] = {}; > + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; > struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; > const int max_size = ARRAY_SIZE(actions_orders_policy); > > @@ -1477,8 +1477,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) > if (actions_orders[i]) { > int err; > > - if (flower->action_count >= TCA_ACT_MAX_PRIO) { > - VLOG_DBG_RL(&error_rl, "Can only support %d actions", flower->action_count); > + if (flower->action_count >= TCA_ACT_MAX_NUM) { > + VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); > return EOPNOTSUPP; > } > err = nl_parse_single_action(actions_orders[i], flower); > diff --git a/lib/tc.h b/lib/tc.h > index f4213579a2fd..f4073c6c47e6 100644 > --- a/lib/tc.h > +++ b/lib/tc.h > @@ -208,6 +208,8 @@ enum tc_offloaded_state { > TC_OFFLOADED_STATE_NOT_IN_HW, > }; > > +#define TCA_ACT_MAX_NUM 16 > + > struct tc_flower { > uint32_t handle; > uint32_t prio; > @@ -216,7 +218,7 @@ struct tc_flower { > struct tc_flower_key mask; > > int action_count; > - struct tc_action actions[TCA_ACT_MAX_PRIO]; > + struct tc_action actions[TCA_ACT_MAX_NUM]; > > struct ovs_flow_stats stats; > uint64_t lastused; > -- > 2.8.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2019-10-16 2:40 PM, Simon Horman wrote: > On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: >> From: Chris Mi <chrism@mellanox.com> >> >> Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. >> But net sched api has a limit of 4K message size which is not enough >> for 32 actions when echo flag is set. >> >> After a lot of testing, we find that 16 actions is a reasonable number. >> So in this commit, we introduced a new define to limit the max actions. >> >> Fixes: 0c70132cd288("tc: Make the actions order consistent") >> Signed-off-by: Chris Mi <chrism@mellanox.com> >> Reviewed-by: Roi Dayan <roid@mellanox.com> > > Hi Chris, > > I'm unclear on what problem is this patch solving. Hi Simon, I can help with the answer here. When ovs send netlink msg to tc to add a filter we also add the echo flag to get a reply data. this reply can be big as it contains everything from the request and if it pass the 4K size then the kernel will just not fill/send it but the return status of the tc command is still 0 for success. In ovs we use that reply to get back the handle id on success but for this case will fail. To make sure this ack reply always exists for successful tc calls we limit the number of actions here to avoid reaching the 4K size limit. Thanks, Roi > >> --- >> lib/netdev-offload-tc.c | 4 ++-- >> lib/tc.c | 6 +++--- >> lib/tc.h | 4 +++- >> 3 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 6814390eab2f..f6d1abb2e695 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -1360,8 +1360,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, >> } >> >> NL_ATTR_FOR_EACH(nla, left, actions, actions_len) { >> - if (flower.action_count >= TCA_ACT_MAX_PRIO) { >> - VLOG_DBG_RL(&rl, "Can only support %d actions", flower.action_count); >> + if (flower.action_count >= TCA_ACT_MAX_NUM) { >> + VLOG_DBG_RL(&rl, "Can only support %d actions", TCA_ACT_MAX_NUM); >> return EOPNOTSUPP; >> } >> action = &flower.actions[flower.action_count]; >> diff --git a/lib/tc.c b/lib/tc.c >> index 316a0ee5dc7c..d5487097d8ec 100644 >> --- a/lib/tc.c >> +++ b/lib/tc.c >> @@ -1458,7 +1458,7 @@ static int >> nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) >> { >> const struct nlattr *actions = attrs[TCA_FLOWER_ACT]; >> - static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO + 1] = {}; >> + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; >> struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; >> const int max_size = ARRAY_SIZE(actions_orders_policy); >> >> @@ -1477,8 +1477,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) >> if (actions_orders[i]) { >> int err; >> >> - if (flower->action_count >= TCA_ACT_MAX_PRIO) { >> - VLOG_DBG_RL(&error_rl, "Can only support %d actions", flower->action_count); >> + if (flower->action_count >= TCA_ACT_MAX_NUM) { >> + VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); >> return EOPNOTSUPP; >> } >> err = nl_parse_single_action(actions_orders[i], flower); >> diff --git a/lib/tc.h b/lib/tc.h >> index f4213579a2fd..f4073c6c47e6 100644 >> --- a/lib/tc.h >> +++ b/lib/tc.h >> @@ -208,6 +208,8 @@ enum tc_offloaded_state { >> TC_OFFLOADED_STATE_NOT_IN_HW, >> }; >> >> +#define TCA_ACT_MAX_NUM 16 >> + >> struct tc_flower { >> uint32_t handle; >> uint32_t prio; >> @@ -216,7 +218,7 @@ struct tc_flower { >> struct tc_flower_key mask; >> >> int action_count; >> - struct tc_action actions[TCA_ACT_MAX_PRIO]; >> + struct tc_action actions[TCA_ACT_MAX_NUM]; >> >> struct ovs_flow_stats stats; >> uint64_t lastused; >> -- >> 2.8.4 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Croid%40mellanox.com%7C9097cc90c5ac40952b3f08d7522da0e1%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637068228233302023&sdata=vEyT6W3%2Fd%2B3gcyygwFwqXJxQHPsxC7gyzmNTs8FquVw%3D&reserved=0 >>
On Wed, Oct 16, 2019 at 11:53:52AM +0000, Roi Dayan wrote: > > > On 2019-10-16 2:40 PM, Simon Horman wrote: > > On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: > >> From: Chris Mi <chrism@mellanox.com> > >> > >> Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. > >> But net sched api has a limit of 4K message size which is not enough > >> for 32 actions when echo flag is set. > >> > >> After a lot of testing, we find that 16 actions is a reasonable number. > >> So in this commit, we introduced a new define to limit the max actions. > >> > >> Fixes: 0c70132cd288("tc: Make the actions order consistent") > >> Signed-off-by: Chris Mi <chrism@mellanox.com> > >> Reviewed-by: Roi Dayan <roid@mellanox.com> > > > > Hi Chris, > > > > I'm unclear on what problem is this patch solving. > > Hi Simon, > > I can help with the answer here. > When ovs send netlink msg to tc to add a filter we also add > the echo flag to get a reply data. this reply can be big as > it contains everything from the request and if it pass the 4K > size then the kernel will just not fill/send it but the return > status of the tc command is still 0 for success. > > In ovs we use that reply to get back the handle id on success but > for this case will fail. > To make sure this ack reply always exists for successful tc calls > we limit the number of actions here to avoid reaching the 4K size limit. Thanks, It sounds like it would be good to develop a mechanism where the information required can be retrieved in a less verbose manner, thus allowing a higher limit. But I do agree that this resolves a problem and I have applied it to master. Let me know if you think it is also appropriate for older branches. Kind regards, Simon > > Thanks, > Roi > > > > >> --- > >> lib/netdev-offload-tc.c | 4 ++-- > >> lib/tc.c | 6 +++--- > >> lib/tc.h | 4 +++- > >> 3 files changed, 8 insertions(+), 6 deletions(-) > >> > >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > >> index 6814390eab2f..f6d1abb2e695 100644 > >> --- a/lib/netdev-offload-tc.c > >> +++ b/lib/netdev-offload-tc.c > >> @@ -1360,8 +1360,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, > >> } > >> > >> NL_ATTR_FOR_EACH(nla, left, actions, actions_len) { > >> - if (flower.action_count >= TCA_ACT_MAX_PRIO) { > >> - VLOG_DBG_RL(&rl, "Can only support %d actions", flower.action_count); > >> + if (flower.action_count >= TCA_ACT_MAX_NUM) { > >> + VLOG_DBG_RL(&rl, "Can only support %d actions", TCA_ACT_MAX_NUM); > >> return EOPNOTSUPP; > >> } > >> action = &flower.actions[flower.action_count]; > >> diff --git a/lib/tc.c b/lib/tc.c > >> index 316a0ee5dc7c..d5487097d8ec 100644 > >> --- a/lib/tc.c > >> +++ b/lib/tc.c > >> @@ -1458,7 +1458,7 @@ static int > >> nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) > >> { > >> const struct nlattr *actions = attrs[TCA_FLOWER_ACT]; > >> - static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO + 1] = {}; > >> + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; > >> struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; > >> const int max_size = ARRAY_SIZE(actions_orders_policy); > >> > >> @@ -1477,8 +1477,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) > >> if (actions_orders[i]) { > >> int err; > >> > >> - if (flower->action_count >= TCA_ACT_MAX_PRIO) { > >> - VLOG_DBG_RL(&error_rl, "Can only support %d actions", flower->action_count); > >> + if (flower->action_count >= TCA_ACT_MAX_NUM) { > >> + VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); > >> return EOPNOTSUPP; > >> } > >> err = nl_parse_single_action(actions_orders[i], flower); > >> diff --git a/lib/tc.h b/lib/tc.h > >> index f4213579a2fd..f4073c6c47e6 100644 > >> --- a/lib/tc.h > >> +++ b/lib/tc.h > >> @@ -208,6 +208,8 @@ enum tc_offloaded_state { > >> TC_OFFLOADED_STATE_NOT_IN_HW, > >> }; > >> > >> +#define TCA_ACT_MAX_NUM 16 > >> + > >> struct tc_flower { > >> uint32_t handle; > >> uint32_t prio; > >> @@ -216,7 +218,7 @@ struct tc_flower { > >> struct tc_flower_key mask; > >> > >> int action_count; > >> - struct tc_action actions[TCA_ACT_MAX_PRIO]; > >> + struct tc_action actions[TCA_ACT_MAX_NUM]; > >> > >> struct ovs_flow_stats stats; > >> uint64_t lastused; > >> -- > >> 2.8.4 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Croid%40mellanox.com%7C9097cc90c5ac40952b3f08d7522da0e1%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637068228233302023&sdata=vEyT6W3%2Fd%2B3gcyygwFwqXJxQHPsxC7gyzmNTs8FquVw%3D&reserved=0 > >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2019-10-18 1:00 PM, Simon Horman wrote: > On Wed, Oct 16, 2019 at 11:53:52AM +0000, Roi Dayan wrote: >> >> >> On 2019-10-16 2:40 PM, Simon Horman wrote: >>> On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: >>>> From: Chris Mi <chrism@mellanox.com> >>>> >>>> Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. >>>> But net sched api has a limit of 4K message size which is not enough >>>> for 32 actions when echo flag is set. >>>> >>>> After a lot of testing, we find that 16 actions is a reasonable number. >>>> So in this commit, we introduced a new define to limit the max actions. >>>> >>>> Fixes: 0c70132cd288("tc: Make the actions order consistent") >>>> Signed-off-by: Chris Mi <chrism@mellanox.com> >>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>> >>> Hi Chris, >>> >>> I'm unclear on what problem is this patch solving. >> >> Hi Simon, >> >> I can help with the answer here. >> When ovs send netlink msg to tc to add a filter we also add >> the echo flag to get a reply data. this reply can be big as >> it contains everything from the request and if it pass the 4K >> size then the kernel will just not fill/send it but the return >> status of the tc command is still 0 for success. >> >> In ovs we use that reply to get back the handle id on success but >> for this case will fail. >> To make sure this ack reply always exists for successful tc calls >> we limit the number of actions here to avoid reaching the 4K size limit. > > Thanks, > > It sounds like it would be good to develop a mechanism where > the information required can be retrieved in a less verbose manner, > thus allowing a higher limit. > > But I do agree that this resolves a problem and I have applied it to > master. Let me know if you think it is also appropriate for older branches. > > Kind regards, > Simon > thanks Simon. this patch can be applied also to branches 2.10, 2.11, 2.12. >> >> Thanks, >> Roi >> >>> >>>> --- >>>> lib/netdev-offload-tc.c | 4 ++-- >>>> lib/tc.c | 6 +++--- >>>> lib/tc.h | 4 +++- >>>> 3 files changed, 8 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>> index 6814390eab2f..f6d1abb2e695 100644 >>>> --- a/lib/netdev-offload-tc.c >>>> +++ b/lib/netdev-offload-tc.c >>>> @@ -1360,8 +1360,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, >>>> } >>>> >>>> NL_ATTR_FOR_EACH(nla, left, actions, actions_len) { >>>> - if (flower.action_count >= TCA_ACT_MAX_PRIO) { >>>> - VLOG_DBG_RL(&rl, "Can only support %d actions", flower.action_count); >>>> + if (flower.action_count >= TCA_ACT_MAX_NUM) { >>>> + VLOG_DBG_RL(&rl, "Can only support %d actions", TCA_ACT_MAX_NUM); >>>> return EOPNOTSUPP; >>>> } >>>> action = &flower.actions[flower.action_count]; >>>> diff --git a/lib/tc.c b/lib/tc.c >>>> index 316a0ee5dc7c..d5487097d8ec 100644 >>>> --- a/lib/tc.c >>>> +++ b/lib/tc.c >>>> @@ -1458,7 +1458,7 @@ static int >>>> nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) >>>> { >>>> const struct nlattr *actions = attrs[TCA_FLOWER_ACT]; >>>> - static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO + 1] = {}; >>>> + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; >>>> struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; >>>> const int max_size = ARRAY_SIZE(actions_orders_policy); >>>> >>>> @@ -1477,8 +1477,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) >>>> if (actions_orders[i]) { >>>> int err; >>>> >>>> - if (flower->action_count >= TCA_ACT_MAX_PRIO) { >>>> - VLOG_DBG_RL(&error_rl, "Can only support %d actions", flower->action_count); >>>> + if (flower->action_count >= TCA_ACT_MAX_NUM) { >>>> + VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); >>>> return EOPNOTSUPP; >>>> } >>>> err = nl_parse_single_action(actions_orders[i], flower); >>>> diff --git a/lib/tc.h b/lib/tc.h >>>> index f4213579a2fd..f4073c6c47e6 100644 >>>> --- a/lib/tc.h >>>> +++ b/lib/tc.h >>>> @@ -208,6 +208,8 @@ enum tc_offloaded_state { >>>> TC_OFFLOADED_STATE_NOT_IN_HW, >>>> }; >>>> >>>> +#define TCA_ACT_MAX_NUM 16 >>>> + >>>> struct tc_flower { >>>> uint32_t handle; >>>> uint32_t prio; >>>> @@ -216,7 +218,7 @@ struct tc_flower { >>>> struct tc_flower_key mask; >>>> >>>> int action_count; >>>> - struct tc_action actions[TCA_ACT_MAX_PRIO]; >>>> + struct tc_action actions[TCA_ACT_MAX_NUM]; >>>> >>>> struct ovs_flow_stats stats; >>>> uint64_t lastused; >>>> -- >>>> 2.8.4 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Croid%40mellanox.com%7C0006a61c42a8429f0b3408d753b21362%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637069896613483487&sdata=3yAqcavjPoih%2B6aBqZ5lIZY3zv8mBqwJNnjzBIMhnp8%3D&reserved=0 >>>> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Croid%40mellanox.com%7C0006a61c42a8429f0b3408d753b21362%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637069896613483487&sdata=3yAqcavjPoih%2B6aBqZ5lIZY3zv8mBqwJNnjzBIMhnp8%3D&reserved=0 >>
On Mon, Oct 21, 2019 at 07:01:38AM +0000, Roi Dayan wrote: > > > On 2019-10-18 1:00 PM, Simon Horman wrote: > > On Wed, Oct 16, 2019 at 11:53:52AM +0000, Roi Dayan wrote: > >> > >> > >> On 2019-10-16 2:40 PM, Simon Horman wrote: > >>> On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: > >>>> From: Chris Mi <chrism@mellanox.com> > >>>> > >>>> Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. > >>>> But net sched api has a limit of 4K message size which is not enough > >>>> for 32 actions when echo flag is set. > >>>> > >>>> After a lot of testing, we find that 16 actions is a reasonable number. > >>>> So in this commit, we introduced a new define to limit the max actions. > >>>> > >>>> Fixes: 0c70132cd288("tc: Make the actions order consistent") > >>>> Signed-off-by: Chris Mi <chrism@mellanox.com> > >>>> Reviewed-by: Roi Dayan <roid@mellanox.com> > >>> > >>> Hi Chris, > >>> > >>> I'm unclear on what problem is this patch solving. > >> > >> Hi Simon, > >> > >> I can help with the answer here. > >> When ovs send netlink msg to tc to add a filter we also add > >> the echo flag to get a reply data. this reply can be big as > >> it contains everything from the request and if it pass the 4K > >> size then the kernel will just not fill/send it but the return > >> status of the tc command is still 0 for success. > >> > >> In ovs we use that reply to get back the handle id on success but > >> for this case will fail. > >> To make sure this ack reply always exists for successful tc calls > >> we limit the number of actions here to avoid reaching the 4K size limit. > > > > Thanks, > > > > It sounds like it would be good to develop a mechanism where > > the information required can be retrieved in a less verbose manner, > > thus allowing a higher limit. > > > > But I do agree that this resolves a problem and I have applied it to > > master. Let me know if you think it is also appropriate for older branches. > > > > Kind regards, > > Simon > > > > thanks Simon. > this patch can be applied also to branches 2.10, 2.11, 2.12. Thanks, I have pushed a backport to branch-2.12. And I intend to do likewise for branch-2.11 if travis-ci passes. https://travis-ci.org/horms2/ovs/builds/602211621 I am however holding back on branch-2.10 as it seems broken with respect to travis-ci. And I do not like to add patches to broken branches. I am trying to investigate the cause of that breakage. I would also welcome any help in this area. Kind regards, Simon
On Thu, Oct 24, 2019 at 10:43:00AM +0200, Simon Horman wrote: > On Mon, Oct 21, 2019 at 07:01:38AM +0000, Roi Dayan wrote: > > > > > > On 2019-10-18 1:00 PM, Simon Horman wrote: > > > On Wed, Oct 16, 2019 at 11:53:52AM +0000, Roi Dayan wrote: > > >> > > >> > > >> On 2019-10-16 2:40 PM, Simon Horman wrote: > > >>> On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: > > >>>> From: Chris Mi <chrism@mellanox.com> > > >>>> > > >>>> Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. > > >>>> But net sched api has a limit of 4K message size which is not enough > > >>>> for 32 actions when echo flag is set. > > >>>> > > >>>> After a lot of testing, we find that 16 actions is a reasonable number. > > >>>> So in this commit, we introduced a new define to limit the max actions. > > >>>> > > >>>> Fixes: 0c70132cd288("tc: Make the actions order consistent") > > >>>> Signed-off-by: Chris Mi <chrism@mellanox.com> > > >>>> Reviewed-by: Roi Dayan <roid@mellanox.com> > > >>> > > >>> Hi Chris, > > >>> > > >>> I'm unclear on what problem is this patch solving. > > >> > > >> Hi Simon, > > >> > > >> I can help with the answer here. > > >> When ovs send netlink msg to tc to add a filter we also add > > >> the echo flag to get a reply data. this reply can be big as > > >> it contains everything from the request and if it pass the 4K > > >> size then the kernel will just not fill/send it but the return > > >> status of the tc command is still 0 for success. > > >> > > >> In ovs we use that reply to get back the handle id on success but > > >> for this case will fail. > > >> To make sure this ack reply always exists for successful tc calls > > >> we limit the number of actions here to avoid reaching the 4K size limit. > > > > > > Thanks, > > > > > > It sounds like it would be good to develop a mechanism where > > > the information required can be retrieved in a less verbose manner, > > > thus allowing a higher limit. > > > > > > But I do agree that this resolves a problem and I have applied it to > > > master. Let me know if you think it is also appropriate for older branches. > > > > > > Kind regards, > > > Simon > > > > > > > thanks Simon. > > this patch can be applied also to branches 2.10, 2.11, 2.12. > > Thanks, > > I have pushed a backport to branch-2.12. > And I intend to do likewise for branch-2.11 if travis-ci passes. I have now pushed this patch to branch-2.11. > > I am however holding back on branch-2.10 as it seems broken > with respect to travis-ci. And I do not like to add patches > to broken branches. > > I am trying to investigate the cause of that breakage. > I would also welcome any help in this area. I have proposed a fix for this problem. https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/363868.html
On Fri, Oct 25, 2019 at 12:19:22PM +0200, Simon Horman wrote: > On Thu, Oct 24, 2019 at 10:43:00AM +0200, Simon Horman wrote: > > On Mon, Oct 21, 2019 at 07:01:38AM +0000, Roi Dayan wrote: > > > > > > > > > On 2019-10-18 1:00 PM, Simon Horman wrote: > > > > On Wed, Oct 16, 2019 at 11:53:52AM +0000, Roi Dayan wrote: > > > >> > > > >> > > > >> On 2019-10-16 2:40 PM, Simon Horman wrote: > > > >>> On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: > > > >>>> From: Chris Mi <chrism@mellanox.com> > > > >>>> > > > >>>> Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. > > > >>>> But net sched api has a limit of 4K message size which is not enough > > > >>>> for 32 actions when echo flag is set. > > > >>>> > > > >>>> After a lot of testing, we find that 16 actions is a reasonable number. > > > >>>> So in this commit, we introduced a new define to limit the max actions. > > > >>>> > > > >>>> Fixes: 0c70132cd288("tc: Make the actions order consistent") > > > >>>> Signed-off-by: Chris Mi <chrism@mellanox.com> > > > >>>> Reviewed-by: Roi Dayan <roid@mellanox.com> > > > >>> > > > >>> Hi Chris, > > > >>> > > > >>> I'm unclear on what problem is this patch solving. > > > >> > > > >> Hi Simon, > > > >> > > > >> I can help with the answer here. > > > >> When ovs send netlink msg to tc to add a filter we also add > > > >> the echo flag to get a reply data. this reply can be big as > > > >> it contains everything from the request and if it pass the 4K > > > >> size then the kernel will just not fill/send it but the return > > > >> status of the tc command is still 0 for success. > > > >> > > > >> In ovs we use that reply to get back the handle id on success but > > > >> for this case will fail. > > > >> To make sure this ack reply always exists for successful tc calls > > > >> we limit the number of actions here to avoid reaching the 4K size limit. > > > > > > > > Thanks, > > > > > > > > It sounds like it would be good to develop a mechanism where > > > > the information required can be retrieved in a less verbose manner, > > > > thus allowing a higher limit. > > > > > > > > But I do agree that this resolves a problem and I have applied it to > > > > master. Let me know if you think it is also appropriate for older branches. > > > > > > > > Kind regards, > > > > Simon > > > > > > > > > > thanks Simon. > > > this patch can be applied also to branches 2.10, 2.11, 2.12. > > > > Thanks, > > > > I have pushed a backport to branch-2.12. > > And I intend to do likewise for branch-2.11 if travis-ci passes. > > I have now pushed this patch to branch-2.11. > > > > > I am however holding back on branch-2.10 as it seems broken > > with respect to travis-ci. And I do not like to add patches > > to broken branches. > > > > I am trying to investigate the cause of that breakage. > > I would also welcome any help in this area. > > I have proposed a fix for this problem. > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/363868.html That fix was merged by Ben and now that branch-2.10 is travis-ci clean I have gone ahead and applied this patch there.
On 2019-10-28 10:15 AM, Simon Horman wrote: > On Fri, Oct 25, 2019 at 12:19:22PM +0200, Simon Horman wrote: >> On Thu, Oct 24, 2019 at 10:43:00AM +0200, Simon Horman wrote: >>> On Mon, Oct 21, 2019 at 07:01:38AM +0000, Roi Dayan wrote: >>>> >>>> >>>> On 2019-10-18 1:00 PM, Simon Horman wrote: >>>>> On Wed, Oct 16, 2019 at 11:53:52AM +0000, Roi Dayan wrote: >>>>>> >>>>>> >>>>>> On 2019-10-16 2:40 PM, Simon Horman wrote: >>>>>>> On Wed, Oct 16, 2019 at 11:37:14AM +0300, Roi Dayan wrote: >>>>>>>> From: Chris Mi <chrism@mellanox.com> >>>>>>>> >>>>>>>> Currently, ovs supports to offload max TCA_ACT_MAX_PRIO(32) actions. >>>>>>>> But net sched api has a limit of 4K message size which is not enough >>>>>>>> for 32 actions when echo flag is set. >>>>>>>> >>>>>>>> After a lot of testing, we find that 16 actions is a reasonable number. >>>>>>>> So in this commit, we introduced a new define to limit the max actions. >>>>>>>> >>>>>>>> Fixes: 0c70132cd288("tc: Make the actions order consistent") >>>>>>>> Signed-off-by: Chris Mi <chrism@mellanox.com> >>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>>>>> >>>>>>> Hi Chris, >>>>>>> >>>>>>> I'm unclear on what problem is this patch solving. >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> I can help with the answer here. >>>>>> When ovs send netlink msg to tc to add a filter we also add >>>>>> the echo flag to get a reply data. this reply can be big as >>>>>> it contains everything from the request and if it pass the 4K >>>>>> size then the kernel will just not fill/send it but the return >>>>>> status of the tc command is still 0 for success. >>>>>> >>>>>> In ovs we use that reply to get back the handle id on success but >>>>>> for this case will fail. >>>>>> To make sure this ack reply always exists for successful tc calls >>>>>> we limit the number of actions here to avoid reaching the 4K size limit. >>>>> >>>>> Thanks, >>>>> >>>>> It sounds like it would be good to develop a mechanism where >>>>> the information required can be retrieved in a less verbose manner, >>>>> thus allowing a higher limit. >>>>> >>>>> But I do agree that this resolves a problem and I have applied it to >>>>> master. Let me know if you think it is also appropriate for older branches. >>>>> >>>>> Kind regards, >>>>> Simon >>>>> >>>> >>>> thanks Simon. >>>> this patch can be applied also to branches 2.10, 2.11, 2.12. >>> >>> Thanks, >>> >>> I have pushed a backport to branch-2.12. >>> And I intend to do likewise for branch-2.11 if travis-ci passes. >> >> I have now pushed this patch to branch-2.11. >> >>> >>> I am however holding back on branch-2.10 as it seems broken >>> with respect to travis-ci. And I do not like to add patches >>> to broken branches. >>> >>> I am trying to investigate the cause of that breakage. >>> I would also welcome any help in this area. >> >> I have proposed a fix for this problem. >> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2019-October%2F363868.html&data=02%7C01%7Croid%40mellanox.com%7Caca4498333aa467cae6c08d75b7ef3d1%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637078473125610665&sdata=p4oai3sC8F4FHfFrvfv7q0XAzk6%2BY%2Bs9cD736XgLvdg%3D&reserved=0 > > That fix was merged by Ben and now that branch-2.10 is travis-ci clean > I have gone ahead and applied this patch there. > thanks!
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 6814390eab2f..f6d1abb2e695 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -1360,8 +1360,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, } NL_ATTR_FOR_EACH(nla, left, actions, actions_len) { - if (flower.action_count >= TCA_ACT_MAX_PRIO) { - VLOG_DBG_RL(&rl, "Can only support %d actions", flower.action_count); + if (flower.action_count >= TCA_ACT_MAX_NUM) { + VLOG_DBG_RL(&rl, "Can only support %d actions", TCA_ACT_MAX_NUM); return EOPNOTSUPP; } action = &flower.actions[flower.action_count]; diff --git a/lib/tc.c b/lib/tc.c index 316a0ee5dc7c..d5487097d8ec 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1458,7 +1458,7 @@ static int nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) { const struct nlattr *actions = attrs[TCA_FLOWER_ACT]; - static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO + 1] = {}; + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; const int max_size = ARRAY_SIZE(actions_orders_policy); @@ -1477,8 +1477,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower) if (actions_orders[i]) { int err; - if (flower->action_count >= TCA_ACT_MAX_PRIO) { - VLOG_DBG_RL(&error_rl, "Can only support %d actions", flower->action_count); + if (flower->action_count >= TCA_ACT_MAX_NUM) { + VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); return EOPNOTSUPP; } err = nl_parse_single_action(actions_orders[i], flower); diff --git a/lib/tc.h b/lib/tc.h index f4213579a2fd..f4073c6c47e6 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -208,6 +208,8 @@ enum tc_offloaded_state { TC_OFFLOADED_STATE_NOT_IN_HW, }; +#define TCA_ACT_MAX_NUM 16 + struct tc_flower { uint32_t handle; uint32_t prio; @@ -216,7 +218,7 @@ struct tc_flower { struct tc_flower_key mask; int action_count; - struct tc_action actions[TCA_ACT_MAX_PRIO]; + struct tc_action actions[TCA_ACT_MAX_NUM]; struct ovs_flow_stats stats; uint64_t lastused;