Opened 10 years ago

Closed 9 years ago

Last modified 7 years ago

#1042 closed defect (fixed)

rate-limit MaxOnionsPending warns

Reported by: arma Owned by:
Priority: Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version: 0.2.1.19
Severity: Keywords: easy
Cc: arma, nickm, karsten, Sebastian, chris@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by Sebastian)

If your cpu can't keep up with the number of create cells you get, you'll
fill your logs with warnings:

if (ol_length >= get_options()->MaxOnionsPending) {

log_warn(LD_GENERAL,

"Your computer is too slow to handle this many circuit "
"creation requests! Please consider using the "
"MaxAdvertisedBandwidth config option or choosing a more "
"restricted exit policy.");

We should make the warning only happen once a minute or something.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Attachments (2)

Change History (22)

comment:1 Changed 10 years ago by arma

This would be a good self-contained task for a volunteer to work on.

See warn_too_many_conns() in connection.c for an example of how to
go about it.

comment:2 Changed 10 years ago by arma

Karsten added this in 19ddee5582bf6dc3d53cb319

Closing.

comment:3 Changed 10 years ago by arma

The patch works great, but we left out a piece:

Dec 20 23:10:16.446 [warn] Your computer is too slow to handle this many circuit

creation requests! Please consider using the MaxAdvertisedBandwidth config opti

on or choosing a more restricted exit policy.
Dec 20 23:10:16.567 [warn] Failed to hand off onionskin. Closing.
Dec 20 23:10:16.615 [warn] Failed to hand off onionskin. Closing.
Dec 20 23:10:16.627 [warn] Failed to hand off onionskin. Closing.
Dec 20 23:10:16.638 [warn] Failed to hand off onionskin. Closing.
Dec 20 23:10:16.648 [warn] Failed to hand off onionskin. Closing.
Dec 20 23:10:16.651 [warn] Failed to hand off onionskin. Closing.
Dec 20 23:10:16.687 [warn] Failed to hand off onionskin. Closing.
Dec 20 23:10:16.705 [warn] Failed to hand off onionskin. Closing.
... thousands more

Option one is to turn this into an info-level log, since we are already
logging warns (albeit rate-limited) in the most common failure case of
assign_onionskin_to_cpuworker().

Option two would be to rate limit the 'hand off onionskin' msgs too.

I vote for option one.

comment:4 Changed 9 years ago by Sebastian

info sounds sane, the warning doesn't really help users, I think.
So either reword + ratelimit the warning, or just put it in info.

comment:5 Changed 9 years ago by nickm

Keywords: easy added
Milestone: Tor: 0.2.2.x-final

comment:6 Changed 9 years ago by cjb

Here's the obvious patch. (Feel free to drop the changes file if you don't think it's useful.)

comment:7 Changed 9 years ago by Sebastian

Description: modified (diff)

NAK on the patch. It changes the wrong warning.

comment:8 Changed 9 years ago by cjb

Oops, so much for the obvious patch. Fixed in v2, thanks for noticing.

comment:9 Changed 9 years ago by Sebastian

That one's good, thanks

comment:10 Changed 9 years ago by nickm

Status: newneeds_review

comment:11 Changed 9 years ago by nickm

(Aside: On and off, I've been thinking of a generic warning-rate-limit mechanism. But "the perfect is the enemy of the good," so cleaning up an annoying warning shouldn't wait on me.)

comment:12 Changed 9 years ago by cjb

Hm, what do you think of the kernel's mechanism? (linux/lib/ratelimit.c -- you call printk_ratelimit() to find out whether the system's currently ratelimited, and if it disallows you to printk then it'll let the user know how many printks it suppressed. The default is one printk every five seconds, but there's also a burst parameter for how many messages to allow per-time-period before suppression starts to kick in, default is ten.)

comment:13 Changed 9 years ago by cjb

Cc: chris@… added

comment:14 Changed 9 years ago by nickm

See branch ratelim in my master: it replaces existing log rate-limits with a generic rate-limiter structure.

comment:15 Changed 9 years ago by nickm

errr, that should have been "see branch ratelim in my public repository."

comment:16 Changed 9 years ago by Sebastian

I think you mean <= instead of >= in this line, otherwise this never triggers:

+  if (lim->rate + lim->last_allowed >= now) {

Also I'd make rate_limit_is_ready() static. It can't be used to peek how many log messages where generated so far and for logging purposes rate_limit_log() should be entirely sufficient.

For the interval that is used to ratelimit onionskin handoff messages, we should probably use WARN_TOO_MANY_CONNS_INTERVAL, or is there a reason why you want it to log more often?

Other than that, looks good to me.

comment:17 Changed 9 years ago by nickm

Agreed on the first two issues.

I'm fine making the handoff_warning interval longer, but it isn't WARN_TOO_MANY_CONNS_INTERVAL; that is for too many conns, not failed onionskin handoffs.

Added another commit to branch "ratelim" in my public to fix these.

comment:18 Changed 9 years ago by Sebastian

looks good, thanks

comment:19 Changed 9 years ago by nickm

Resolution: Nonefixed
Status: needs_reviewclosed

Great; merging and closing.

comment:20 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.