Opened 6 months ago

Closed 2 months ago

#29085 closed defect (fixed)

WTF-PAD: Reduce monotime usage because of performance issues

Reported by: asn Owned by:
Priority: Very High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: wtf-pad, tor-relay, tor-cell, padding, 041-proposed, network-team-roadmap-2019-Q1Q2, asn-merge
Cc: nickm, asn Actual Points:
Parent ID: #28634 Points: 4
Reviewer: nickm Sponsor: Sponsor2

Description

There are various concerns about the monotime usage of circpadding. We should look into this before enabling any machines here.

See https://github.com/torproject/tor/pull/624#discussion_r246443113
and https://trac.torproject.org/projects/tor/ticket/28142#comment:32

Child Tickets

Change History (15)

comment:1 Changed 6 months ago by mikeperry

Keywords: 041-proposed added

comment:2 Changed 6 months ago by mikeperry

Points: 4

comment:3 Changed 6 months ago by mikeperry

Priority: MediumVery High

comment:4 Changed 6 months ago by nickm

Sponsor: Sponsor2

comment:5 Changed 3 months ago by mikeperry

Cc: asn added
Status: newneeds_information

Ok, so if we don't make use of token removal or RTT estimates, we don't need monotime absolute_usec() at all.

However, right now our machine in #28634 does use token removal (unnecessarily, though), and probably *should* use RTT estimates. The good news is that RTT estimates only need to make like 3 monotime calls total during each circuit construction, and only on the relay side. So that usage is not heavy (and their accuracy requirements are probably ~5ms, give or take).

The other way to do this is to make a runtime test of how expensive monotime is, and decide if we should use monotime_coarse based on that result.

Last edited 3 months ago by mikeperry (previous) (diff)

comment:6 Changed 2 months ago by mikeperry

Status: needs_informationneeds_review

Ok, here is a branch that refactors all of our features along the monotime-related codepaths. With this branch, if machines specify CIRCPAD_TOKEN_REMOVAL_NONE and also set use_rtt_estimate=0, then they won't use any monotime calls at all: https://github.com/torproject/tor/pull/1007

asn and I have also established that the machines in #28634 don't need these features; future revisions of that branch will cease using them.

comment:7 Changed 2 months ago by asn

Taking a quick look in case I can manage to review this week. Can you please split 3208b792960ed0333d3cdc7695d30d355e9c7b30 into more commits, and/or explain better what this refactoring is supposed to do . I just see random code beeing changed and moved around without a rationale of why.

comment:8 Changed 2 months ago by mikeperry

Ok I split that commit into the non-padding path refactoring (aka taking stuff out of circpad_remove_token) and the padding path (aka taking stuff out of the padding callback). It should hopefully be easier to see what went where, now: https://github.com/torproject/tor/pull/1014

comment:9 Changed 2 months ago by dgoulet

Reviewer: nickm

comment:10 Changed 2 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.1.x-final

comment:11 Changed 2 months ago by nickm

Status: needs_reviewneeds_revision

I left comments on PR 1007, but I hadn't seen 1014 at the time. Please let me know if you need a different review.

comment:12 Changed 2 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:13 Changed 2 months ago by mikeperry

Status: needs_revisionneeds_review

Thanks Nick, your review found 1 bug and prompted new unit tests that found 2 more!

I replied on the old PR, but I had to open a new PR again because of merge/practracker conflicts.

New PR is at https://github.com/torproject/tor/pull/1024

Last edited 2 months ago by mikeperry (previous) (diff)

comment:14 Changed 2 months ago by nickm

Keywords: asn-merge added
Status: needs_reviewmerge_ready

I think this is okay now -- at least, I'm out of issues. I'm going to pass it on to asn for a final look before squash&merge.

comment:15 Changed 2 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged! Thanks everyone!

Note: See TracTickets for help on using tickets.