Opened 4 months ago

Closed 4 months ago

#22883 closed defect (fixed)

Bridge unavailable during differential consensus update

Reported by: torvlnt33r Owned by: nickm
Priority: High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.4-alpha
Severity: Normal Keywords: regression, cpu, usage, review-group-21
Cc: Actual Points: 1
Parent ID: Points: 1
Reviewer: dgoulet Sponsor:

Description

After updating Tor from 0.3.0.9 to 0.3.1.4 (on a Raspberry Pi running Raspbian), I observed that my bridge became unable to answer new connection requests for extended periods, from 10 to 20 minutes or more, while tor process uses all available cpu (more than 90% on average - with no corresponding traffic for clients) and files are updated in /var/lib/tor/diff-cache directory.
(For example, if I launch a TorBrowser using this bridge during such an update, it remains blocked before opening the Welcome window, and the corresponding client tor process remains blocked after "Bootstrapped 90%: Establishing a Tor circuit", for 10 minutes or more).
So I had to downgrade to 0.3.0.9, so that my bridge doesn't become unavailable for a total of several hours per day.

May I suggest that an option make it possible to choose between the new differential update mode and the traditional one?
First, this would be necessary for some (like me) to keep following new Tor versions long term.
And even on more powerful platforms, and even if a better handling of priorities could make it possible for main Tor thread to remain available on Pi, Tor relay operators might want to make different choices between minimizing bandwidth for consensus transfer or minimizing cpu usage, and hence power consumption. (Please think of the Planet globally, and not only of the network ;) )

Child Tickets

Change History (19)

comment:1 Changed 4 months ago by nickm

Keywords: regression cpu usage added
Milestone: Tor: 0.3.1.x-final
Priority: MediumHigh

Probably the easiest workaround here for low-powered machines would be to produce and keep diffs only from the most recent consensus.

comment:2 Changed 4 months ago by nickm

I've implemented one slightly better approach here, though that workaround is still a fairly good idea as well.

For review on oniongit at: https://oniongit.eu/nickm/tor/merge_requests/3

comment:3 Changed 4 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:4 Changed 4 months ago by nickm

Status: acceptedneeds_review

comment:5 Changed 4 months ago by nickm

Additionally, I've done the simple approach as https://oniongit.eu/nickm/tor/merge_requests/4

comment:6 Changed 4 months ago by nickm

Actual Points: 1
Points: 1

comment:7 Changed 4 months ago by ahf

The simple approach looks good. I'm not marking it as merge_ready as the "better approach" patch needs to be reviewed as well.

comment:8 Changed 4 months ago by nickm

Merged the simple approach to 0.3.1 and forward. The complex approach is still here, in need of review:

For review on oniongit at: ​https://oniongit.eu/nickm/tor/merge_requests/3

comment:9 Changed 4 months ago by nickm

Reviewer: dgoulet

comment:10 Changed 4 months ago by nickm

Keywords: review-group-21 added

comment:11 Changed 4 months ago by ahf

Owner: changed from nickm to ahf
Status: needs_reviewassigned

comment:12 Changed 4 months ago by ahf

Owner: changed from ahf to nickm
Reviewer: dgouletahf

comment:13 Changed 4 months ago by ahf

Status: assignedneeds_review

comment:14 Changed 4 months ago by ahf

Status: needs_reviewmerge_ready

The more complex approach looks good to me as well. There is a minor typo in a docstring that should be fixed before this land in the repository.

comment:15 Changed 4 months ago by nickm

Reviewer: ahfdgoulet
Status: merge_readyneeds_review

Okay. I'm putting this back in needs-review in hopes dgoulet will have time for an extra review, since threading bugs are scary. :)

comment:16 Changed 4 months ago by dgoulet

Status: needs_reviewneeds_revision

I've gone over, afaict, it's good. Small typo and doc issues. Putting it back in needs_revision because there are typos that need to be fixed. After that, imo it can be merged. I'll run that on my relay as soon as it is upstream.

comment:17 Changed 4 months ago by nickm

Status: needs_revisionneeds_review

ok, thanks! I've pushed some updates, if you have time to take a quick look.

comment:18 Changed 4 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;!

comment:19 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Squashed and merged to 0.3.1 and forward. Let's see if anything explodes. ;)

Note: See TracTickets for help on using tickets.