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 <chr...@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 <chr...@mellanox.com> > >> Reviewed-by: Roi Dayan <r...@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 > >> d...@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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev