Re: [dpdk-dev] [PATCH 0/2] dpdk: Allow for dynamic enablement of some isolated features

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Wednesday, July 30, 2014 2:01 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/2] dpdk: Allow for dynamic enablement of some isolated features
> 
> On Wed, Jul 30, 2014 at 12:07:39PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > Hi Neil,
> >
> > > Hey all-
> > >         I've been trying to update the fedora dpdk package to support VFIO
> > > enabled drivers and ran into a problem in which ixgbe didn't compile because the
> > > rxtx_vec code uses sse4.2 instruction intrinsics, which aren't supported in the
> > > default config I have.  I tried to remedy this by replacing the intrinsics with
> > > the __builtin macros, but it was pointed out (correctly), that this doesn't work
> > > properly.  So this is my second attempt, which I actually like a bit better.  I
> > > noted that code that uses intrinsics (ixgbe and the acl library), don't need to
> > > have those instructions turned on build-wide.  Rather, we can just enable the
> > > instructions in the specific code we want to build with support for that, and
> > > test for instruction support dynamically at run time.  This allows me to build
> > > the dpdk for a generic platform, but in such a way that some optimizations can
> > > be used if the executing cpu supports them at run time.
> >
> > Indeed it looks much better to me too.
> > Just few nits from me:
> >
> > 1. > @@ -112,6 +112,15 @@ rte_acl_create(const struct rte_acl_param *param)
> > >  	struct rte_acl_list *acl_list;
> > >  	struct rte_tailq_entry *te;
> > >  	char name[sizeof(ctx->name)];
> > > +	static int acl_supported = -1;
> > > +
> > > +	if (acl_supported == -1)
> > > +		acl_supported = rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_2);
> >
> > Do we really need acl_supported here?
> > It seems not a big deal to just always call  rte_cpu_get_flag_enabled().
> > After all it is a create function, and no-one expects it to be extremely fast.
> >
> Need, no.  My only thought was that some poorly behaved application will call
> rte_acl_create multiple times regardless of the error returned, and doing so
> will cause large volumes of calls to cpuid, which evicts several high-use
> registers, so I didn't want to call it more than needed.  If you think its ok to
> call it multiple times though, I'm fine with removing it.

>From my thought rte_acl_create() is not supposed to be called in  the middle packet processing.
It is sort of setup function. That's why I think nothing wrong would happen even if cpuid would be called several times. 
Again  ixgbe_rx_vec_condition_check() would probably be called much more often (for each ixgbe rx queue we are going to use).

> > 2. Can you add RTE_LOG(ERR, ...) for re_acl_create() and ixgbe_rx_vec_condition_check() if sse4.2 is not supported?
> >
> Absolutely, v2 shortly.
> Neil
> 
> > Konstantin
> >
> >