Opened 5 years ago

Closed 5 years ago

#11792 closed enhancement (implemented)

Consider directory connections zlib buffers when handling OOM

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay, dir, zlib, oom, 026-triaged-1
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (6)

comment:1 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added

comment:2 Changed 5 years ago by nickm

Status: newneeds_review

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

comment:3 Changed 5 years ago by sysrqb

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 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.

comment:4 Changed 5 years ago by 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?
  • 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.

comment:5 in reply to:  4 Changed 5 years ago by nickm

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!

comment:6 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Type: defectenhancement

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).

Note: See TracTickets for help on using tickets.