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 02:09:20PM -0700, Bruce Richardson wrote:
> On Wed, Jul 30, 2014 at 03:28:44PM -0400, Neil Horman wrote:
> > On Wed, Jul 30, 2014 at 11:59:03AM -0700, Bruce Richardson wrote:
> > > On Tue, Jul 29, 2014 at 04:24:24PM -0400, Neil Horman wrote:
> > > > 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.
> > > > 
> > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > CC: Thomas Monjalon <thomas.monjalon@6wind.com>
> > > >
> > > I'd prefer if a solution could be found based off your original patch
> > > set, as it gives us more chance to deprecate the older code paths in
> > > future. Looking at the Intel Intrinsics Guide site online, it shows that
> > > the _mm_shuffle_epi8 intrinsic came in with SSSE3, rather than SSE4.x,
> > > and so should be available on all 64-bit systems, I believe. The
> > > popcount intrinsic is newer, but it's a much more basic instruction so
> > > hopefully the __builtin should work for that.
> > > 
> > Yes, but as I look at it, thats somewhat counter to my goal, which is to offer
> > accelerated code paths on systems that can make use of it at run time.  If We
> > use the __builtin compiler functions, we will either:
> > 
> > 1) Build those code paths with advanced instructions that won't work on older
> > systems (i.e. crash)
> > 
> > 2) Build those code paths with less advanced instructions, meaning that we won't
> > speedup execution on systems that are capable of using the more advanced
> > instructions.
> > 
> > Using this run time check, we can, at least in these situations, make use of the
> > accelerated paths when the instructions are available, and ignore them when
> > they're not, at run time.
> > 
> > What would be ideal, would be an alternative type macro, like the linux kernel
> > employs, but implementing that would require some pretty significant work and
> > testing.  This seems like a much simpler approach.
> > 
> 
> Ok, I understand where you are coming from indeed. However, within that,
> I'd like to see us reduce the amount of code that's needed for
> maintenance.
> 
Ok, but that seems orthogonal to what I've done here.  I've added about 10 lines
of easily understandable code, which seems reasonable to me.

> What we should really aim for, is to have common code, with perhaps some
> small ifdefs or __builtins, and then compile that code multiple times
> for multiple different architectures. So in this case, it would be nice
> to use the __builtin, and then compile that code up with and without SSE
> and select at runtime the code path to be used. Ideally, this could be
> done at the driver level.
> 
No, that is in direct conflict with what I'm trying to do here.  My goal is to
enable a build for a least common denominator system, but include those code
paths that allow some performance benefit on systems which support the feature,
to be determined at run time.  My goal is improving performance in the Fedora
dpdk package, which we build once for all supported systems on an arch.
Building multiple libraries for multiple system configurations is simply an
unmaintainable solution.

Now, a macro that selected an instruction optimized or generic path is fine, as
long as it can happen at run time.  The Linux kernel has such a feature, called
alternatives.  But its a complex subsystem that does run time replacement of
instructions based on cpu feature flags.  It would be great to have in the DPDK,
but its a significant code base and difficult to maintain, which goes against
your desire to reduce code.

> However, once you get down this path, you are dealing with more than
> just SSE. If I compile up the PMD on my system, which has a chip based
> on Sandy Bridge uarch, I find that there are multiple instructions
> starting with "vp" which means that they are actually AVX instructions.

You've done it wrong, you're building for the native machine target.  For fedora
I build for the default machine target (core2) which is the minimal system that
fedora supports.  So all the code emitted for the dpdk is functional for all
fedora systems.  This patch set then just enables the sse4.2 intrinsics for the
ixgbe vec rx/tx path and the acl library, and inculdes a run time check for
SSE4.2 before either of those paths can be used.

> Even though the code is written using intrinsics which correspond to SSE
> operations, the compiler is free to use AVX instructions where necessary
Not if you use the default machine target.

> to improve performance. Therefore, if we go down this road, we need to
> look to compile up the code for all microarchitectures, rather than just
> assuming that we will get equivalent performance to "native" by turning
> on the instruction set indicated by the primitives in the code. This is
No, you compile for the least common demonitor system, and enable more
performant paths opportunistically as run time checks allow.

> where having one codepath recompiled multiple times will work far better
> than having multiple code paths.
Only if you're only concern is performance.  As noted above, my goal is more
than just performance, its compatibility accross systems.  Multiple builds for
multiple cpu flag availability is simply a non-starter for a generic
distribution.

Regards
Neil