Re: [PATCH v2 2/4] ether: add flow last hit query support

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Sun, Apr 01, 2018 at 05:19:20PM -0400, Qi Zhang wrote:
> Enhanced the action RTE_FLOW_TYPE_ACTION_COUNT, number of
> milliseconds since last hit can be queried.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

Please confirm whether existing devices have the ability to report time
elapsed since last hit, or if PMDs are supposed to take care of that
entirely on their own in software?

If the latter, I suggest to drop this patch and let applications check
counters regularly on their own. Unlike applications, PMDs do not easily
have access to a reliable time source.

Otherwise, the patch looks acceptable but I can't tell if milliseconds are
the right unit for such information. Same issue as mbuf timestamps [1]
basically. As a 64-bit field, a precision down to the nanosecond is a
possibility so perhaps like mbufs, the reference and precision should be
undefined in the API in order to be processed by a PMD callback?

More comments below.

[1] commit 918ae9dc775e ("mbuf: add a timestamp field")

> ---
>  lib/librte_ether/rte_flow.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 1080086..8f75db0 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -1054,9 +1054,11 @@ struct rte_flow_query_count {
>  	uint32_t reset:1; /**< Reset counters after query [in]. */
>  	uint32_t hits_set:1; /**< hits field is set [out]. */
>  	uint32_t bytes_set:1; /**< bytes field is set [out]. */
> +	uint32_t last_hit_set:1; /**< last_hit field is set [out]. */
>  	uint32_t reserved:29; /**< Reserved, must be zero [in, out]. */

You need to decrement reserved bits.

>  	uint64_t hits; /**< Number of hits for this rule [out]. */
>  	uint64_t bytes; /**< Number of bytes through this rule [out]. */
> +	uint64_t last_hit; /**< Number of milliseconds since last hit [out]. */
>  };

Doing so impacts ABI compatibility. While normally frowned upon for
rte_flow, it's OK for 18.05 because we already destroyed it. You still need
to mention what functions are impacted by this change as in "ethdev: add
encap level to RSS flow API action" [2] and update the .map files where
necessary.

In this case at least rte_flow_query() is impacted.

Please update doc/guides/prog_guide/rte_flow.rst as well (look for "COUNT
query").

[2] http://dpdk.org/ml/archives/dev/2018-April/096531.html

-- 
Adrien Mazarguil
6WIND