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 <ma...@nvidia.com> > >>> Acked-by: Dekel Peled <dek...@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. > > > >> 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. OK, thanks.