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

[Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com>]

> 19/06/2024 08:45, Wathsala Vithanage:
> > --- a/lib/eal/arm/include/rte_cpuflags_64.h
> > +++ b/lib/eal/arm/include/rte_cpuflags_64.h
> > @@ -36,6 +36,7 @@ enum rte_cpu_flag_t {
> >  	RTE_CPUFLAG_SVEF64MM,
> >  	RTE_CPUFLAG_SVEBF16,
> >  	RTE_CPUFLAG_AARCH64,
> > +	RTE_CPUFLAG_WFXT,
> >  };
> 
> It may be useful to add comments explaining each flag.
> May be a separate patch in this series?
> 
+1
> 
> > - * Copyright(c) 2019 Arm Limited
> > + * Copyright(c) 2024 Arm Limited
> 
> No, it's wrong to remove initial date,
> and no, you don't need to update dates at all.
> 
> 
> > -#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
> 
> Why removing this #ifdef?

It's not removed, it's moved further down and just above rte_wait_until_equal_X functions.
Use of SEV, and WFE are not limited to  rte_wait_until_equal_X functions, 
PMDs should be able to use them for power management. 

> 
> > -/* Send a local event to quit WFE. */
> > +/* Send a local event to quit WFE/WFxT. */
> >  #define __RTE_ARM_SEVL() { asm volatile("sevl" : : : "memory"); }
> >
> > -/* Send a global event to quit WFE for all cores. */
> > +/* Send a global event to quit WFE/WFxT for all cores. */
> >  #define __RTE_ARM_SEV() { asm volatile("sev" : : : "memory"); }
> >
> >  /* Put processor into low power WFE(Wait For Event) state. */
> > #define __RTE_ARM_WFE() { asm volatile("wfe" : : : "memory"); }
> >
> > +/* Put processor into low power WFET (WFE with Timeout) state. */
> > +#ifdef RTE_ARM_FEATURE_WFXT
> > +#define __RTE_ARM_WFET(t) {                              \
> > +	asm volatile("wfet %x[to]"                        \
> > +			:                                 \
> > +			: [to] "r" (t)                    \
> > +			: "memory");                      \
> > +	}
> 
> Is there any intrinsic function available?
> 

We don't have an intrinsic at the moment.

> 
> [...]
> > --- a/lib/eal/arm/rte_cpuflags.c
> > +++ b/lib/eal/arm/rte_cpuflags.c
> > @@ -115,6 +115,7 @@ const struct feature_entry rte_cpu_feature_table[] =
> {
> >  	FEAT_DEF(SVEF32MM,	REG_HWCAP2,   10)
> >  	FEAT_DEF(SVEF64MM,	REG_HWCAP2,   11)
> >  	FEAT_DEF(SVEBF16,	REG_HWCAP2,   12)
> > +	FEAT_DEF(WFXT,		REG_HWCAP2,   31)
> >  	FEAT_DEF(AARCH64,	REG_PLATFORM,  0)
> 
> Are you sure of alignment? (looks wrong in my email client)

Didn't see this before, I will check.

> 
> 
> [...]
> >  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 ?

> 
> 
> > +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.


> > +
> > +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.

> >  	const unsigned int lcore_id = rte_lcore_id();
> >  	uint64_t cur_value;
> >
> > @@ -33,28 +44,30 @@ rte_power_monitor(const struct
> > rte_power_monitor_cond *pmc,
> >
> >  	switch (pmc->size) {
> >  	case sizeof(uint8_t):
> > -		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value,
> rte_memory_order_relaxed)
> > -		__RTE_ARM_WFE()
> > +		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value,
> > +rte_memory_order_relaxed);
> 
>