> 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);
>
>
