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

[Ananyev, Konstantin]

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


I am agree with Jerin, that we need some generic way to
figure-out does platform supports power_monitor() or not.
Though not sure do we need to create a new feature-get framework here...
Might be just something like:
 rte_power_monitor(...) == -ENOTSUP
be enough indication for that?
So user can just do:
if (rte_power_monitor(NULL, 0, 0, 0, 0) == -ENOTSUP) {
	/* not supported  path */
}

To check is that feature supported or not.

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