Skip to content

Conversation

@changchengx
Copy link

Ceph need to deal with the case when the input buffer is NULL:
https://github.com/ceph/ceph/blob/master/src/test/common/test_crc32c.cc#L347

Signed-off-by: Changcheng Liu [email protected]

@changchengx
Copy link
Author

@tchaikov Please help review.
We're going to use isa-l's crc32 to replace with src/common/crc32c_intel_fast assembler implementation

Copy link
Collaborator

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of forking intel/isa-l, i am inclined to bump up ceph/isa-l's master to track the upstream if it is not updated, i am afraid it's not the case. i think this source file is almost identical to https://github.com/intel/isa-l/blob/8074e3fe1b9398a9d3b717267790050fc5041594/crc/crc32_iscsi_00.asm . can we just use it instead?

Ceph need to deal with the case when the input buffer is NULL:
https://github.com/ceph/ceph/blob/master/src/test/common/test_crc32c.cc#L347

Signed-off-by: Changcheng Liu <[email protected]>
@changchengx
Copy link
Author

@tchaikov
I'm raising a similar question at: intel#159

@tchaikov
Copy link
Collaborator

@aclamk do you happen to know if we have any real-world use case of https://github.com/ceph/ceph/blob/master/src/test/common/test_crc32c.cc#L358 ? or it's created this way just because it was more convenient than creating a buffer filled with zero?

@aclamk
Copy link

aclamk commented Oct 29, 2020

@tchaikov

aclamk do you happen to know if we have any real-world use case of https://github.com/ceph/ceph/blob/master/src/test/common/test_crc32c.cc#L358 ? or it's created this way just because it was more convenient than creating a buffer filled with zero?

This code is testing performace of special crc function that calculates crc from zeros.
We are using this shortcut for caching crc values: https://github.com/ceph/ceph/blob/master/src/common/buffer.cc#L1975

@tchaikov
Copy link
Collaborator

thanks @aclamk . now i understand it better. seems we do need to have a dedicate function for calculating the crc of zero-filled buffers.

@tchaikov
Copy link
Collaborator

tchaikov commented Oct 30, 2020

@changchengx i feel that it could be function which can also benefit other projects interested in high-performance CRC32 calculating using the technique of cached/precalculated CRC32. if this makes sense to you, probably we can upstream this change? before that, i'd suggest create a dedicated function and keep it in Ceph.

what do you think?

@changchengx
Copy link
Author

@tchaikov

benefit other projects interested in high-performance CRC32 calculating using the technique of cached/precalculated CRC32.

I haven't understood this well.
I'll feedback to you later after checking https://github.com/ceph/ceph/blob/master/src/common/buffer.cc#L1975

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants