Consider directory connections zlib buffers when handling OOM

Our current OOM handler code looks at edge connections and circuit queues when deciding whether to declare OOM and when finding circuits to kill. It should also consider killing directory connections, and consider the total allocation for zlib compression objects.

See also #11791 (moved)

changed milestone to %Tor: 0.2.6.x-final

added 026-triaged-1 component::core tor/tor dir milestone::Tor: 0.2.6.x-final oom priority::medium resolution::implemented status::closed tor-relay type::enhancement zlib labels

Trac:
Keywords: tor-relay dir zlib oom deleted, oom, dir, zlib, tor-relay, 026-triaged-1 added

My branch bug11792_1 should do this stuff. Could use some preliminary review.

Trac:
Status: new to needs_review

Prelim review.

conn_get_buffer_age:

• You can probably drop the age2 from the conn->outbuf conditional block, now that this logic is in its own function

  if (conn->outbuf) {
-    age2 = buf_get_oldest_chunk_timestamp(conn->outbuf, now);
+    age = buf_get_oldest_chunk_timestamp(conn->outbuf, now);
-    if (age2 > age)
-      age = age2;
  }

tor_zlib_state_size_precalc:

• Without knowing the preexisting code and structs it's is difficult to understand. Reading #11791 (moved) helps. Some of the numbers are obvious, but adding a descriptive comment to the return statement explaining the components would also help a lot.

Other than that, this looks sane.

Begin code review:

• ec59167cae1f5b3057ed722857d78ec78239e991:

  • This looks fine to me, I think. I presume you've checked that we're not at risk of multiple-counting by also freeing the linked conn later and adjusting for its memory usage again?

• c5260f5b78c434a223dc82fd3153cd9c8a4f63bf:

  • I think this is okay.

• 55556568283d44219e8439d0dea7d01bb182623d:

  • I think this looks reasonable, but conn_get_buffer_age() defined the age of a circuit with nothing buffered in either direction as zero isn't totally intuitive to me. It leads to reasonable behavior, but maybe a sentence explicitly stating that it does that in its comment is in order?

• 58020ed079af3794a0869f4a6bfa34ccaecc4b61:

  • This looks fine to me.

End code review.

Replying to andrea:

Begin code review:

• ec59167cae1f5b3057ed722857d78ec78239e991:
  • This looks fine to me, I think. I presume you've checked that we're not at risk of multiple-counting by also freeing the linked conn later and adjusting for its memory usage again?

The linked conn can't be another stream ... and we don't consider linked directory connections later on, only the linked ones. So I think it can't be double-counted.

• c5260f5b78c434a223dc82fd3153cd9c8a4f63bf:

  • I think this is okay.

• 55556568283d44219e8439d0dea7d01bb182623d:

  • I think this looks reasonable, but conn_get_buffer_age() defined the age of a circuit with nothing buffered in either direction as zero isn't totally intuitive to me. It leads to reasonable behavior, but maybe a sentence explicitly stating that it does that in its comment is in order?

Will do.

• 58020ed079af3794a0869f4a6bfa34ccaecc4b61:
  • This looks fine to me.

End code review.

Thanks!

Pushed two extra commits to bug11792_1. Squashed the branch as bug11792_1_squashed, then merged it into master as 59e114832e7f67cb2fac1258c7423b6359160505 (conflicts resolved, I believe).

Trac:
Status: needs_review to closed
Resolution: N/A to implemented
Type: defect to enhancement

closed

moved to tpo/core/tor#11792 (closed)