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

[Thomas Monjalon]

30/10/2020 17:36, Burakov, Anatoly:
> On 30-Oct-20 3:44 PM, Thomas Monjalon wrote:
> > 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:
> >>>>>> 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;
> >>>>>> 	}
> >>>>>
[...]
> >>> 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?
> 
> Aw, crap. You're right, it's an artifact of earlier implementation. 
> We'll fix this. Thanks for letting us know!

This is what happens when everybody pushes me to merge a patch
that I believe not ready but with a lot of "acked but not reviewed".

The context around this patch series is not good to allow good quality.
That's why I think we should not merge any more patch on top of it
except DLB PMDs and fixes in this release.