[dpdk-dev] [PATCH] app/testpmd: support age shared action context

[Ferruh Yigit]

On 11/4/2020 1:28 PM, Matan Azrad wrote:
> 
> 
>   From: Ferruh Yigit
>> On 11/3/2020 7:33 AM, Matan Azrad wrote:
>>> Hi Ferruh
>>>
>>> Thank you for the fast review.
>>> Please see inline
>>>
>>> From: Ferruh Yigit
>>>> On 11/1/2020 5:48 PM, Matan Azrad wrote:
>>>>> When an age action becomes aged-out the rte_flow_get_aged_flows
>>>>> should return the action context supplied by the configuration structure.
>>>>>
>>>>> In case the age action created by the shared action API, the shared
>>>>> action context of the Testpmd application was not set.
>>>>>
>>>>> In addition, the application handler of the contexts returned by the
>>>>> rte_flow_get_aged_flows API didn't consider the fact that the action
>>>>> could be set by the shared action API and considered it as regular
>>>>> flow context.
>>>>>
>>>>> This caused a crash in Testpmd when the context is parsed.
>>>>>
>>>>> This patch set context type in the flow and shared action context
>>>>> and uses it to parse the aged-out contexts correctly.
>>>>>
>>>>> Signed-off-by: Matan Azrad <matan at nvidia.com>
>>>>> Acked-by: Dekel Peled <dekelp at nvidia.com>
>>>>> ---
>>>>>     app/test-pmd/config.c  | 57
>>>>> ++++++++++++++++++++++++++++++++++++-----
>>>> ---------
>>>>>     app/test-pmd/testpmd.h |  7 +++++++
>>>>>     2 files changed, 48 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>> e0203f0..3581f3d 100644
>>>>> --- a/app/test-pmd/config.c
>>>>> +++ b/app/test-pmd/config.c
>>>>> @@ -1665,8 +1665,10 @@ void port_flow_tunnel_create(portid_t
>>>>> port_id,
>>>> const struct tunnel_ops *ops)
>>>>>                 return NULL;
>>>>>         }
>>>>>         if (rte_flow_conv(RTE_FLOW_CONV_OP_RULE, &pf->rule, ret, &rule,
>>>>> -                       error) >= 0)
>>>>> +                       error) >= 0) {
>>>>> +             pf->ctype = CONTEXT_TYPE_FLOW;
>>>>>                 return pf;
>>>>> +     }
>>>>>         free(pf);
>>>>>         return NULL;
>>>>>     }
>>>>> @@ -1831,6 +1833,7 @@ void port_flow_tunnel_create(portid_t port_id,
>>>> const struct tunnel_ops *ops)
>>>>>         }
>>>>>         psa->next = *ppsa;
>>>>>         psa->id = id;
>>>>> +     psa->ctype = CONTEXT_TYPE_SHARED_ACTION;
>>>>>         *ppsa = psa;
>>>>>         *action = psa;
>>>>>         return 0;
>>>>> @@ -1849,6 +1852,12 @@ void port_flow_tunnel_create(portid_t
>>>>> port_id,
>>>> const struct tunnel_ops *ops)
>>>>>         ret = action_alloc(port_id, id, &psa);
>>>>>         if (ret)
>>>>>                 return ret;
>>>>> +     if (action->type == RTE_FLOW_ACTION_TYPE_AGE) {
>>>>> +             struct rte_flow_action_age *age =
>>>>> +                             (void *)(uintptr_t)(action->conf);
>>>>> +
>>>>> +             age->context = psa;
>>>>> +     }
>>>>
>>>> The port flow is using 'update_age_action_context()' function, can
>>>> same function be utilized to update age context for shared action too?
>>>
>>> For updating flow context, the code iterates all actions to find the age action -
>> so it worth to call dedicate function.
>>> For updating  shared action context - it a direct access.
>>> So, they have different search method.
>>>
>>
>> Just to reduce the age action related churn in the code, if it can be abstracted
>> in to a single function I prefer it, if that doesn't make sense it is OK.
>>
>>>
>>>>
>>>> btw, not sure why 'update_age_action_context()' is not static, if you
>>>> will touch it can you please make it static function?
>>>>
>>>> And overall this context setting for the age action is requiring the
>>>> special conditions in the flow create path, can you please check if
>>>> it can be moved to 'cmdline_flow.c' for age parsing code somehow?
>>>>
>>>>>         /* Poisoning to make sure PMDs update it in case of error. */
>>>>>         memset(&error, 0x22, sizeof(error));
>>>>>         psa->action = rte_flow_shared_action_create(port_id, conf,
>>>>> action, @@ -2379,7 +2388,10 @@ struct rte_flow_shared_action *
>>>>>         void **contexts;
>>>>>         int nb_context, total = 0, idx;
>>>>>         struct rte_flow_error error;
>>>>> -     struct port_flow *pf;
>>>>> +     union {
>>>>> +             struct port_flow *pf;
>>>>> +             struct port_shared_action *psa;
>>>>> +     } ctx;
>>>>>
>>>>>         if (port_id_is_invalid(port_id, ENABLED_WARN) ||
>>>>>             port_id == (portid_t)RTE_PORT_ALL) @@ -2397,7 +2409,7 @@
>>>>> struct rte_flow_shared_action *
>>>>>                 printf("Cannot allocate contexts for aged flow\n");
>>>>>                 return;
>>>>>         }
>>>>> -     printf("ID\tGroup\tPrio\tAttr\n");
>>>>> +     printf("%-20s\tID\tGroup\tPrio\tAttr\n", "Type");
>>>>>         nb_context = rte_flow_get_aged_flows(port_id, contexts, total,
>> &error);
>>>>>         if (nb_context != total) {
>>>>>                 printf("Port:%d get aged flows count(%d) !=
>>>>> total(%d)\n", @@ -2406,18 +2418,31 @@ struct rte_flow_shared_action *
>>>>>                 return;
>>>>>         }
>>>>>         for (idx = 0; idx < nb_context; idx++) {
>>>>> -             pf = (struct port_flow *)contexts[idx];
>>>>> -             if (!pf) {
>>>>> +             ctx.pf = (struct port_flow *)contexts[idx];
>>>>> +             if (!ctx.pf) {
>>>>>                         printf("Error: get Null context in port %u\n", port_id);
>>>>>                         continue;
>>>>>                 }
>>>>> -             printf("%" PRIu32 "\t%" PRIu32 "\t%" PRIu32 "\t%c%c%c\t\n",
>>>>> -                    pf->id,
>>>>> -                    pf->rule.attr->group,
>>>>> -                    pf->rule.attr->priority,
>>>>> -                    pf->rule.attr->ingress ? 'i' : '-',
>>>>> -                    pf->rule.attr->egress ? 'e' : '-',
>>>>> -                    pf->rule.attr->transfer ? 't' : '-');
>>>>> +             switch (ctx.pf->ctype) {
>>>>
>>>>
>>>> At this stage you don't know if the context is 'pf' or 'psa', but you
>>>> rely that both structure first element is "enum testpmd_context_type"
>>>> and this requirement is completely undocumented.
>>>
>>> Yes, will add a comment.
>>>
>>>>
>>>> Why don't create a common context and pass that one the the age
>>>> action for both 'pf' & 'psa', like
>>>>
>>>> struct port_flow_age_action_context {
>>>>        enum testpmd_context_type ctype;
>>>>        union {
>>>>            struct port_flow *pf;
>>>>            struct port_shared_action *psa;
>>>>        } ctx;
>>>> };
>>>
>>> We considered this option too,
>>> It looked us more optimized to not utilize more memory and alloc\free time
>> for each age context.
>>>
>>> One more option we considered:
>>>
>>> Use age action context pointer as uint32_t\uintptr_t - use 2 bits for type and
>> others for pf->id psa->id.
>>> What do you think about this?
>>>
>>
>> Will 'id' be enough? I see other information is used, though not sure if it is only
>> for print.
>>
> 
>  From the id we can get the pointer(and other information) - this is the same ID as supplied by the command line user to query\destroy an existed flows.
> 
>> I will be unexpected to use the pointer for id but it works, can you please add
>> enough comment to clarify the usage?
> 
> If you mean code comment, yes I will add.
> 

Yes, thanks.