From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 55B2CA0543; Tue, 4 Oct 2022 08:36:58 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EF33940DDC; Tue, 4 Oct 2022 08:36:57 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id BBA9C40A87 for ; Tue, 4 Oct 2022 08:36:56 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 24AB95D; Tue, 4 Oct 2022 09:36:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 24AB95D DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1664865416; bh=Ob6CmvhhhJHcnqIFmbzVR4V0Dd47d51qHP2S6fGCN0s=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=HwLDkm46y0DtwHvj5zbgcsr4DO9WbM7TolvGSCtQg/0IEHAhAXMeqdQGAj9TXQBmD Offi0aiYnhNm5mCIg5e+9um6EN/zPfngB5k4viQFkWhBw7wdqUatIqpay8VqNkv2QM WPLOdyJdBNc+7DbggPkrS8bH7ugP5llCRsCBNr2w= Message-ID: Date: Tue, 4 Oct 2022 09:36:55 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH 3/3] ethdev: add structure for indirect AGE update Content-Language: en-US To: Ori Kam , Michael Baum , "dev@dpdk.org" Cc: Matan Azrad , Raslan Darawsheh References: <20220921145409.511328-1-michaelba@nvidia.com> <20220921145409.511328-4-michaelba@nvidia.com> <811afcdb-2081-b20d-5d49-8659760a7026@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 10/3/22 18:13, Ori Kam wrote: > Hi Andrew, > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Monday, 3 October 2022 16:18 >> >> On 10/3/22 11:30, Ori Kam wrote: >>> Hi Andrew, >>> >>>> -----Original Message----- >>>> From: Andrew Rybchenko >>>> Sent: Monday, 3 October 2022 11:04 >>>> >>>> On 9/21/22 17:54, Michael Baum wrote: >>>>> Add a new structure for indirect AGE update. >>>>> >>>>> This new structure enables: >>>>> 1. Update timeout value. >>>>> 2. Stop AGE checking. >>>>> 3. Start AGE checking. >>>>> 4. restart AGE checking. >>>>> >>>>> Signed-off-by: Michael Baum >>>>> --- >>>>> app/test-pmd/cmdline_flow.c | 66 >>>> ++++++++++++++++++++++++++++++ >>>>> app/test-pmd/config.c | 18 +++++++- >>>>> doc/guides/prog_guide/rte_flow.rst | 25 +++++++++-- >>>>> lib/ethdev/rte_flow.h | 27 ++++++++++++ >>>>> 4 files changed, 132 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test- >> pmd/cmdline_flow.c >>>>> index 4fb90a92cb..a315fd9ded 100644 >>>>> --- a/app/test-pmd/cmdline_flow.c >>>>> +++ b/app/test-pmd/cmdline_flow.c >>>>> @@ -583,6 +583,9 @@ enum index { >>>>> ACTION_SET_IPV6_DSCP_VALUE, >>>>> ACTION_AGE, >>>>> ACTION_AGE_TIMEOUT, >>>>> + ACTION_AGE_UPDATE, >>>>> + ACTION_AGE_UPDATE_TIMEOUT, >>>>> + ACTION_AGE_UPDATE_TOUCH, >>>>> ACTION_SAMPLE, >>>>> ACTION_SAMPLE_RATIO, >>>>> ACTION_SAMPLE_INDEX, >>>>> @@ -1869,6 +1872,7 @@ static const enum index next_action[] = { >>>>> ACTION_SET_IPV4_DSCP, >>>>> ACTION_SET_IPV6_DSCP, >>>>> ACTION_AGE, >>>>> + ACTION_AGE_UPDATE, >>>>> ACTION_SAMPLE, >>>>> ACTION_INDIRECT, >>>>> ACTION_MODIFY_FIELD, >>>>> @@ -2113,6 +2117,14 @@ static const enum index action_age[] = { >>>>> ZERO, >>>>> }; >>>>> >>>>> +static const enum index action_age_update[] = { >>>>> + ACTION_AGE_UPDATE, >>>>> + ACTION_AGE_UPDATE_TIMEOUT, >>>>> + ACTION_AGE_UPDATE_TOUCH, >>>>> + ACTION_NEXT, >>>>> + ZERO, >>>>> +}; >>>>> + >>>>> static const enum index action_sample[] = { >>>>> ACTION_SAMPLE, >>>>> ACTION_SAMPLE_RATIO, >>>>> @@ -2191,6 +2203,9 @@ static int parse_vc_spec(struct context *, const >>>> struct token *, >>>>> const char *, unsigned int, void *, unsigned int); >>>>> static int parse_vc_conf(struct context *, const struct token *, >>>>> const char *, unsigned int, void *, unsigned int); >>>>> +static int parse_vc_conf_timeout(struct context *, const struct token *, >>>>> + const char *, unsigned int, void *, >>>>> + unsigned int); >>>>> static int parse_vc_item_ecpri_type(struct context *, const struct token >> *, >>>>> const char *, unsigned int, >>>>> void *, unsigned int); >>>>> @@ -6194,6 +6209,30 @@ static const struct token token_list[] = { >>>>> .next = NEXT(action_age, >>>> NEXT_ENTRY(COMMON_UNSIGNED)), >>>>> .call = parse_vc_conf, >>>>> }, >>>>> + [ACTION_AGE_UPDATE] = { >>>>> + .name = "age_update", >>>>> + .help = "update aging parameter", >>>>> + .next = NEXT(action_age_update), >>>>> + .priv = PRIV_ACTION(AGE, >>>>> + sizeof(struct rte_flow_update_age)), >>>>> + .call = parse_vc, >>>>> + }, >>>>> + [ACTION_AGE_UPDATE_TIMEOUT] = { >>>>> + .name = "timeout", >>>>> + .help = "age timeout update value", >>>>> + .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_update_age, >>>>> + timeout, 24)), >>>>> + .next = NEXT(action_age_update, >>>> NEXT_ENTRY(COMMON_UNSIGNED)), >>>>> + .call = parse_vc_conf_timeout, >>>>> + }, >>>>> + [ACTION_AGE_UPDATE_TOUCH] = { >>>>> + .name = "touch", >>>>> + .help = "this flow is touched", >>>>> + .next = NEXT(action_age_update, >>>> NEXT_ENTRY(COMMON_BOOLEAN)), >>>>> + .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_update_age, >>>>> + touch, 1)), >>>>> + .call = parse_vc_conf, >>>>> + }, >>>>> [ACTION_SAMPLE] = { >>>>> .name = "sample", >>>>> .help = "set a sample action", >>>>> @@ -7031,6 +7070,33 @@ parse_vc_conf(struct context *ctx, const >> struct >>>> token *token, >>>>> return len; >>>>> } >>>>> >>>>> +/** Parse action configuration field. */ >>>>> +static int >>>>> +parse_vc_conf_timeout(struct context *ctx, const struct token *token, >>>>> + const char *str, unsigned int len, >>>>> + void *buf, unsigned int size) >>>>> +{ >>>>> + struct buffer *out = buf; >>>>> + struct rte_flow_update_age *update; >>>>> + >>>>> + (void)size; >>>>> + if (ctx->curr != ACTION_AGE_UPDATE_TIMEOUT) >>>>> + return -1; >>>>> + /* Token name must match. */ >>>>> + if (parse_default(ctx, token, str, len, NULL, 0) < 0) >>>>> + return -1; >>>>> + /* Nothing else to do if there is no buffer. */ >>>>> + if (!out) >>>>> + return len; >>>>> + /* Point to selected object. */ >>>>> + ctx->object = out->args.vc.data; >>>>> + ctx->objmask = NULL; >>>>> + /* Update the timeout is valid. */ >>>>> + update = (struct rte_flow_update_age *)out->args.vc.data; >>>>> + update->timeout_valid = 1; >>>>> + return len; >>>>> +} >>>>> + >>>>> /** Parse eCPRI common header type field. */ >>>>> static int >>>>> parse_vc_item_ecpri_type(struct context *ctx, const struct token >> *token, >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >>>>> index 31952467fb..45495385d7 100644 >>>>> --- a/app/test-pmd/config.c >>>>> +++ b/app/test-pmd/config.c >>>>> @@ -2065,6 +2065,7 @@ port_action_handle_update(portid_t port_id, >>>> uint32_t id, >>>>> if (!pia) >>>>> return -EINVAL; >>>>> switch (pia->type) { >>>>> + case RTE_FLOW_ACTION_TYPE_AGE: >>>>> case RTE_FLOW_ACTION_TYPE_CONNTRACK: >>>>> update = action->conf; >>>>> break; >>>>> @@ -2904,6 +2905,8 @@ port_queue_action_handle_update(portid_t >>>> port_id, >>>>> struct rte_port *port; >>>>> struct rte_flow_error error; >>>>> struct rte_flow_action_handle *action_handle; >>>>> + struct port_indirect_action *pia; >>>>> + const void *update; >>>>> >>>>> action_handle = port_action_handle_get_by_id(port_id, id); >>>>> if (!action_handle) >>>>> @@ -2915,8 +2918,21 @@ port_queue_action_handle_update(portid_t >>>> port_id, >>>>> return -EINVAL; >>>>> } >>>>> >>>>> + pia = action_get_by_id(port_id, id); >>>>> + if (!pia) >>>>> + return -EINVAL; >>>>> + >>>>> + switch (pia->type) { >>>>> + case RTE_FLOW_ACTION_TYPE_AGE: >>>>> + update = action->conf; >>>>> + break; >>>>> + default: >>>>> + update = action; >>>>> + break; >>>>> + } >>>>> + >>>>> if (rte_flow_async_action_handle_update(port_id, queue_id, &attr, >>>>> - action_handle, action, NULL, &error)) { >>>>> + action_handle, update, NULL, &error)) { >>>>> return port_flow_complain(&error); >>>>> } >>>>> printf("Indirect action #%u update queued\n", id); >>>>> diff --git a/doc/guides/prog_guide/rte_flow.rst >>>> b/doc/guides/prog_guide/rte_flow.rst >>>>> index 588914b231..dae9121279 100644 >>>>> --- a/doc/guides/prog_guide/rte_flow.rst >>>>> +++ b/doc/guides/prog_guide/rte_flow.rst >>>>> @@ -2958,7 +2958,7 @@ Otherwise, RTE_FLOW_ERROR_TYPE_ACTION >>>> error will be returned. >>>>> Action: ``AGE`` >>>>> ^^^^^^^^^^^^^^^ >>>>> >>>>> -Set ageing timeout configuration to a flow. >>>>> +Set aging timeout configuration to a flow. >>>>> >>>>> Event RTE_ETH_EVENT_FLOW_AGED will be reported if >>>>> timeout passed without any matching on the flow. >>>>> @@ -2977,8 +2977,8 @@ timeout passed without any matching on the >>>> flow. >>>>> | ``context`` | user input flow context | >>>>> +--------------+---------------------------------+ >>>>> >>>>> -Query structure to retrieve ageing status information of a >>>>> -shared AGE action, or a flow rule using the AGE action: >>>>> +Query structure to retrieve aging status information of an >>>>> +indirect AGE action, or a flow rule using the AGE action: >>>>> >>>>> .. _table_rte_flow_query_age: >>>>> >>>>> @@ -2994,6 +2994,25 @@ shared AGE action, or a flow rule using the >> AGE >>>> action: >>>>> | ``sec_since_last_hit`` | out | Seconds since last traffic hit | >>>>> +------------------------------+-----+----------------------------------------+ >>>>> >>>>> +Update structure to modify the parameters of an indirect AGE action. >>>>> +The update structure is used by ``rte_flow_action_handle_update()`` >>>> function. >>>>> + >>>>> +.. _table_rte_flow_update_age: >>>>> + >>>>> +.. table:: AGE update >>>>> + >>>>> + +-------------------+--------------------------------------------------------------+ >>>>> + | Field | Value | >>>>> + >>>> +===================+===================================== >>>> =========================+ >>>>> + | ``timeout`` | 24 bits timeout value | >>>>> + +-------------------+--------------------------------------------------------------+ >>>>> + | ``timeout_valid`` | 1 bit, timeout value is valid | >>>>> + +-------------------+--------------------------------------------------------------+ >>>>> + | ``touch`` | 1 bit, touch the AGE action to set >> ``sec_since_last_hit`` 0 >>>> | >>>>> + +-------------------+--------------------------------------------------------------+ >>>>> + | ``reserved`` | 6 bits reserved, must be zero | >>>>> + +-------------------+--------------------------------------------------------------+ >>>>> + >>>>> Action: ``SAMPLE`` >>>>> ^^^^^^^^^^^^^^^^^^ >>>>> >>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h >>>>> index d830b02321..a21d437cf8 100644 >>>>> --- a/lib/ethdev/rte_flow.h >>>>> +++ b/lib/ethdev/rte_flow.h >>>>> @@ -2954,6 +2954,33 @@ struct rte_flow_query_age { >>>>> uint32_t sec_since_last_hit:24; /**< Seconds since last traffic hit. */ >>>>> }; >>>>> >>>>> +/** >>>>> + * @warning >>>>> + * @b EXPERIMENTAL: this structure may change without prior notice >>>>> + * >>>>> + * RTE_FLOW_ACTION_TYPE_AGE >>>>> + * >>>>> + * Update indirect AGE action attributes: >>>>> + * - Timeout can be updated including stop/start action: >>>>> + * +-------------+-------------+------------------------------+ >>>>> + * | Old Timeout | New Timeout | Updating | >>>>> + * >>>> +=============+=============+============================= >>>> =+ >>>>> + * | 0 | positive | Start aging with new value | >>>>> + * +-------------+-------------+------------------------------+ >>>>> + * | positive | 0 | Stop aging | >>>>> + * +-------------+-------------+------------------------------+ >>>>> + * | positive | positive | Change timeout to new value | >>>>> + * +-------------+-------------+------------------------------+ >>>>> + * - sec_since_last_hit can be reset. >>>>> + */ >>>>> +struct rte_flow_update_age { >>>> >>>> I think it worse to mention the structure in >>>> RTE_FLOW_ACTION_TYPE_AGE description. >>> >>> What do you mean? >> >> RTE_FLOW_ACTION_TYPE_AGE has a description as enum member. >> Typically configuration structures are mentioned there. >> However, I think that it would be useful to mention specific >> update structure if any. Like this one. Just to make it clear >> that if a user wants to update the action configuration it >> should use specific structure. Or am I missing something? >> Misunderstand something? >> > > Do you mean something like: > * see enum RTE_ETH_EVENT_FLOW_AGED > * See struct rte_flow_query_age > + See struct rte_flow_update_age > */ > RTE_FLOW_ACTION_TYPE_AGE, > If so, I fully agree. Yes, exactly. > >>> >>>> >>>>> + uint32_t reserved:6; /**< Reserved, must be zero. */ >>>>> + uint32_t timeout_valid:1; /**< The timeout is valid for update. */ >>>>> + uint32_t timeout:24; /**< Time in seconds. */ >>>>> + uint32_t touch:1; >>>>> + /**< Means that aging should assume packet passed the aging. */ >>>> >>>> Documentation after code makes sense if it is in the same line. >>>> If the documentation is in its own line, it should be before >>>> the code and use /** prefix. >>>> >>> >>> +1 >>> >>>> One more question: why order in the documentation above does >>>> not match order here? >>>> >>> >>> I think we can remove it from the documentation since it doesn't add >> anything >>> >>>>> +}; >>>>> + >>>>> /** >>>>> * @warning >>>>> * @b EXPERIMENTAL: this structure may change without prior notice >>> >