Re: [dpdk-dev] [PATCH v2 1/2] ethdev: support query of AGE action

[Dekel Peled <dekelp@nvidia.com>]

Thanks, PSB.

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Sunday, October 4, 2020 1:02 PM
> To: Dekel Peled <dekelp@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2 1/2] ethdev: support query of AGE action
> 
> Hi Dekel,
> PSB
> 
> > -----Original Message-----
> > From: Dekel Peled <dekelp@nvidia.com>
> > Sent: Thursday, October 1, 2020 10:33 PM
> > Subject: [PATCH v2 1/2] ethdev: support query of AGE action
> >
> > Existing API supports AGE action to monitor the aging of a flow.
> > This patch implements RFC [1], introducing the response format for
> > query of an AGE action.
> > Application will be able to query the AGE action state.
> > The response will be returned in the format implemented here.
> >
> > [1]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> > s.dp
> > dk.org%2Farchives%2Fdev%2F2020-
> >
> September%2F180061.html&amp;data=02%7C01%7Corika%40nvidia.com%7
> Ce
> >
> c8f8a845a284c153d7d08d86640d8e6%7C43083d15727340c1b7db39efd9ccc17
> >
> a%7C0%7C1%7C637371776001401360&amp;sdata=y8g1yZiKsypXy%2BgsXtR5L
> > Q2yVQ%2FudbbCfbbM1Zwt5dY%3D&amp;reserved=0
> >
> > Signed-off-by: Dekel Peled <dekelp@nvidia.com>
> > Acked-by: Matan Azrad <matan@nvidia.com>
> > ---
> >  doc/guides/prog_guide/rte_flow.rst     | 17 +++++++++++++++++
> >  doc/guides/rel_notes/release_20_11.rst |  3 +++
> >  lib/librte_ethdev/rte_flow.h           | 14 ++++++++++++++
> >  3 files changed, 34 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index 119b128..4b8d033 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2666,6 +2666,23 @@ 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:
> > +
> > +.. _table_rte_flow_query_age:
> > +
> > +.. table:: AGE query
> > +
> > +   +-------------------------+-----+-----------------------------------+
> > +   | Field                   | I/O | Value                             |
> > +
> >
> +=========================+=====+=========================
> ======
> > ====+
> > +   | ``aged``                | out | Aging timeout expired             |
> > +   +-------------------------+-----+-----------------------------------+
> > +   | ``last_hit_time_valid`` | out | ``last_hit_time`` field is valid  |
> > +   +-------------------------+-----+-----------------------------------+
> > +   | ``last_hit_time``       | out | Seconds since last traffic hit    |
> > +
> > + +-------------------------+-----+-----------------------------------
> > + +
> > +
> >  Negative types
> >  ~~~~~~~~~~~~~~
> >
> > diff --git a/doc/guides/rel_notes/release_20_11.rst
> > b/doc/guides/rel_notes/release_20_11.rst
> > index 7f9d0dd..dd77f04 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -168,6 +168,9 @@ API Changes
> >
> >  * ethdev: ``rte_eth_rx_descriptor_done()`` API has been deprecated.
> >
> > +* ethdev: Added struct ``rte_flow_query_age`` for use in response to
> > +query
> > +  of AGE action.
> > +
> >  * Renamed internal ethdev APIs:
> >
> >    * ``_rte_eth_dev_callback_process()`` ->
> > ``rte_eth_dev_callback_process()``
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index da8bfa5..1a13802 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -2130,6 +2130,7 @@ enum rte_flow_action_type {
> >  	 * See struct rte_flow_action_age.
> >  	 * See function rte_flow_get_aged_flows
> >  	 * see enum RTE_ETH_EVENT_FLOW_AGED
> > +	 * See struct rte_flow_query_age
> >  	 */
> >  	RTE_FLOW_ACTION_TYPE_AGE,
> >  };
> > @@ -2194,6 +2195,19 @@ struct rte_flow_action_age {  };
> >
> >  /**
> > + * RTE_FLOW_ACTION_TYPE_AGE (query)
> > + *
> > + * Query structure to retrieve the aging status information of a
> > + * shared AGE action, or a flow rule using the AGE action.
> > + */
> > +struct rte_flow_query_age {
> > +	uint32_t aged:1; /**< 1 if aging timeout expired, 0 otherwise. */
> > +	uint32_t last_hit_time_valid:1; /**< 1 if last_hit_time value valid. */
> > +	uint32_t reserved:6; /**< Reserved, must be zero. */
> > +	uint32_t last_hit_time:24; /**< Seconds since last traffic hit. */
> > +};
> From previous patch, I asked you why set the reserved in the middle?

I will move it to the top.

> 
> Also I think last_hit_time is by definition time, that the rule was hit.
> I think you have couple of options:
> 1. keep last_hit_time but then set it to he time the last packet was seen.
> In this case the type should be changed to 64 bit.
> 
> 2. set the name to remaining time - in this case it should show the remaining
> time.
> I think that maybe this is worth adding in any case. Since I can see a lot of
> uses to know how long this flow will age.
> 
> 3. rename to seconds_since_last_hit
> 
> What do you think?

I will go with option 3.

> 
> > +
> > +/**
> >   * @warning
> >   * @b EXPERIMENTAL: this structure may change without prior notice
> >   *
> > --
> > 1.8.3.1