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

[Thomas Monjalon <thomas@monjalon.net>]

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.

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