tls: update BoringSSL to c5ad6dcb (4491). #16104

PiotrSikora · 2021-04-21T21:13:55Z

Signed-off-by: Piotr Sikora piotrsikora@google.com

repokitteh-read-only · 2021-04-21T21:14:00Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #16104 was opened by PiotrSikora.

see: more, trace.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-04-22T09:55:21Z

@moderation This patch is lifted from https://boringssl-review.googlesource.com/c/boringssl/+/47084, so we have to carry it until it reaches Chromium Stable channel that we follow for BoringSSL.

moderation · 2021-04-22T18:21:19Z

@PiotrSikora can we add a comment and TODO explaining that

…-linux_arm64

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

htuch · 2021-04-23T00:20:58Z

@PiotrSikora ETA until it reaches Chromium stable?

PiotrSikora · 2021-04-23T00:36:00Z

@PiotrSikora ETA until it reaches Chromium stable?

Beta on 6/3, Stable on 7/20 (see: https://chromiumdash.appspot.com/schedule).

We flip back and forth between Stable and Beta, depending on QUICHE needs, so we could update it on 6/3 if you prefer to get rid of the patch before the release.

PiotrSikora · 2021-04-23T08:37:09Z

/retest

repokitteh-read-only · 2021-04-23T08:37:12Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #16104 (comment) was created by @PiotrSikora.

see: more, trace.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-04-23T11:52:17Z

/retest

repokitteh-read-only · 2021-04-23T11:52:21Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16104 (comment) was created by @PiotrSikora.

see: more, trace.

htuch · 2021-04-23T15:30:26Z

Does being on stable confer any security or stability benefit (yes, I know it's in the name, but beta means a lot of different things on a per project basis)?

PiotrSikora · 2021-04-23T15:47:59Z

Does being on stable confer any security or stability benefit (yes, I know it's in the name, but beta means a lot of different things on a per project basis)?

I don't believe so, the only advantage (for me) are the regular updates every 6 weeks (changing to every 4 weeks later this year) when following Chromium vs bumping BoringSSL dependency every day/week when following master.

As far as I recall, @mattklein123 always preferred following Chromium Stable because then it's a more widely tested version than a random commit from master.

moderation · 2021-04-23T17:26:49Z

See prior conversation about BoringSSL version at #15181 (comment). At that time we merged a version that was not stable so I think this is OK. As per prior comment I'd like to see a comment and TODO with how the patch gets fixed in the future and who owns the TODO. It makes tracking down a patch owner must easier

PiotrSikora · 2021-04-26T08:19:04Z

See prior conversation about BoringSSL version at #15181 (comment). At that time we merged a version that was not stable so I think this is OK.

Could you elaborate on what is OK? Do you mean that we should switch to following BoringSSL's master branch?

As per prior comment I'd like to see a comment and TODO with how the patch gets fixed in the future and who owns the TODO. It makes tracking down a patch owner must easier

I've added a comment already (see: https://github.com/envoyproxy/envoy/pull/16104/files#diff-0eb42e688409aad0b08239fc702eb26298453f95089f74d02a3cfc5ae91612a9R2).

That's a cherry-pick of a commit merged upstream, so it will be removed once we catch-up with the BoringSSL version that contains it. I can add more text if you can provide it, since I'm not sure what else you want me to add there.

moderation · 2021-04-26T15:55:59Z

Could you elaborate on what is OK? Do you mean that we should switch to following BoringSSL's master branch?

I was pointing out the we have used a pre-master branch of BoringSSL in the past but this PR doesn't change the dependency so my comment isn't relevant.

Your comment in the patch doesn't talk about what occurs to allow for removal of the patch and who has the TODO. The information in #16104 (comment) and #16104 (comment) provides a lot more context.

PiotrSikora · 2021-04-26T19:43:11Z

I was pointing out the we have used a pre-master branch of BoringSSL in the past but this PR doesn't change the dependency so my comment isn't relevant.

Oh, but it does... if we update BoringSSL to master, then we don't need to cherry-pick that commit and carry it as a patch, since it will be already included.

I think that decision is ultimately up to @envoyproxy/dependency-shepherds?

yanavlasov · 2021-04-30T00:08:12Z

Are we ok with this PR as it is? It is not clear from the comments.

PiotrSikora · 2021-04-30T00:24:42Z

I think we (or at least me) are waiting for the decision from @envoyproxy/dependency-shepherds whether we should follow BoringSSL master or version from the Chromium's Stable channel.

moderation · 2021-04-30T02:54:27Z

Confused too. From what I can see this PR changes a single file bazel/boringssl_static.patch. It doesn't change the BoringSSL version. However @PiotrSikora says it does change the BoringSSL version in #16104 (comment). Is there another PR associated with this single file PR? As per above I'm suggesting a simple TODO and comment saying what needs to happen to remove the patch and who owns the patch.

PiotrSikora · 2021-04-30T07:30:44Z

Confused too. From what I can see this PR changes a single file bazel/boringssl_static.patch. It doesn't change the BoringSSL version. However @PiotrSikora says it does change the BoringSSL version in #16104 (comment).

I never said that this (or any other) PR changes the version of BoringSSL.

I said that you comment (#16104 (comment)) is relevant, because we have 2 options to enable those optimizations:

Update BoringSSL to the master branch, which already includes the commit in question.
Carry that commit as a patch until it lands in the version of BoringSSL used in Chromium Stable.

This PR carries the patch (i.e. option (2)), but you've mentioned that using BoringSSL version different from Chromium Stable was fine in the past, so if we're fine with following master branch, then we can simply update BoringSSL instead of carrying this patch (i.e. option (1)), and I'm waiting for a confirmation that this is what you want to do.

moderation · 2021-04-30T23:50:52Z

I think I get it now. We really don't want to carry patches if possible and we would rather be on Chromium stable. However we are currently using a Chromium beta channel BoringSSL and the comment I linked to would indicate that using a closer to head BoringSSL should be OK.

Could we switch to the Chromium Dev channel and get the BoringSSL version we need to avoid the patch? https://chromiumdash.appspot.com/releases?platform=Linux ? The Dev channel has 2 92 releases already.

My vote is to go to Chromium Dev channel with no patch. If we don't want to proceed with the Dev channel my vote is to carry the patch until Beta arrives on 6/3. /cc @htuch

Definitely want to unblock this to get better aarch64 CPU performance

…-linux_arm64

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-05-01T03:42:13Z

@moderation done.

moderation · 2021-05-01T18:28:27Z

/lgtm deps

PiotrSikora · 2021-05-01T19:59:55Z

/retest

repokitteh-read-only · 2021-05-01T19:59:59Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16104 (comment) was created by @PiotrSikora.

see: more, trace.

Fixes envoyproxy#16105. Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 21, 2021

PiotrSikora force-pushed the boringssl-linux_arm64 branch from b1cf0c0 to ab03a3d Compare April 21, 2021 21:16

jensengrey mentioned this pull request Apr 21, 2021

Performance: Over a third of CPU usage is unaccelerated cryptographic operations on arm64 #16105

Closed

tls: include BoringSSL assembly optimizations on Linux-aarch64.

97e879d

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora force-pushed the boringssl-linux_arm64 branch from a6b4fe8 to 97e879d Compare April 22, 2021 00:04

PiotrSikora changed the title ~~tls: include BoringSSL assembly optimizations on Linux/aarch64.~~ tls: include BoringSSL assembly optimizations on Linux-aarch64. Apr 22, 2021

PiotrSikora marked this pull request as ready for review April 22, 2021 00:05

PiotrSikora requested review from moderation and htuch April 22, 2021 09:54

asraa assigned wrowe Apr 22, 2021

PiotrSikora added 3 commits April 22, 2021 22:42

Merge remote-tracking branch 'origin/main' into PiotrSikora/boringssl…

8eaffcb

…-linux_arm64

review: add comment about content of the patch.

1badc0a

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use better source.

7ba57b3

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora added 2 commits April 23, 2021 08:39

review: Kick CI.

8c3409a

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: Kick CI.

80a9f1e

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora added 3 commits April 30, 2021 20:34

Merge remote-tracking branch 'origin/main' into PiotrSikora/boringssl…

f2c815f

…-linux_arm64

review: revert patch.

4fa528d

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

tls: update BoringSSL to c5ad6dcb (4491).

7b46700

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora changed the title ~~tls: include BoringSSL assembly optimizations on Linux-aarch64.~~ tls: update BoringSSL to c5ad6dcb (4491). May 1, 2021

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label May 1, 2021

moderation approved these changes May 1, 2021

View reviewed changes

htuch approved these changes May 2, 2021

View reviewed changes

htuch merged commit 722f12a into envoyproxy:main May 2, 2021

gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021

tls: update BoringSSL to c5ad6dcb (4491). (envoyproxy#16104)

3df15d2

Fixes envoyproxy#16105. Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

moderation mentioned this pull request Jul 13, 2021

Slow performance on aarch64? #17313

Closed

lowjoel mentioned this pull request Mar 9, 2023

Documentation: Performance deltas between AWS Graviton2 (aarch64/neoverse-n1) vs Intel #26004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: update BoringSSL to c5ad6dcb (4491). #16104

tls: update BoringSSL to c5ad6dcb (4491). #16104

PiotrSikora commented Apr 21, 2021 •

edited

Loading

repokitteh-read-only bot commented Apr 21, 2021

PiotrSikora commented Apr 22, 2021

moderation commented Apr 22, 2021

htuch commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

repokitteh-read-only bot commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

repokitteh-read-only bot commented Apr 23, 2021

htuch commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

moderation commented Apr 23, 2021

PiotrSikora commented Apr 26, 2021

moderation commented Apr 26, 2021

PiotrSikora commented Apr 26, 2021

yanavlasov commented Apr 30, 2021

PiotrSikora commented Apr 30, 2021

moderation commented Apr 30, 2021 •

edited

Loading

PiotrSikora commented Apr 30, 2021

moderation commented Apr 30, 2021

PiotrSikora commented May 1, 2021

moderation commented May 1, 2021

PiotrSikora commented May 1, 2021

repokitteh-read-only bot commented May 1, 2021

tls: update BoringSSL to c5ad6dcb (4491). #16104

tls: update BoringSSL to c5ad6dcb (4491). #16104

Conversation

PiotrSikora commented Apr 21, 2021 • edited Loading

repokitteh-read-only bot commented Apr 21, 2021

PiotrSikora commented Apr 22, 2021

moderation commented Apr 22, 2021

htuch commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

repokitteh-read-only bot commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

repokitteh-read-only bot commented Apr 23, 2021

htuch commented Apr 23, 2021

PiotrSikora commented Apr 23, 2021

moderation commented Apr 23, 2021

PiotrSikora commented Apr 26, 2021

moderation commented Apr 26, 2021

PiotrSikora commented Apr 26, 2021

yanavlasov commented Apr 30, 2021

PiotrSikora commented Apr 30, 2021

moderation commented Apr 30, 2021 • edited Loading

PiotrSikora commented Apr 30, 2021

moderation commented Apr 30, 2021

PiotrSikora commented May 1, 2021

moderation commented May 1, 2021

PiotrSikora commented May 1, 2021

repokitteh-read-only bot commented May 1, 2021

PiotrSikora commented Apr 21, 2021 •

edited

Loading

moderation commented Apr 30, 2021 •

edited

Loading