RE: [PATCH v1] power: add wakeup log

["Li, Miao" <miao.li@intel.com>]

Hi,

> -----Original Message-----
> From: Hunt, David <david.hunt@intel.com>
> Sent: Wednesday, February 23, 2022 12:32 AM
> To: Li, Miao <miao.li@intel.com>; dev@dpdk.org
> Cc: Wang, Yinan <yinan.wang@intel.com>; stephen@networkplumber.org
> Subject: Re: [PATCH v1] power: add wakeup log
> 
> 
> 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.

I add a judgment before the output. When no packet arriver, the log will not be printed. When a lot of packets arriver, the rte_power_monitor will not be called. So I think the performance impact is small.

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

I will change the log content in next version.

> 
> Rgds,
> 
> Dave.
> 

Thanks,
Miao