Re: [dpdk-dev] [PATCH v2 1/7] power_intrinsics: use callbacks for comparison

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

 
> Previously, the semantics of power monitor were such that we were
> checking current value against the expected value, and if they matched,
> then the sleep was aborted. This is somewhat inflexible, because it only
> allowed us to check for a specific value.
> 
> This commit replaces the comparison with a user callback mechanism, so
> that any PMD (or other code) using `rte_power_monitor()` can define
> their own comparison semantics and decision making on how to detect the
> need to abort the entering of power optimized state.
> 
> Existing implementations are adjusted to follow the new semantics.
> 
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     v2:
>     - Use callback mechanism for more flexibility
>     - Address feedback from Konstantin
> 
>  doc/guides/rel_notes/release_21_08.rst        |  1 +
>  drivers/event/dlb2/dlb2.c                     | 16 ++++++++--
>  drivers/net/i40e/i40e_rxtx.c                  | 19 ++++++++----
>  drivers/net/iavf/iavf_rxtx.c                  | 19 ++++++++----
>  drivers/net/ice/ice_rxtx.c                    | 19 ++++++++----
>  drivers/net/ixgbe/ixgbe_rxtx.c                | 19 ++++++++----
>  drivers/net/mlx5/mlx5_rx.c                    | 16 ++++++++--
>  .../include/generic/rte_power_intrinsics.h    | 29 ++++++++++++++-----
>  lib/eal/x86/rte_power_intrinsics.c            |  9 ++----
>  9 files changed, 106 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/eal/include/generic/rte_power_intrinsics.h b/lib/eal/include/generic/rte_power_intrinsics.h
> index dddca3d41c..046667ade6 100644
> --- a/lib/eal/include/generic/rte_power_intrinsics.h
> +++ b/lib/eal/include/generic/rte_power_intrinsics.h
> @@ -18,19 +18,34 @@
>   * which are architecture-dependent.
>   */
> 
> +/**
> + * Callback definition for monitoring conditions. Callbacks with this signature
> + * will be used by `rte_power_monitor()` to check if the entering of power
> + * optimized state should be aborted.
> + *
> + * @param val
> + *   The value read from memory.
> + * @param opaque
> + *   Callback-specific data.
> + *
> + * @return
> + *   0 if entering of power optimized state should proceed
> + *   -1 if entering of power optimized state should be aborted
> + */
> +typedef int (*rte_power_monitor_clb_t)(const uint64_t val,
> +		const uint64_t opaque[4]);
>  struct rte_power_monitor_cond {
>  	volatile void *addr;  /**< Address to monitor for changes */
> -	uint64_t val;         /**< If the `mask` is non-zero, location pointed
> -	                       *   to by `addr` will be read and compared
> -	                       *   against this value.
> -	                       */
> -	uint64_t mask;   /**< 64-bit mask to extract value read from `addr` */
> -	uint8_t size;    /**< Data size (in bytes) that will be used to compare
> -	                  *   expected value (`val`) with data read from the
> +	uint8_t size;    /**< Data size (in bytes) that will be read from the
>  	                  *   monitored memory location (`addr`). Can be 1, 2,
>  	                  *   4, or 8. Supplying any other value will result in
>  	                  *   an error.
>  	                  */
> +	rte_power_monitor_clb_t fn; /**< Callback to be used to check if
> +	                             *   entering power optimized state should
> +	                             *   be aborted.
> +	                             */
> +	uint64_t opaque[4]; /**< Callback-specific data */


As a nit - would be good to add some new macro for '4'.
Apart from that - LGTM.
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

>  };
> 
>  /**
> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
> index 39ea9fdecd..3c5c9ce7ad 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -110,14 +110,11 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>  	/* now that we've put this address into monitor, we can unlock */
>  	rte_spinlock_unlock(&s->lock);
> 
> -	/* if we have a comparison mask, we might not need to sleep at all */
> -	if (pmc->mask) {
> +	/* if we have a callback, we might not need to sleep at all */
> +	if (pmc->fn) {
>  		const uint64_t cur_value = __get_umwait_val(
>  				pmc->addr, pmc->size);
> -		const uint64_t masked = cur_value & pmc->mask;
> -
> -		/* if the masked value is already matching, abort */
> -		if (masked == pmc->val)
> +		if (pmc->fn(cur_value, pmc->opaque) != 0)
>  			goto end;
>  	}
> 
> --
> 2.25.1