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

[Neil Horman <nhorman@tuxdriver.com>]

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.

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