[ovs-dev] [PATCH] tc: Limit the max action number to 16
Roi Dayan
roid at mellanox.com
Mon Oct 21 07:01:38 UTC 2019
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 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 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 at 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 at 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
>>
More information about the dev
mailing list