[ovs-dev] [PATCH] tc: Limit the max action number to 16

Simon Horman simon.horman at netronome.com
Fri Oct 18 10:00:46 UTC 2019


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 at 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 at mellanox.com>
> >> Reviewed-by: Roi Dayan <roid at 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 at openvswitch.org
> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=02%7C01%7Croid%40mellanox.com%7C9097cc90c5ac40952b3f08d7522da0e1%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637068228233302023&amp;sdata=vEyT6W3%2Fd%2B3gcyygwFwqXJxQHPsxC7gyzmNTs8FquVw%3D&amp;reserved=0
> >>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 


More information about the dev mailing list