From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Ori Kam <orika@nvidia.com>, Michael Baum <michaelba@nvidia.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: Matan Azrad <matan@nvidia.com>, Raslan Darawsheh <rasland@nvidia.com>
Subject: Re: [PATCH 3/3] ethdev: add structure for indirect AGE update
Date: Tue, 4 Oct 2022 09:36:55 +0300 [thread overview]
Message-ID: <be1a45f2-1593-0aa5-e2bd-a6856118ac90@oktetlabs.ru> (raw)
In-Reply-To: <MW2PR12MB4666C2AA049A3D98A1E9A545D65B9@MW2PR12MB4666.namprd12.prod.outlook.com>
On 10/3/22 18:13, Ori Kam wrote:
> Hi Andrew,
>
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, 3 October 2022 16:18
>>
>> On 10/3/22 11:30, Ori Kam wrote:
>>> Hi Andrew,
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> 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 <michaelba@nvidia.com>
>>>>> ---
>>>>> 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
>>>
>
next prev parent reply other threads:[~2022-10-04 6:36 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-21 14:54 [PATCH 0/3] ethdev: AGE action preparation Michael Baum
2022-09-21 14:54 ` [PATCH 1/3] ethdev: add strict queue to pre-configuration flow hints Michael Baum
2022-09-29 12:04 ` Ori Kam
2022-09-21 14:54 ` [PATCH 2/3] ethdev: add queue-based API to report aged flow rules Michael Baum
2022-09-29 12:04 ` Ori Kam
2022-09-21 14:54 ` [PATCH 3/3] ethdev: add structure for indirect AGE update Michael Baum
2022-09-29 12:39 ` Ori Kam
2022-10-03 8:03 ` Andrew Rybchenko
2022-10-03 8:30 ` Ori Kam
2022-10-03 13:17 ` Andrew Rybchenko
2022-10-03 15:13 ` Ori Kam
2022-10-04 6:36 ` Andrew Rybchenko [this message]
2022-10-19 13:09 ` Michael Baum
2022-09-29 8:40 ` [PATCH 0/3] ethdev: AGE action preparation Andrew Rybchenko
2022-10-19 13:12 ` [PATCH v2 " Michael Baum
2022-10-19 13:12 ` [PATCH v2 1/3] ethdev: add strict queue to pre-configuration flow hints Michael Baum
2022-10-19 13:12 ` [PATCH v2 2/3] ethdev: add queue-based API to report aged flow rules Michael Baum
2022-10-19 13:12 ` [PATCH v2 3/3] ethdev: add structure for indirect AGE update Michael Baum
2022-10-19 14:49 ` [PATCH v3 0/3] ethdev: AGE action preparation Michael Baum
2022-10-19 14:49 ` [PATCH v3 1/3] ethdev: add strict queue to pre-configuration flow hints Michael Baum
2022-10-26 19:10 ` Andrew Rybchenko
2022-10-19 14:49 ` [PATCH v3 2/3] ethdev: add queue-based API to report aged flow rules Michael Baum
2022-10-26 19:15 ` Andrew Rybchenko
2022-10-26 21:17 ` Michael Baum
2022-10-19 14:49 ` [PATCH v3 3/3] ethdev: add structure for indirect AGE update Michael Baum
2022-10-26 19:18 ` Andrew Rybchenko
2022-10-26 21:19 ` Michael Baum
2022-10-26 21:49 ` [PATCH v4 0/3] ethdev: AGE action preparation Michael Baum
2022-10-26 21:49 ` [PATCH v4 1/3] ethdev: add strict queue to pre-configuration flow hints Michael Baum
2022-10-26 21:49 ` [PATCH v4 2/3] ethdev: add queue-based API to report aged flow rules Michael Baum
2022-10-26 21:49 ` [PATCH v4 3/3] ethdev: add structure for indirect AGE update Michael Baum
2022-10-28 10:56 ` [PATCH v4 0/3] ethdev: AGE action preparation Andrew Rybchenko
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=be1a45f2-1593-0aa5-e2bd-a6856118ac90@oktetlabs.ru \
--to=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=matan@nvidia.com \
--cc=michaelba@nvidia.com \
--cc=orika@nvidia.com \
--cc=rasland@nvidia.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).