Re: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Thomas Monjalon <thomas@monjalon.net>]

01/07/2024 23:34, Wathsala Wathawana Vithanage:
> > 19/06/2024 08:45, Wathsala Vithanage:
> > >  rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics)
> > > {
> > >  	memset(intrinsics, 0, sizeof(*intrinsics)); -#ifdef RTE_ARM_USE_WFE
> > >  	intrinsics->power_monitor = 1;
> > > -#endif
> > 
> > Why removing this #ifdef?
> 
> WFE is available in all the Arm platforms DPDK currently supports. Therefore, an explicit flag is not
> required at build time. 
> 
> The purpose of RTE_ARM_USE_WFE seems to be controlling the use of WFE in rte_wait_until_equal
> functions by defining RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED macro  only when RTE_ARM_USE_WFE
> is defined. (eal/arm/include/rte_pause_64.h:15)
> From what I'm hearing this was done due to a performance issue rte_wait_until_equal_X functions had
> when using WFE.
> However, that design is flawed because it made other users of WFE dependent on the choice of
> the use of WFE in rte_wait_until_equal functions as __RTE_ARM_WFE was wrapped in an
> RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED #ifdef block.
> This patch fixes this issue by moving __RTE_ARM_WFE out of RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
> block.
> 
> Perhaps we should change RTE_ARM_USE_WFE to something like 
> RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL ?

Yes perhaps.
And more importantly, you should do such change in a separate patch.
Don't hide it in WFET patch.

> > > +uint8_t wfet_en;
> > 
> > It should be made static probably.
> > This variable will be unused in some cases, needs #ifdef.
> > 
> 
> This variable is used in all cases. It's 1 when WFET is available, 0 when it's not.

It would be 0 if you make it static yes.

> > > +
> > > +RTE_INIT(rte_power_intrinsics_init)
> > > +{
> > > +#ifdef RTE_ARCH_64
> > > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_WFXT))
> > > +		wfet_en = 1;
> > > +#endif
> > > +}
> > > +
> > > +/**
> > > + * This function uses WFE/WFET instruction to make lcore suspend
> > >   * execution on ARM.
> > > - * Note that timestamp based timeout is not supported yet.
> > >   */
> > >  int
> > >  rte_power_monitor(const struct rte_power_monitor_cond *pmc,
> > >  		const uint64_t tsc_timestamp)
> > >  {
> > > -	RTE_SET_USED(tsc_timestamp);
> > > -
> > > -#ifdef RTE_ARM_USE_WFE
> > > +#ifdef RTE_ARCH_64
> > 
> > It looks wrong.
> > If RTE_ARM_USE_WFE is disabled, you should not call __RTE_ARM_WFE().
> > 
> 
> RTE_ARM_USE_WFE should be renamed to reflect its actual use. It's safe to assume that
> WFE is available universally in Arm DPDK context.