[dpdk-dev] [PATCH v4 02/10] eal: add power management intrinsics

[Jerin Jacob]

On Thu, Oct 8, 2020 at 6:57 PM Burakov, Anatoly
<anatoly.burakov at intel.com> wrote:
>
> On 08-Oct-20 9:44 AM, Jerin Jacob wrote:
> > On Thu, Oct 8, 2020 at 2:04 PM Thomas Monjalon <thomas at monjalon.net> wrote:
> >>
> >>> Add two new power management intrinsics, and provide an implementation
> >>> in eal/x86 based on UMONITOR/UMWAIT instructions. The instructions
> >>> are implemented as raw byte opcodes because there is not yet widespread
> >>> compiler support for these instructions.
> >>>
> >>> The power management instructions provide an architecture-specific
> >>> function to either wait until a specified TSC timestamp is reached, or
> >>> optionally wait until either a TSC timestamp is reached or a memory
> >>> location is written to. The monitor function also provides an optional
> >>> comparison, to avoid sleeping when the expected write has already
> >>> happened, and no more writes are expected.
> >>>
> >>> For more details, Please reference Intel SDM Volume 2.
> >>
> >> I really would like to see feedbacks from other arch maintainers.
> >> Unfortunately they were not Cc'ed.
> >
> > Shared the feedback from the arm64 perspective here. Yet to get a reply on this.
> > http://mails.dpdk.org/archives/dev/2020-September/181646.html
> >
> >> Also please mark the new functions as experimental.
> >>
> >>
>
> Hi Jerin,

Hi Anatoly,

>
>  > IMO, We must introduce some arch feature-capability _get_ scheme to tell
>  > the consumer of this API is only supported on x86. Probably as
> functions[1]
>  > or macro flags scheme and have a stub for the other architectures as the
>  > API marked as generic ie rte_power_* not rte_x86_..
>  >
>  > This will help the consumer to create workers based on the
> instruction features
>  > which can NOT be abstracted as a generic feature across the
> architectures.
>
> I'm not entirely sure what you mean by that.
>
> I mean, yes, we should have added stubs for other architectures, and we
> will add those in future revisions, but what does your proposed runtime
> check accomplish that cannot currently be done with CPUID flags?


RTE_CPUFLAG_WAITPKG  flag definition is not available in other architectures.
i.e RTE_CPUFLAG_WAITPKG defined in lib/librte_eal/x86/include/rte_cpuflags.h
and it is used in http://patches.dpdk.org/patch/79540/ as generic API.
I doubt http://patches.dpdk.org/patch/79540/  would compile on non-x86.



>
> If you look at patch 1 [1], we added CPUID flags that the user can
> check, and in fact this is precisely what we do in patch 4 [2] before
> enabling the UMWAIT path. We could perhaps document this better and
> outline the dependency on the WAITPKG CPUID flag more explicitly, but
> otherwise i don't see how what you're proposing isn't already possible
> to do.
>
> [1] http://patches.dpdk.org/patch/79539/
> [2] http://patches.dpdk.org/patch/79540/ , function
> rte_power_pmd_mgmt_queue_enable()
>
> --
> Thanks,
> Anatoly