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 1155AA04B1; Wed, 4 Nov 2020 13:58:38 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 41D7AC8FA; Wed, 4 Nov 2020 13:58:36 +0100 (CET) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 61661C8F8 for ; Wed, 4 Nov 2020 13:58:34 +0100 (CET) IronPort-SDR: QBhmja3QXmcR/a2CqfCd0oPme1Ba/2PYi5AMwr8Skw+x2swZylz5u9fEAvLupgC/aUUly6eqtY J40LjV5a8gdQ== X-IronPort-AV: E=McAfee;i="6000,8403,9794"; a="168424059" X-IronPort-AV: E=Sophos;i="5.77,450,1596524400"; d="scan'208";a="168424059" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Nov 2020 04:58:31 -0800 IronPort-SDR: Ft7RRp+H9E2pd1wBbkIOqfSI5gGrL18ZY+g3kG/LzBi7bHnncXT4BwQeir4pdXix2YB+D8X2Y1 dZZHwWinVSHA== X-IronPort-AV: E=Sophos;i="5.77,450,1596524400"; d="scan'208";a="353823350" 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 04:58:29 -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> From: Ferruh Yigit Message-ID: <0af253e4-74cf-61ab-124e-6d873474a113@intel.com> Date: Wed, 4 Nov 2020 12:58:26 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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/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. I will be unexpected to use the pointer for id but it works, can you please add enough comment to clarify the usage? > >> I think this also prevents to corrupt 'pf' and 'psa' just for age action. > > >> >>> + case CONTEXT_TYPE_FLOW: >>> + printf("%-20s\t%" PRIu32 "\t%" PRIu32 "\t%" PRIu32 >>> + "\t%c%c%c\t\n", >>> + "Flow", >>> + ctx.pf->id, >>> + ctx.pf->rule.attr->group, >>> + ctx.pf->rule.attr->priority, >>> + ctx.pf->rule.attr->ingress ? 'i' : '-', >>> + ctx.pf->rule.attr->egress ? 'e' : '-', >>> + ctx.pf->rule.attr->transfer ? 't' : '-'); >>> + break; >>> + case CONTEXT_TYPE_SHARED_ACTION: >>> + printf("%-20s\t%" PRIu32 "\n", "Shared action", >>> + ctx.psa->id); >>> + break; >>> + default: >>> + printf("Error: invalid context type %u\n", port_id); >>> + break; >>> + } >>> } >>> if (destroy) { >>> int ret; >>> @@ -2426,15 +2451,15 @@ struct rte_flow_shared_action * >>> total = 0; >>> printf("\n"); >>> 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 || ctx.pf->ctype != >>> + CONTEXT_TYPE_FLOW) >>> continue; >> >> When the context is 'CONTEXT_TYPE_SHARED_ACTION', who destroys it? > > Destroy request is optional, I didn't add a support to destroy something here: > 1 options here is to save all the flows assigned to the age shared action inside the shared action context and destroy all of them + the shared aged action. > It can be step 2 later. > OK >> >>> - flow_id = pf->id; >>> + flow_id = ctx.pf->id; >>> ret = port_flow_destroy(port_id, 1, &flow_id); >>> if (!ret) >>> total++; >>> } >>> - printf("%d flows be destroyed\n", total); >>> + printf("%d flows destroyed\n", total); >>> } >>> free(contexts); >>> } >>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index >>> 519d551..92aaa19 100644 >>> --- a/app/test-pmd/testpmd.h >>> +++ b/app/test-pmd/testpmd.h >>> @@ -143,8 +143,14 @@ struct fwd_stream { >>> struct pkt_burst_stats tx_burst_stats; >>> }; >>> >>> +enum testpmd_context_type { >>> + CONTEXT_TYPE_FLOW, >>> + CONTEXT_TYPE_SHARED_ACTION, >>> +}; >>> + >> >> The enum prefix is too generic, 'CONTEXT_TYPE_', what do you think clarifying >> what context we are talking about? > > enum flow_age_action_context_type { > FLOW_AGE_ACTION_CTX_FLOW, > FLOW_AGE_ACTION_CTX_SHARED_ACTION, > } > > ? I think better, thanks. >> >>> /** Descriptor for a single flow. */ >>> struct port_flow { >>> + enum testpmd_context_type ctype; /**< Context type. */ >>> struct port_flow *next; /**< Next flow in list. */ >>> struct port_flow *tmp; /**< Temporary linking. */ >>> uint32_t id; /**< Flow rule ID. */ @@ -155,6 +161,7 @@ struct >>> port_flow { >>> >>> /* Descriptor for shared action */ >>> struct port_shared_action { >>> + enum testpmd_context_type ctype; /**< Context type. */ >>> struct port_shared_action *next; /**< Next flow in list. */ >>> uint32_t id; /**< Shared action ID. */ >>> enum rte_flow_action_type type; /**< Action type. */ >>> > > > What do you think about changing the rte_flow_get_aged_flows API name to rte_flow_get_aged_contexts ? > Here context has some data do identify the aged flows, right? If so 'rte_flow_get_aged_flows' also reasonable I think. No strong opinion but the API name as it is looks good to me.