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

[Thomas Monjalon <thomas@monjalon.net>]

09/10/2020 11:25, Burakov, Anatoly:
> On 09-Oct-20 6:42 AM, Jerin Jacob wrote:
> > On Thu, Oct 8, 2020 at 10:38 PM Ananyev, Konstantin
> > <konstantin.ananyev@intel.com> wrote:
> >>
> >>>
> >>> On Thu, Oct 8, 2020 at 6:57 PM Burakov, Anatoly
> >>> <anatoly.burakov@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@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...
> > 
> > That's works too. Some means of generic probing is fine. Following
> > schemed needs
> > more documentation on that usage, as, it is not straight forward compare to
> > feature-get framework. Also, on the other thread, we are adding the
> > new instructions like
> > demote cacheline etc, maybe if the user wants to KNOW if the arch
> > supports it then
> > the feature-get framework is good.
> > If we think, there is no other usecase for generic arch feature-get
> > framework then
> > we can keep the below scheme else generic arch feature is better for
> > more forward
> > looking use cases.
> > 
> >> 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.
> > 
> > 
> 
> Looking at CLDEMOTE patches, CLDEMOTE is a noop on other archs. I think 
> we can safely make this intrinsic as a noop on other archs as well, as 
> it's functionally identical to waking up immediately.
> 
> If we're not creating this for CLDEMOTE, we don't need it here as well. 
> If we do need it for this, then we arguably need it for CLDEMOTE too.

Sorry I don't understand what you mean, too many "it" and "this" :)