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.
next prev parent 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).