From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 99D76A04B1; Wed, 4 Nov 2020 14:45:51 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 31616C918; Wed, 4 Nov 2020 14:45:49 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 9CA95C90E for ; Wed, 4 Nov 2020 14:45:46 +0100 (CET) IronPort-SDR: BIiqcYAX/7dGOXgcEHfjXICzoQA77AOJa+7ibUxFR3ZoN9QKu2xAgQBjlGWjkb4ZWboUqBO8iH YO1QQ/Grpn7A== X-IronPort-AV: E=McAfee;i="6000,8403,9794"; a="169323241" X-IronPort-AV: E=Sophos;i="5.77,451,1596524400"; d="scan'208";a="169323241" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Nov 2020 05:45:42 -0800 IronPort-SDR: 6O9tEYbmD0ezDKWw3TKiO8ZrmAWeZSyl4krnbENNW4qtK0Un4E/tCiaOyfKPzRko2rKm436h5y 7+3e9X99i7zw== X-IronPort-AV: E=Sophos;i="5.77,451,1596524400"; d="scan'208";a="353835411" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.228.199]) ([10.213.228.199]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Nov 2020 05:45:40 -0800 To: Matan Azrad , Wenzhuo Lu , Beilei Xing , Bernard Iremonger , Ori Kam Cc: "dev@dpdk.org" References: <1604252927-213452-1-git-send-email-matan@nvidia.com> <9183aeb8-240f-ee83-8997-7628cc73077c@intel.com> <0af253e4-74cf-61ab-124e-6d873474a113@intel.com> From: Ferruh Yigit Message-ID: Date: Wed, 4 Nov 2020 13:45:34 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] app/testpmd: support age shared action context X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>>>> Acked-by: Dekel Peled >>>>> --- >>>>> 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.