New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

erasure-code: Remove duplicate of isa-l files #15372

Merged
merged 1 commit into from Jun 1, 2017

Conversation

Projects
None yet
3 participants
@ganeshmaharaj
Contributor

ganeshmaharaj commented May 30, 2017

There are two copies of isa-l. While one is a git submodule the other
was a static copy from an older version. This change helps with removing
the duplicate and maintain a single copy.

Signed-off-by: Ganesh Mahalingam ganesh.mahalingam@intel.com

@ganeshmaharaj

This comment has been minimized.

Contributor

ganeshmaharaj commented May 31, 2017

@dachary your thoughts on this patch will be much appreciated.

@ghost ghost added cleanup core labels May 31, 2017

@ghost

@ganeshmaharaj good move :-) It looks like the two versions are not exactly the same so it's an upgrade, right ? Since it passes the erasure-code-corpus test, it shows there is no change in how data is encoded / decoded. It would be good to explain from which version to which version the upgrade is, in the commit message, for clarity.

@ghost

This comment has been minimized.

ghost commented May 31, 2017

For the record:

144/173 Test   #2: erasure-decode-non-regression.sh ........   Passed  538.24 sec
erasure-code: Remove duplicate of isa-l files
There are two copies of isa-l. While one is a git submodule the other
was a static copy from an older version. This change helps with removing
the duplicate and maintain a single copy.
This is also upgrading isa-l used by (src/erasure-code/isa/isa-l) from
v2.14 to v2.16.0

Signed-off-by: Ganesh Mahalingam <ganesh.mahalingam@intel.com>
@ganeshmaharaj

This comment has been minimized.

Contributor

ganeshmaharaj commented May 31, 2017

Thanks @dachary. Have updated the commit msg and pushed a new version of the patch. I believe i should have addressed the change request in the previous patch before force-pushing the change. Apologies if i have borked the normal workflow.

@ghost ghost added the needs-qa label May 31, 2017

@ghost

ghost approved these changes May 31, 2017

@ghost

This comment has been minimized.

ghost commented May 31, 2017

@ganeshmaharaj you did good, thanks :-)

@ganeshmaharaj

This comment has been minimized.

Contributor

ganeshmaharaj commented May 31, 2017

@dachary on a side note. isa-l library has had more releases since the last time ceph/isa-l has had 2 more releases. Would this be a nice change to upgrade the base also or would you prefer to keep it as a seperate patch if we wish to get that as well. Here are the release notes from v2.18 https://github.com/01org/isa-l/blob/7e1a337433a340bc0974ed0f04301bdaca374af6/Release_notes.txt

@ghost

This comment has been minimized.

ghost commented May 31, 2017

@ganeshmaharaj at first glance I don't see something from the release notes that would benefit Ceph directly. Do you have something specific in mind ?

@ganeshmaharaj

This comment has been minimized.

Contributor

ganeshmaharaj commented May 31, 2017

@dachary Nothing pressing that will need us to update right away. maybe performance improvements as mentioned here https://github.com/01org/isa-l/blob/7e1a337433a340bc0974ed0f04301bdaca374af6/Release_notes.txt#L53-L77 but I will need to test it before i can say in confidence that the upgrade will help. Let me do that and if it is worth it, will do a seperate PR for it.

@tsg-

This comment has been minimized.

tsg- commented May 31, 2017

@dachary ISA-L 2.18 add compression enhancements related to avx512 that should be useful for bluestore block compression.

Also @ganeshmaharaj, let's add new avx512 related files from the ISA-L EC sources and then merge this. I have some pending changes that I will push to your branch and then resubmit this PR?

@ghost

This comment has been minimized.

ghost commented May 31, 2017

@tsg- that's a compelling argument :-) Would you mind doing the upgrade as another pull request ? The implications are beyond the scope of EC and will need review from other parties.

@tsg-

This comment has been minimized.

tsg- commented May 31, 2017

@dachary make sense - we'll handle v2.18 upgrade as a separate PR. I will only add the missing files from v2.16 to this PR and resubmit - these files are related to avx512 improvements that were part of v2.16 (already the version the ISA-L git submodule is at) and offer upto 200% improvements in EC performance. Is that OK?

@ghost

This comment has been minimized.

ghost commented May 31, 2017

Ok, I'll take a look to better undrestand why they are needed.

@liewegas liewegas merged commit d3887bf into ceph:master Jun 1, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@ganeshmaharaj ganeshmaharaj deleted the ganeshmaharaj:reuse-isal branch Jun 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment