SHA256 QAT acceleration #7295

tcaputi · 2018-03-12T19:31:34Z

This patch enables acceleration of SHA256 and SHA512
checksums using Intel Quick Assist Technology.

Signed-off-by: Tom Caputi tcaputi@datto.com

How Has This Been Tested?

Tested on an Ubuntu 16.04 machine with a DH895XCC QAT add-in card. I will have performance numbers soon.

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.

behlendorf

Logistically I can see why you opted to put the qat_checksum() functionality in to qat_crypt.c since it makes it easy to share the instance handles. However, it's frankly not very intuitive.

It looks like it would be relatively straight forward to move the common instance management code in to qat.c and then provide a helper function to claim an instance. Then you could split the new checksum functionality in to qat_checksum.c. Or did you already try this and run in to complications?

behlendorf · 2018-03-12T20:28:33Z

module/zfs/qat.c

@@ -99,4 +102,7 @@ qat_fini(void)
 	qat_dc_fini();
 }

+module_param(zfs_qat_disable, int, 0644);
+MODULE_PARM_DESC(zfs_qat_disable, "Disable QAT acceleration");


Is there any value in making this more granual (i.e discard checksum, crypt, or compress)? Or can this already be effectively controlled with the QAT config file?

This can be done via the config files, but I agree it would be nice to be able to disable each one (for instance if you had another application that you wanted to use the compression units). I will make the change.

behlendorf · 2018-03-12T20:33:08Z

module/zfs/qat_crypt.c

+	if (status != CPA_STATUS_SUCCESS) {
+		/* don't count CCM as a failure since it's not supported */
+		if (zio_crypt_table[crypt].ci_crypt_type == ZC_TYPE_GCM)
+			QAT_STAT_BUMP(crypt_fails);


Let's either put this little fix in a seperate PR, or at least call it out in the commit comment.

behlendorf · 2018-03-12T20:42:51Z

module/zfs/sha256.c

 	SHA2_CTX ctx;
 	zio_cksum_t tmp;

+	if (qat_crypt_use_accel(size)) {


Let's add a qat_checksum_use_accel wrapper to make this interface less aukward.

behlendorf · 2018-03-12T20:47:26Z

module/zfs/qat_crypt.c

@@ -192,7 +193,49 @@ init_cy_session_ctx(qat_encrypt_dir_t dir, CpaInstanceHandle inst_handle,
 }

 static CpaStatus
-init_cy_buffer_lists(CpaInstanceHandle inst_handle, uint32_t nr_bufs,
+qat_init_cksum_session_ctx(CpaInstanceHandle inst_handle,


nit: qat_init_cksum_session_ctx -> qat_init_checksum_session_ctx would be consistent with the other function names.

tcaputi · 2018-03-13T00:16:16Z

It looks like it would be relatively straight forward to move the common instance management code in to qat.c and then provide a helper function to claim an instance. Then you could split the new checksum functionality in to qat_checksum.c. Or did you already try this and run in to complications?

The only thing about this was that it felt awkward that more than half of the functions in qat.c would only pertain to qat_checksum.c and qat_crypt.c, but not qat_compress.c. If I was trying to put this into OOP terms, the "base class" qat.c would contain functionality that could not be used for all of its "inheriting classes", if that makes any sense.

If you still think its appropriate, I can split it out as you requested.

tcaputi · 2018-03-13T09:54:41Z

@wli5 has informed me that the SHA512 implementation needs to be removed, since technically we actually use SHA512/256, which uses a different IV than standard SHA512 and is not supported by QAT. I will remove support for it tomorrow and do a complete test to verify compatibility.

rlaager · 2018-03-13T09:59:54Z

Is there a QAT card in a machine running as part of the test suite? If not, has anyone asked Intel if they'd be willing to donate a card for that?

tcaputi · 2018-03-13T12:17:44Z

@rlaager I think my company would be happy to purchase / donate a card to the cause. The only issue would be that I believe all the testing infrastructure runs in AWS, and I don't think we can just pop the card into the cloud :). I definitely plan to completely verify this patch and the performance benefits before it gets merged. Perhaps I should have put a WIP tag on it when I posted it. I Any thoughts on testing going forward @behlendorf?

tcaputi · 2018-03-13T13:46:07Z

Some performance testing on my machine (single spinning disk pool running yes | pv > /pool/test/yes.txt) showed CPU utilization reduction of 28% when using QAT checksumming. Disk bandwidth also went from 191MB/s to 204 MB/s.

behlendorf · 2018-03-13T21:02:17Z

The only thing about this was that it felt awkward that more than half of the functions in qat.c would only pertain to qat_checksum.c and qat_crypt.c, but not qat_compress.c. If I was trying to put this into OOP terms, the "base class" qat.c would contain functionality that could not be used for all of its "inheriting classes", if that makes any sense.

I can understand that argument. Or you could view the qat.c code as a library of which you're only using a subset of its functionality. Alternately, I'd be OK with the proposed organization if we renamed qat_crypt.c to something like qat_inst.c to broaden its scope. Otherwise, it feels to me like we're hiding this accelerted checksum code in the encryption code.

As for testing @tcaputi is right, since all the automated testing is all in AWS it's a bit tricky to add the QAT hardware for testing. We could probably add/modify a builder to verify that everything builds when the QAT source is enabled, but actually testing with real hardware is a bit more difficult. Also since we don't expect this code to change much once all this functionality is working it's probably not critical and we can rely on manual testing for now.

codecov · 2018-03-14T01:04:07Z

Codecov Report

Merging #7295 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7295      +/-   ##
==========================================
+ Coverage   76.36%    76.4%   +0.03%     
==========================================
  Files         328      328              
  Lines      104023   104008      -15     
==========================================
+ Hits        79439    79465      +26     
+ Misses      24584    24543      -41

Flag	Coverage Δ
#kernel	`76.44% <ø> (+0.38%)`	⬆️
#user	`65.67% <ø> (+0.22%)`	⬆️

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 de4f8d5...d3cebb2. Read the comment docs.

wli5 · 2018-03-14T02:45:07Z

module/zfs/sha256.c

+		ret = qat_checksum(ZIO_CHECKSUM_SHA256, buf, size, &tmp);
+		abd_return_buf(abd, buf, size);
+		if (ret == CPA_STATUS_SUCCESS)
+			goto bswap;


for SHA512, we should just fall back to software, instead of jumping to bswap?

Not sure what you mean here. The goto bswap here is meant to handle a strange implementation detail that ZFS always stores SHA256 checksums in big endian order. For SHA512 we don't do this.

If we jump to bswap the software implementation will be skipped for SHA512.

We only do the jump in the event that QAT checksumming is enabled and if the QAT implementation worked. Is that correct?

Im also a little confused with what you mean by "software implementation of SHA512" I removed the QAT implementation of SHA512 since ZFS actually uses SHA512/256, as you pointed out.

Sorry for confusion.. I just relook at the code, it is good.
I have thought SHA512 shared the same function, I just noticed there is another sha512 function which is not changed and will be always in "software" mode.
Just ignore this comment. thanks!

wli5 · 2018-03-14T03:53:38Z

module/zfs/qat_crypt.c

+	flat_src_buf = flat_src_buf_array;
+	while (bytes_left > 0) {
+		in_pages[page_num] = qat_mem_to_page(data);
+		flat_src_buf->pData = kmap(in_pages[page_num]);


There is a problem here when the src buffer is in 4K page boundary, the flat_src_buf->pData need to add the offset of the src address in the page frame. same problem is also in compression code, I will submit a separate PR to fix it for compression, can you please fix it for checksum and encryption code in this PR?

Or does it make sense if you can fix the compression code also in this PR? It will be great if we can do it together.

I can add it to this PR. Can you give me an example of how to do this so I'm sure I'm doing it right?

Here is the example:
flat_buf_src->pData = kmap(in_page) + ((long)data & ~PAGE_MASK);

Would an adjustment need to be made to flat_src_buf->dataLenInBytes as well? I'm assuming this applies to encryption, checksumming, and compression?

Thanks for reminder, yes, the dataLenInBytes need to be adjusted as well, and it applies to encryption, checksum and compression.

The fix for this has been pushed. I will make a separate PR for fixing compression later today.

This patch enables acceleration of SHA256 checksums using Intel Quick Assist Technology. This patch also fixes up and refactors some of the code from QAT encryption to make the behavior consistent. Signed-off-by: Chengfeix Zhu <chengfeix.zhu@intel.com> Signed-off-by: Weigang Li <weigang.li@intel.com> Signed-off-by: Tom Caputi <tcaputi@datto.com>

wli5 · 2018-03-19T08:46:38Z

@behlendorf @tcaputi @rlaager regarding to the auto test for QAT, we plan to setup a daily test environment in our lab on a machine with QAT plug-in, hopefully this way we can catch any issue related to QAT timingly.

tcaputi force-pushed the qat branch from d7336e7 to 5fe614d Compare March 12, 2018 19:33

behlendorf requested changes Mar 12, 2018

View reviewed changes

tcaputi force-pushed the qat branch 2 times, most recently from 89de5d2 to e8e169b Compare March 13, 2018 13:26

tcaputi force-pushed the qat branch from e8e169b to 7e43fd5 Compare March 14, 2018 00:08

wli5 reviewed Mar 14, 2018

View reviewed changes

tcaputi force-pushed the qat branch from 7e43fd5 to d3cebb2 Compare March 14, 2018 14:43

tcaputi changed the title ~~SHA256 / SHA512 QAT acceleration~~ SHA256 QAT acceleration Mar 14, 2018

behlendorf approved these changes Mar 14, 2018

View reviewed changes

behlendorf merged commit 3874220 into openzfs:master Mar 15, 2018

geppi mentioned this pull request Jan 21, 2019

Intel QAT support when root is on a ZFS filesystem #8323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SHA256 QAT acceleration #7295

SHA256 QAT acceleration #7295

tcaputi commented Mar 12, 2018

behlendorf left a comment •

edited

Loading

behlendorf Mar 12, 2018

tcaputi Mar 13, 2018

behlendorf Mar 12, 2018

behlendorf Mar 12, 2018

behlendorf Mar 12, 2018

tcaputi commented Mar 13, 2018

tcaputi commented Mar 13, 2018

rlaager commented Mar 13, 2018

tcaputi commented Mar 13, 2018

tcaputi commented Mar 13, 2018

behlendorf commented Mar 13, 2018

codecov bot commented Mar 14, 2018 •

edited

Loading

wli5 Mar 14, 2018

tcaputi Mar 14, 2018

wli5 Mar 14, 2018

tcaputi Mar 14, 2018

tcaputi Mar 14, 2018

wli5 Mar 14, 2018

wli5 Mar 14, 2018

wli5 Mar 14, 2018

tcaputi Mar 14, 2018

wli5 Mar 14, 2018

tcaputi Mar 14, 2018

wli5 Mar 14, 2018

tcaputi Mar 14, 2018

wli5 commented Mar 19, 2018

SHA256 QAT acceleration #7295

SHA256 QAT acceleration #7295

Conversation

tcaputi commented Mar 12, 2018

How Has This Been Tested?

Types of changes

Checklist:

behlendorf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcaputi commented Mar 13, 2018

tcaputi commented Mar 13, 2018

rlaager commented Mar 13, 2018

tcaputi commented Mar 13, 2018

tcaputi commented Mar 13, 2018

behlendorf commented Mar 13, 2018

codecov bot commented Mar 14, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli5 commented Mar 19, 2018

behlendorf left a comment •

edited

Loading

codecov bot commented Mar 14, 2018 •

edited

Loading