Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16659 )
Change subject: KUDU-3210 Add lock before verifying signature ...................................................................... Patch Set 14: (5 comments) http://gerrit.cloudera.org:8080/#/c/16659/8/src/kudu/security/crypto.cc File src/kudu/security/crypto.cc: http://gerrit.cloudera.org:8080/#/c/16659/8/src/kudu/security/crypto.cc@145 PS8, Line 145: #if OPENSSL_VERSION_NUMBER < 0x10100000L : std::unique_lock<OpenSSLMutex> l(mutex); : #endif > Thank you for putting together this stats! They are definitely helpful to Thanks for these benchmarks, Alexey! This does indeed seem concerning. Added the conditional locking as agreed on the call yesterday. Would you mind running the benchmarks with the current patch (with defensive locking disabled) on the same system just to make sure this patch doesn't affect performance in the common case? http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.h@202 PS13, Line 202: pper around > naming nit: it seems we predominantly use OpenSSL in other code around (e.g Done http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.cc@52 PS13, Line 52: in certain OpenSSL versions > nit: does it make sense to be more specific on the versions affected by the I was intentionally vague here to remain flexible. If someone's really curious, they can check the commit message for the motivation. http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.cc@372 PS13, Line 372: > Since the --openssl_defensive_locking is marked as a run-time flag, there i Done http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/tls_handshake.cc File src/kudu/security/tls_handshake.cc: http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/tls_handshake.cc@50 PS13, Line 50: anonymous > nit for here an other comments for closing braces for an anonymous namespac Done -- To view, visit http://gerrit.cloudera.org:8080/16659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifafc7dcf91db910123276b657515e410bb7f6fcd Gerrit-Change-Number: 16659 Gerrit-PatchSet: 14 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 03 Nov 2020 19:50:48 +0000 Gerrit-HasComments: Yes
