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.
