cryptomb: add queue size statistics by VillePihlava · Pull Request #19180 · envoyproxy/envoy

VillePihlava · 2021-12-03T07:54:50Z

Commit Message:

Add queue size statistics to CryptoMb private key provider. The queue size statistics show the amount of simultaneous crypto operations handled at the time of a SIMD (single instruction, multiple data) operation. The statistics consist of a histogram that indicates how often different queue sizes have been processed.

Additional Description:

CryptoMb performance depends on the amount of simultaneous crypto operations processed. Statistics show how often different queue sizes are processed. The bigger the queue size, the better the throughput. The statistics help with debugging performance issues, finding good poll delay values, and show the distribution of different queue sizes.

Risk Level: Low
Testing: Unit testing, manual testing
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: CryptoMb private key provider requires Intel 3rd generation Xeon Scalable server processor for the AVX-512 IFMA instruction set.

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

repokitteh-read-only · 2021-12-03T07:54:57Z

Hi @VillePihlava, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19180 was opened by VillePihlava.

see: more, trace.

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

ggreenway

This would make more sense as a histogram; you're basically re-implementing a histogram in this code. cc @jmarantz

jmarantz

looks great generally, with one concern/question about how frequently we are going to be doing stats symbol-table lookups, and a few style/readability nits.

jmarantz · 2021-12-03T19:18:12Z

contrib/cryptomb/private_key_providers/source/cryptomb_private_key_provider.cc

    : us_(std::chrono::duration_cast<std::chrono::microseconds>(poll_delay)), type_(type),
-      key_size_(keysize), ipp_(ipp),
-      timer_(d.createTimer([this]() -> void { processRequests(); })) {
+      key_size_(keysize), ipp_(ipp), timer_(d.createTimer([this]() -> void { processRequests(); })),


you don't need the -> void

Removed -> void from two places.

jmarantz · 2021-12-03T19:19:07Z

contrib/cryptomb/private_key_providers/source/cryptomb_private_key_provider.cc

 void CryptoMbQueue::processRequests() {
  if (type_ == KeyType::Rsa) {
+    // Increment correct queue size statistic.
+    stats_.getQueueSizeCounters()[request_queue_.size() - 1].get().inc();


s/getQueueSizeCounters/queueSizeCounters/ per convention

Removed after changing counters to histogram.

jmarantz · 2021-12-03T19:20:37Z

contrib/cryptomb/private_key_providers/source/cryptomb_private_key_provider.cc

-      tls_(ThreadLocal::TypedSlot<ThreadLocalData>::makeUnique(factory_context.threadLocal())) {
+      tls_(ThreadLocal::TypedSlot<ThreadLocalData>::makeUnique(factory_context.threadLocal())),
+      stats_(factory_context.scope(), CryptoMbQueue::MULTIBUFF_BATCH, "cryptomb",
+             "rsa_queue_size_") {


I assume this does not get called in response to requests, right? this happens at config-time?

If this does happen in response to requests then we should pre-symbolize the stat names and I can help you solve that without taking symbol-table locks. If this is called on config it's ok. But given it's not obvious how often this gets created, we should either comment on that assumption, or we should just go ahead and pre-symbolize all the names during config or (better yet) during startup.

This happens at config-time, I added a comment about it.

jmarantz · 2021-12-03T21:32:18Z

contrib/cryptomb/private_key_providers/test/ops_test.cc

@@ -382,8 +407,11 @@ TEST(CryptoMbProviderTest, TestRSATimer) {
  const BIGNUM *e, *n, *d;
  RSA_get0_key(rsa, &n, &e, &d);
  fakeIpp->setRsaKey(n, e, d);


this is pre-existing but the above 2 lines disturb me. They occur here and 4x in the prod code that I can see in this PR.

Can we have a helper API FakeIppCryptoImpl::setRsaKey(rsa); which captures the calls to RSA_get0_key and this->setRsaKey but initializes the 3 pointers to nullptr, and changes the 4 call-sites I can see in this PR and any others I can't see? :)

Refactored tests in ops_test.cc and changed this.

jmarantz · 2021-12-05T18:06:32Z

contrib/cryptomb/private_key_providers/source/cryptomb_stats.cc

+CryptoMbStats::CryptoMbStats(Stats::Scope& scope, uint32_t max_queue_size,
+                             absl::string_view stats_prefix,
+                             absl::string_view queue_size_stat_prefix)
+    : stat_name_pool_(scope.symbolTable()), stats_prefix_(stat_name_pool_.add(stats_prefix)) {


stats_prefix_ is not referenced after the constructor so you can drop the member variable and make this a local.

This does not exist anymore after the histogram change.

jmarantz · 2021-12-05T18:10:27Z

contrib/cryptomb/private_key_providers/test/ops_test.cc

+                      stats);
+
+  size_t in_len = 32;
+  uint8_t in[32] = {0x7f};


there's a lot of repeated blocks of code in this test file. WDYT of factoring some of those out into methods/members of a test class for CryptoMbProviderTest, and shortening all these tests to hopefully make them more readable?

Then we can document some of the code that's mysterious like what this 0x7f is supposed to represent, or why in is 32 bytes long.

Refactored tests in ops_test.cc and added comments.

jmarantz · 2021-12-05T18:29:09Z

/wait

jmarantz · 2021-12-05T18:30:54Z

RE @ggreenway 's comment about histograms: I agree that would be a lot better, and that changes a few of my suggestions also. One thing to note is that the XDR histograms auto-decide buckets and I'm not sure if you need something more precise for this. I'm guessing auto-bucketed histograms would be fine.

VillePihlava · 2021-12-07T07:02:39Z

Thanks for the comments!

@jmarantz @ggreenway I tried a histogram for this at one point, but the mentioned auto-decided buckets were confusing. Keeping each queue size as a separate counter shows the exact kind of queue sizes that come up. For example, the information on how frequently the biggest queue size comes up can be especially useful. However, I do agree that the histogram would be a better solution if it worked this way.

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

ggreenway · 2021-12-13T18:53:20Z

There are two things happening with histograms: the internal storage auto-decides buckets (but is probably quite accurate). And the display from /stats displays a configured (not auto-determined) set of buckets. There is a default set of buckets configured, but that can be specified in the bootstrap config under stats_config. Also, some stat sinks (such as statsd) don't use either of those mechanisms, and send the raw data.

Can you clarify what the data looked like when you tried with a histogram? It may have just been that you need to configure the displayed buckets to get the results you need.

/wait-any

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava · 2021-12-16T12:26:49Z

@ggreenway Thank you for the explanation! I changed the counters back to a histogram and the statsd solution worked as you explained.

My initial confusion came from the way the histogram stats are displayed from the /stats endpoint, something like this:

listener.0.0.0.0_9000.cryptomb.rsa_queue_sizes: P0(nan,1.0) P25(nan,8.024987213802275) P50(nan,8.049991475868183) P75(nan,8.074995737934092) P90(nan,8.089998295173636) P95(nan,8.094999147586819) P99(nan,8.098999829517364) P99.5(nan,8.099499914758681) P99.9(nan,8.099899982951737) P100(nan,8.1)

The /stats/prometheus endpoint is a bit more helpful for seeing individual queue sizes, but the buckets only have an upper bound and include the queue sizes below it:

envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="2"} 1
envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="3"} 1
envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="4"} 2
envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="5"} 3
envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="6"} 4
envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="7"} 9
envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="8"} 10
envoy_listener_cryptomb_rsa_queue_sizes_bucket{envoy_listener_address="0.0.0.0_9000",le="9"} 58667

When this is in the config:

stats_config:
  histogram_bucket_settings:
    match:
      contains: "cryptomb"
    buckets: [2,3,4,5,6,7,8,9]

Would there be an easy way to see individual queue sizes for this histogram, for example, from the /stats endpoint?

ggreenway · 2021-12-16T22:28:06Z

Would there be an easy way to see individual queue sizes for this histogram, for example, from the /stats endpoint?

I don't think there is currently. The base /stats endpoint just prints the summary that you see. That could be an interesting feature though, via something like /stats?histogram_buckets=1 or something and print out the bucket counts. It wouldn't be hard to implement (see how it's done in the prometheus exporter; would probably be a handful of lines of code).

jmarantz

looks great; just a few nits. Will you follow-up with another PR to add detail to the /stats endpoint for histograms?

jmarantz · 2021-12-21T15:23:55Z

contrib/cryptomb/private_key_providers/test/ops_test.cc

+  ssl_private_key_result_t res_;
+
+  // A size for signing and decryption operation input chosen for tests.
+  static constexpr size_t IN_LEN = 32;


Const member variables follow member-variable naming convention, not macro convention, so call these in_len_, in_, etc.

jmarantz · 2021-12-21T15:24:33Z

contrib/cryptomb/private_key_providers/test/ops_test.cc

+  // Size of output in out_ from an operation.
+  size_t out_len_ = 0;
+
+  const std::string QUEUE_SIZE_HISTOGRAM_NAME = "cryptomb.rsa_queue_sizes";


Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava · 2021-12-22T07:19:48Z

I can start looking into a follow-up PR on the /stats endpoint.

jmarantz

Can you add an issue for the /stats histogram detail follow-up? I'll assign it to you.

I'll pass over to Greg for a senior maintainer pass.

ggreenway

LGTM

VillePihlava · 2022-01-18T15:23:19Z

The linked pull request #19586 is the follow-up to the discussion in this pull request.

Add queue size statistics to CryptoMb private key provider. The queue size statistics show the amount of simultaneous crypto operations handled at the time of a SIMD (single instruction, multiple data) operation. The statistics consist of a histogram that indicates how often different queue sizes have been processed. Signed-off-by: Ville Pihlava <ville.pihlava@intel.com> Signed-off-by: Josh Perry <josh.perry@mx.com>

Add cryptomb queue size statistics.

5ca6d3c

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava requested a review from rojkov as a code owner December 3, 2021 07:54

Add missing newlines.

30768ff

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

ggreenway requested changes Dec 3, 2021

View reviewed changes

ggreenway assigned jmarantz Dec 3, 2021

jmarantz reviewed Dec 5, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 5, 2021

VillePihlava added 5 commits December 6, 2021 23:28

Remove unnecessary -> void.

b2df330

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change getQueueSizeCounters to queueSizeCounters.

2e51af7

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Add comment.

e634b2e

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Refactor setRsaKey and initialize BIGNUM pointers to nullptr.

1728354

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change stats_prefix_ to local variable and remove unnecessary comments.

3a86b6d

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

repokitteh-read-only bot added waiting:any and removed waiting labels Dec 13, 2021

VillePihlava added 7 commits December 14, 2021 04:02

Refactor tests.

95d11b6

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Fix format.

2f0528f

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change queue size counters to histogram.

7d784fe

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change stats test to work with histogram.

024ab51

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Add callbacks back.

c790431

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Add sub test classes and fix format.

a6c3626

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change comment and fix format.

a906fee

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

repokitteh-read-only bot removed the waiting:any label Dec 16, 2021

jmarantz reviewed Dec 21, 2021

View reviewed changes

Fix variable names.

3fe1d40

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

jmarantz approved these changes Jan 4, 2022

View reviewed changes

jmarantz assigned ggreenway Jan 4, 2022

VillePihlava mentioned this pull request Jan 4, 2022

Add detail about histogram buckets to the /stats endpoint #19378

Closed

ggreenway approved these changes Jan 4, 2022

View reviewed changes

ggreenway merged commit 5fd31ab into envoyproxy:main Jan 4, 2022

This was referenced Feb 17, 2022

stats: add ability to view histogram buckets to the /stats endpoint #19586

Merged

Configurable histogram buckets without using config #20088

Closed

Conversation

VillePihlava commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 3, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Dec 5, 2021

Uh oh!

jmarantz commented Dec 5, 2021

Uh oh!

VillePihlava commented Dec 7, 2021

Uh oh!

ggreenway commented Dec 13, 2021

Uh oh!

VillePihlava commented Dec 16, 2021

Uh oh!

ggreenway commented Dec 16, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VillePihlava commented Dec 22, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

VillePihlava commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VillePihlava commented Dec 3, 2021 •

edited

Loading

jmarantz Dec 3, 2021 •

edited

Loading