DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Matan Azrad <matan@nvidia.com>, Wenzhuo Lu <wenzhuo.lu@intel.com>,
	Beilei Xing <beilei.xing@intel.com>,
	Bernard Iremonger <bernard.iremonger@intel.com>,
	Ori Kam <orika@nvidia.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: support age shared action context
Date: Wed, 4 Nov 2020 13:45:34 +0000	[thread overview]
Message-ID: <a5f47484-0aa3-30c7-ef3d-7eb48cfc56b0@intel.com> (raw)
In-Reply-To: <MW2PR12MB2492691971F3A4EB25A1D20BDFEF0@MW2PR12MB2492.namprd12.prod.outlook.com>

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@nvidia.com>
>>>>> Acked-by: Dekel Peled <dekelp@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.

  reply	other threads:[~2020-11-04 13:45 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-01 17:48 Matan Azrad
2020-11-02 18:50 ` Ferruh Yigit
2020-11-03  7:33   ` Matan Azrad
2020-11-04 12:58     ` Ferruh Yigit
2020-11-04 13:28       ` Matan Azrad
2020-11-04 13:45         ` Ferruh Yigit [this message]
2020-11-05 21:32 ` [dpdk-dev] [PATCH v2] " Matan Azrad
2020-11-06 13:57   ` Ferruh Yigit
2020-11-07 17:30     ` Matan Azrad
2020-11-09 10:21       ` Ferruh Yigit
2020-11-09 10:38         ` Matan Azrad
2020-11-09 11:12           ` Ferruh Yigit
2020-11-10  8:30             ` Ori Kam
2020-11-10  9:43               ` Ferruh Yigit
2020-11-10 10:58                 ` Matan Azrad
2020-11-10 17:06   ` [dpdk-dev] [PATCH v3] " Matan Azrad
2020-11-11 12:51     ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a5f47484-0aa3-30c7-ef3d-7eb48cfc56b0@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=beilei.xing@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=matan@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).