[dpdk-dev] [PATCH v1 1/7] power_intrinsics: allow monitor checks inversion

[Ananyev, Konstantin]

Hi Anatoly,

> 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 adds an option to reverse the check, so that we can have
> monitor sleep aborted if the expected value *doesn't* match what's in
> memory. This allows us to both implement all currently implemented
> driver code, as well as support more use cases which don't easily map to
> previous semantics (such as waiting on writes to AF_XDP counter value).
> 
> Since the old behavior is the default, no need to adjust existing
> implementations.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
> ---
>  lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
>  lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eal/include/generic/rte_power_intrinsics.h b/lib/eal/include/generic/rte_power_intrinsics.h
> index dddca3d41c..1006c2edfc 100644
> --- a/lib/eal/include/generic/rte_power_intrinsics.h
> +++ b/lib/eal/include/generic/rte_power_intrinsics.h
> @@ -31,6 +31,10 @@ struct rte_power_monitor_cond {
>  	                  *   4, or 8. Supplying any other value will result in
>  	                  *   an error.
>  	                  */
> +	uint8_t invert;  /**< Invert check for expected value (e.g. instead of
> +	                  *   checking if `val` matches something, check if
> +	                  *   `val` *doesn't* match a particular value)
> +	                  */
>  };
> 
>  /**
> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
> index 39ea9fdecd..5d944e9aa4 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>  		const uint64_t masked = cur_value & pmc->mask;
> 
>  		/* if the masked value is already matching, abort */
> -		if (masked == pmc->val)
> +		if (!pmc->invert && masked == pmc->val)
> +			goto end;
> +		/* same, but for inverse check */
> +		if (pmc->invert && masked != pmc->val)
>  			goto end;
>  	}
> 

Hmm..., such approach looks too 'patchy'...
Can we at least replace 'inver' with something like:
enum rte_power_monitor_cond_op {
	_EQ, NEQ,...
};
Then at least new comparions ops can be added in future.
Even better I think would be to just leave to PMD to provide a comparison callback.
Will make things really simple and generic:
struct rte_power_monitor_cond {
     volatile void *addr;
     int (*cmp)(uint64_t val);
     uint8_t size;
};
And then in rte_power_monitor(...):
....
const uint64_t cur_value = __get_umwait_val(pmc->addr, pmc->size);
if (pmc->cmp(cur_value) != 0)
	goto end;
....