Enable QAT support in zfs-dkms RPM #6932

daweiq · 2017-12-07T09:34:16Z

Description

Enable QAT accelerated gzip compression in zfs-dkms RPM package when
environment variant ICP_ROOT is set to QAT drive source code
folder and QAT hardware presence.
Otherwise, switch back to default gzip compression.

Motivation and Context

QAT accelerated gzip compression is only supported when user specific the QAT driver source code path in building zfs from source code.
By adding this new patch, ZFS user can enable QAT support by set ICP_ROOT to local QAT driver source code path before installing the ZFS-DKMS RPM package.

How Has This Been Tested?

Tested the ZFS-DKMS package on CentOS7 with/without ICP_ROOT is set.
Tested the ZFS-DKMS package on CentOS7 with/without QAT driver.
Tested the ZFS-kmod package on CentOS7 without QAT driver.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.
Change has been approved by a ZFS on Linux member.

gmelikov

error: missing "Signed-off-by"

Please, add Signed-off-by line to commit message.

Otherwise LGTM.

Enable QAT accelerated gzip compression in zfs-dkms RPM package when environment variant ICP_ROOT is set to QAT drive source code folder and QAT hardware presence. Otherwise, switch back to default gzip compression. Signed-off-by: David Qian <david.qian@intel.com>

behlendorf · 2017-12-08T02:11:41Z

scripts/dkms.mkconf

@@ -30,6 +30,7 @@ PRE_BUILD="configure
  --with-spl=\${source_tree}/spl-\${PACKAGE_VERSION}
  --with-spl-obj=\${dkms_tree}/spl/\${PACKAGE_VERSION}/\${kernelver}/\${arch}
  --with-spl-timeout=300
+  --with-qat=\${ICP_ROOT}


@daweiq is the QAT driver packaged using dkms? If so it would make sense to first check for an installed dkms version and if detected provide that to --with-qat and --with-qat-obj. Then additionally provide a way with an environment variable (or two) to specify a location. Please do this in a way such that -with-qat isn't passed unconditionally to configure when it's not available.

@behlendorf QAT driver only provides source code tar ball now. So, we cannot detect the QAT driver source code unless user specific it. After passing this environment variable to configure, the source file check inside configure file will validate the path and decide whether to include QAT or not.

Fair enough, then please just update the PR so the --with-qat is only provided to configure when ICP_ROOT is defined. You can use a modified versions of how --enable-debug is provided and check if the environment variable has a non-zero length.

Done, patch updated.

Enable QAT accelerated gzip compression in zfs-dkms RPM package when environment variant ICP_ROOT is set to QAT drive source code folder and QAT hardware presence. Otherwise, switch back to default gzip compression. Signed-off-by: David Qian <david.qian@intel.com>

daweiq · 2017-12-11T06:28:02Z

See the following comparing error when fetch the latest code from upstrea/master even after I revert all my patch. So, just keep my provide repo unchanged:

In file included from ../../module/zcommon/zfs_fletcher_avx512.c:29:0:
../../lib/libspl/include/sys/frame.h:33:14: error: conflicting types for 'greg_t'
typedef long greg_t;
^
In file included from /usr/include/signal.h:360:0,
from /usr/include/sys/param.h:28,
from ../../lib/libspl/include/sys/param.h:30,
from ../../lib/libspl/include/sys/types.h:100,
from ../../include/linux/simd_x86.h:75,
from ../../module/zcommon/zfs_fletcher_avx512.c:27:
/usr/include/sys/ucontext.h:31:37: note: previous declaration of 'greg_t' was here
extension typedef long long int greg_t;
^

codecov · 2017-12-11T06:59:56Z

Codecov Report

Merging #6932 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6932      +/-   ##
==========================================
- Coverage   75.34%   75.32%   -0.03%     
==========================================
  Files         296      296              
  Lines       95457    95457              
==========================================
- Hits        71922    71900      -22     
- Misses      23535    23557      +22

Flag	Coverage Δ
#kernel	`74.69% <ø> (-0.08%)`	⬇️
#user	`67.44% <ø> (-0.28%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 674b893...c85abb0. Read the comment docs.

codecov · 2017-12-11T11:32:21Z

Codecov Report

Merging #6932 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6932      +/-   ##
==========================================
+ Coverage   75.34%   75.34%   +<.01%     
==========================================
  Files         296      296              
  Lines       95457    95457              
==========================================
+ Hits        71922    71925       +3     
+ Misses      23535    23532       -3

Flag	Coverage Δ
#kernel	`74.5% <ø> (-0.27%)`	⬇️
#user	`67.66% <ø> (-0.06%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 674b893...c85abb0. Read the comment docs.

behlendorf

Minor nit, then LGTM.

behlendorf · 2017-12-11T18:13:56Z

scripts/dkms.mkconf

@@ -31,6 +31,12 @@ PRE_BUILD="configure
  --with-spl-obj=\${dkms_tree}/spl/\${PACKAGE_VERSION}/\${kernelver}/\${arch}
  --with-spl-timeout=300
  --with-qat=\${ICP_ROOT}
+  \$(
+    [[ -n \${ICP_ROOT} ]] && \\


Since ICP_ROOT may not be defined it should be quoted to avoid an error.

behlendorf · 2017-12-12T21:00:50Z

@daweiq yup, sorry about the unrelated build issue. Will fix.

Enable QAT accelerated gzip compression in zfs-dkms RPM package when environment variant ICP_ROOT is set to QAT drive source code folder and QAT hardware presence. Otherwise, use default gzip compression. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: David Qian <david.qian@intel.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #6932

Enable QAT accelerated gzip compression in zfs-dkms RPM package when environment variant ICP_ROOT is set to QAT drive source code folder and QAT hardware presence. Otherwise, use default gzip compression. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: David Qian <david.qian@intel.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#6932

gmelikov approved these changes Dec 7, 2017

View reviewed changes

behlendorf requested changes Dec 8, 2017

View reviewed changes

behlendorf approved these changes Dec 11, 2017

View reviewed changes

behlendorf closed this in f6940bb Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable QAT support in zfs-dkms RPM #6932

Enable QAT support in zfs-dkms RPM #6932

daweiq commented Dec 7, 2017

gmelikov left a comment

behlendorf Dec 8, 2017

daweiq Dec 8, 2017

behlendorf Dec 8, 2017

daweiq Dec 11, 2017

daweiq commented Dec 11, 2017

codecov bot commented Dec 11, 2017

codecov bot commented Dec 11, 2017

behlendorf left a comment

behlendorf Dec 11, 2017

behlendorf commented Dec 12, 2017

Enable QAT support in zfs-dkms RPM #6932

Enable QAT support in zfs-dkms RPM #6932

Conversation

daweiq commented Dec 7, 2017

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

gmelikov left a comment

Choose a reason for hiding this comment

behlendorf Dec 8, 2017

Choose a reason for hiding this comment

daweiq Dec 8, 2017

Choose a reason for hiding this comment

behlendorf Dec 8, 2017

Choose a reason for hiding this comment

daweiq Dec 11, 2017

Choose a reason for hiding this comment

daweiq commented Dec 11, 2017

See the following comparing error when fetch the latest code from upstrea/master even after I revert all my patch. So, just keep my provide repo unchanged:

codecov bot commented Dec 11, 2017

Codecov Report

codecov bot commented Dec 11, 2017

Codecov Report

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Dec 11, 2017

Choose a reason for hiding this comment

behlendorf commented Dec 12, 2017