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

[Roi Dayan]

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,
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
>>