Re: [dpdk-dev] [PATCH v10 3/9] eal: add intrinsics support check infrastructure

[Thomas Monjalon <thomas@monjalon.net>]

30/10/2020 16:27, Burakov, Anatoly:
> On 30-Oct-20 2:09 PM, Thomas Monjalon wrote:
> > 30/10/2020 14:37, Burakov, Anatoly:
> >> On 30-Oct-20 10:14 AM, Thomas Monjalon wrote:
> >>> 30/10/2020 11:09, Burakov, Anatoly:
> >>>> On 29-Oct-20 9:27 PM, David Marchand wrote:
> >>>>> On Tue, Oct 27, 2020 at 4:00 PM Liang Ma <liang.j.ma@intel.com> wrote:
> >>>>>>
> >>>>>> Currently, it is not possible to check support for intrinsics that
> >>>>>> are platform-specific, cannot be abstracted in a generic way, or do not
> >>>>>> have support on all architectures. The CPUID flags can be used to some
> >>>>>> extent, but they are only defined for their platform, while intrinsics
> >>>>>> will be available to all code as they are in generic headers.
> >>>>>>
> >>>>>> This patch introduces infrastructure to check support for certain
> >>>>>> platform-specific intrinsics, and adds support for checking support for
> >>>>>> IA power management-related intrinsics for UMWAIT/UMONITOR and TPAUSE.
> >>>>>>
> >>>>>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >>>>>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> >>>>>> Acked-by: David Christensen <drc@linux.vnet.ibm.com>
> >>>>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
> >>>>>> Acked-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>>>>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> >>>>>
> >>>>> Coming late to the party, it seems crowded...
> >>>>>
> >>>>>
> >>>>>
> >>>>>> diff --git a/lib/librte_eal/include/generic/rte_cpuflags.h b/lib/librte_eal/include/generic/rte_cpuflags.h
> >>>>>> index 872f0ebe3e..28a5aecde8 100644
> >>>>>> --- a/lib/librte_eal/include/generic/rte_cpuflags.h
> >>>>>> +++ b/lib/librte_eal/include/generic/rte_cpuflags.h
> >>>>>> @@ -13,6 +13,32 @@
> >>>>>>     #include "rte_common.h"
> >>>>>>     #include <errno.h>
> >>>>>>
> >>>>>> +#include <rte_compat.h>
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * Structure used to describe platform-specific intrinsics that may or may not
> >>>>>> + * be supported at runtime.
> >>>>>> + */
> >>>>>> +struct rte_cpu_intrinsics {
> >>>>>> +       uint32_t power_monitor : 1;
> >>>>>> +       /**< indicates support for rte_power_monitor function */
> >>>>>> +       uint32_t power_pause : 1;
> >>>>>> +       /**< indicates support for rte_power_pause function */
> >>>>>> +};
> >>>>>
> >>>>> - The rte_power library is supposed to be built on top of cpuflags.
> >>>>> Not the other way around.
> >>>>> Those capabilities should have been kept inside the rte_power_ API and
> >>>>> not pollute the cpuflags API.
> >>>>>
> >>>>> - All of this should have come as a single patch as the previously
> >>>>> introduced API is unusable before.
> >>>>>
> >>>>>
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * @warning
> >>>>>> + * @b EXPERIMENTAL: this API may change without prior notice
> >>>>>> + *
> >>>>>> + * Check CPU support for various intrinsics at runtime.
> >>>>>> + *
> >>>>>> + * @param intrinsics
> >>>>>> + *     Pointer to a structure to be filled.
> >>>>>> + */
> >>>>>> +__rte_experimental
> >>>>>> +void
> >>>>>> +rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics);
> >>>>>> +
> >>>>>>     /**
> >>>>>>      * Enumeration of all CPU features supported
> >>>>>>      */
> >>>>>> diff --git a/lib/librte_eal/include/generic/rte_power_intrinsics.h b/lib/librte_eal/include/generic/rte_power_intrinsics.h
> >>>>>> index fb897d9060..03a326f076 100644
> >>>>>> --- a/lib/librte_eal/include/generic/rte_power_intrinsics.h
> >>>>>> +++ b/lib/librte_eal/include/generic/rte_power_intrinsics.h
> >>>>>> @@ -32,6 +32,10 @@
> >>>>>>      * checked against the expected value, and if they match, the entering of
> >>>>>>      * optimized power state may be aborted.
> >>>>>>      *
> >>>>>> + * @warning It is responsibility of the user to check if this function is
> >>>>>> + *   supported at runtime using `rte_cpu_get_features()` API call. Failing to do
> >>>>>> + *   so may result in an illegal CPU instruction error.
> >>>>>> + *
> >>>>>
> >>>>> - Reading this API description... what am I supposed to do in my
> >>>>> application or driver who wants to use the new
> >>>>> rte_power_monitor/rte_power_pause stuff?
> >>>>>
> >>>>> I should call rte_cpu_get_features(TOTO) ?
> >>>>> This comment does not give a hint.
> >>>>>
> >>>>> I suppose the intent was to refer to the rte_cpu_get_intrinsics_support() thing.
> >>>>> This must be fixed.
> >>>>>
> >>>>>
> >>>>> - Again, I wonder why we are exposing all this stuff.
> >>>>> This should be hidden in the rte_power API.
> >>>>>
> >>>>
> >>>> We're exposing all of this here because the intrinsics are *not* part of
> >>>> the power API but rather are generic headers within EAL. Therefore, any
> >>>> infrastructure checking for their support can *not* reside in the power
> >>>> library, but instead has to be in EAL.
> >>>>
> >>>> The intended usage here is to call this function before calling
> >>>> rte_power_monitor(), such that:
> >>>>
> >>>> 	struct rte_cpu_intrinsics intrinsics;
> >>>>
> >>>> 	rte_cpu_get_intrinsics_support(&intrinsics);
> >>>>
> >>>> 	if (!intrinsics.power_monitor) {
> >>>> 		// rte_power_monitor not supported and cannot be used
> >>>> 		return;
> >>>> 	}
> >>>
> >>> This check could be done inside the rte_power API.
> >>
> >> I'm not quite clear on exactly what you're asking here.
> >>
> >> Do you mean the example code above? If so, code like that is already
> >> present in the power library, at the callback enable stage.
> >>
> >> If you mean to say, i should put this check into the rte_power_monitor
> >> intrinsic, then no, i don't think it's a good idea to have this
> >> expensive check every time you call rte_power_monitor.
> > 
> > No but it can be done at initialization time.
> > According to what you say above, it is alread done at callback enable stage.
> > So the app does not need to do the check?
> 
> Admittedly it's a bit confusing, but please bear with me.
> 
> There are two separate issues at hand: the intrinsic itself, and the 
> calling code. We provide both.
> 
> That means, the *calling code* should do the check. In our case, *our* 
> calling code is the callback. However, nothing stops someone else from 
> implementing their own scheme using our intrinsic - in that case, the 
> user will be responsible to check if the intrinsic is supported before 
> using it in their own code, because they won't be using our callback but 
> will be using our intrinsic.
> 
> So, we have a check *in our calling code*. But if someone were to use 
> the *intrinsic* directly (like DLB), they would have to add their own 
> checks around the intrinsic usage.
> 
> Our power intrinsic is a static inline function. Are you proposing to 
> add some sort of function pointer wrapper and make it an indirect call 
> instead of a static inline function? (or indeed a proper function)
> 
> > 
> >> If you mean put this entire infrastructure into the power API - well,
> >> that kinda defeats the purpose of both having these intrinsics in
> >> generic headers and having a generic CPU feature check infrastructure
> >> that was requested of us during the review. We of course can move the
> >> intrinsic to the power library and outside of EAL, but then anything
> >> that requires UMWAIT will have to depend on the librte_power.
> > 
> > Yes the intrinsics can be in EAL if usable.
> > But it seems DLB author cannot use what is in EAL.
> 
> I'll let the DLB authors clarify that themselves, but as far as i'm 
> aware, it seems that this is not the case - while their current code 
> wouldn't be able to use these intrinsics by search-and-replace, they 
> will be able to use them with a couple of changes to their code that 
> basically amounted to reimplementation of our intrinsics.
> 
> > 
> >> Please clarify exactly what changes you would like to see here, and what
> >> is your objection.
> >>
> >>>
> >>>> 	// proceed with rte_power_monitor usage
> >>>>
> >>>> Failing to do that will result in either -ENOTSUP on non-x86, or illegal
> >>>> instruction crash on x86 that doesn't have that instruction (because we
> >>>> encode raw opcode).
> >>>>
> >>>> I've *not* added this to the previous patches because i wanted to get
> >>>> this part reviewed specifically, and not mix it with other IA-specific
> >>>> stuff. It seems that i've succeeded in that goal, as this patch has 4
> >>>> likes^W acks :)
> >>>
> >>> You did not explain the need for rte_cpu_get_features() call.
> >>>
> >>
> >> Did not explain *where*? Are you suggesting i put things about
> >> rte_power_monitor into documentation for rte_cpu_get_intrinsics_support?
> >> The documentation for rte_power_monitor already states that one should
> >> use rte_cpu_get_intrinsics_support API to check if the rte_power_monitor
> >> is supported on current machine. What else is missing?
> > 
> > In your example above, you do not call rte_cpu_get_features()
> > which is documented as required in the EAL doc.
> > 
> 
> I'm not sure i follow. This is unrelated to rte_cpu_get_features call. 
> The rte_cpu_get_features is a CPUID check, and it was decided not to use 
> it because the WAITPKG CPUID flag is only defined for x86 and not for 
> other archs. This new call (rte_cpu_get_intrinsics_support) is non-arch 
> specific, but will have an arch-specific implementation (which happens 
> to use rte_cpu_get_features to detect support for WAITPKG). I have given 
> the example code of how to detect support for rte_power_monitor using 
> this new code, in the code example you just referred to.

Please read the API again:
http://git.dpdk.org/dpdk/tree/lib/librte_eal/include/generic/rte_power_intrinsics.h
"
 * @warning It is responsibility of the user to check if this function is
 *   supported at runtime using `rte_cpu_get_features()` API call.
 *   Failing to do so may result in an illegal CPU instruction error.
"
Why is it referring to rte_cpu_get_features?