Re: [PATCH v1] power: add wakeup log

[David Hunt <david.hunt@intel.com>]

On 22/2/2022 1:52 PM, Miao Li wrote:
> This patch adds a log in rte_power_monitor to show the core has been
> waked up.
>
> Signed-off-by: Miao Li <miao.li@intel.com>
> ---
>   lib/eal/x86/rte_power_intrinsics.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
> index f749da9b85..dd63e2b6eb 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -128,6 +128,14 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>   			: "D"(0), /* enter C0.2 */
>   			  "a"(tsc_l), "d"(tsc_h));
>   
> +	cur_value = __get_umwait_val(pmc->addr, pmc->size);
> +
> +	/* check if core has been waked up by changing monitoring value */
> +	if (pmc->fn(cur_value, pmc->opaque) != 0)
> +		RTE_LOG(INFO, EAL,
> +			"lcore %u is waked up from value change\n",
> +			rte_lcore_id());
> +
>   end:
>   	/* erase sleep address */
>   	rte_spinlock_lock(&s->lock);


Hi Li,

If I'm not mistaken, a similar patch was added to a previous DPDK 
release and then removed because of the enormous performance impact.

This looks to be something similar, and it's adding a log message to a 
low-level function. Also, as mentioned before, the intention in the 
future is to call this function much more agressively, so there would be 
hundreds of thousands of messages every second.

We cannot add an RTE_LOG here. Please rework and put the log in the test 
case instead.

Also, regarding the wording, I would suggest  "lcore %u awoke due to 
monitor address value change\n"

Rgds,

Dave.