Opened 6 years ago

Closed 6 years ago

#9716 closed enhancement (fixed)

Don't hardcode listen() backlog

Reported by: philip Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.16-rc
Severity: Keywords: tor-relay easy 024-backport limits, socket, freebsd, linux
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

While investigating Ticket #9708, I also found a lot of kernel messages along the lines of:

  • sonewconn: pcb 0xfffffe005b7af000: Listen queue overflow: 193 already in queue awaiting acceptance

This despite the fact that "kern.somaxconn", which controls the length of the listen queue, was set to 4096.

Reading through the code, I found two instances of "listen(s,SOMAXCONN)" in src/or/connection.c. I think using SOMAXCONN as backlog is wrong. I think the correct backlog parameter is -1 -- which tells the kernel to accept as many connections as it's configured to accept, rather than a hardcoded constant (which happens to be the default the kernel will accept -- if not configured diferently).

SOMAXCONN is defined to 128 in <sys/socket.h> and is the default for kern.somaxconn. I'm not sure why it's exposed to userland. Possibly hysterical raisins?

Setting the backlog to -1 will allow tor to accept more connections faster if kern.somaxconn > SOMAXCONN (ie > 128).

This is true for FreeBSD (viz solisten_proto() in sys/kern/uipc_socket.) I've not tested Linux, but looking at the code (SYSCALL_DEFINE2(listen, int, fd, int, backlog) in net/socket.c) it should behave the same if I'm reading that cast correctly. On Linux, the moral equivalent of kern.somaxconn is configured with /proc/sys/net/core/somaxconn. It's also set to 128 by default. If raised to something huge, and listen() given a backlog of -1, it will also accept more connections.

Child Tickets

Change History (8)

comment:1 Changed 6 years ago by nickm

Component: - Select a componentTor
Keywords: tor-relay easy 024-backport added
Milestone: Tor: 0.2.5.x-final

Is that behavior of -1 according to specification? I don't see it documented on the manpage for listen(2) on Linux or OSX, and the Windows documentation says "Illegal values are replaced by the nearest legal value", which might imply that -1 turns into 0 or 1 or something.

Both manpages document that values larger than the limit are rounded down to the true maximum, implying that on those platforms INT_MAX is the doucmented way to do this.

Perhaps we want to do something as simple as:

  if (listen(fd, INT_MAX) < 0) {
    // that didn't work; fall back to the stupid thing.
    if (listen (fd, SOMAXCONN) < 0) {
       // neither one worked.
    }
  }

comment:2 Changed 6 years ago by philip

The closest thing to a "specification" is probably the UNIX Network Programming book. I just looked it up (page 104 and further, third edition). Much historical context, but no suggestion as to what to say for "as many as possible". It does say that all relevant systems will truncate to as large as they're prepared to accept if you specify something larger than that.

I looked at the kernel side of listen() on FreeBSD, OS X and Linux, and they will all Do The Right Thing[tm] with that. The fallback should catch anything else.

Thanks!

comment:3 Changed 6 years ago by nickm

Status: newneeds_review

Have a look at branch "bug9716_024" in my public repository (see https://gitweb.torproject.org/nickm/tor.git ) -- it needs review and testing.

comment:4 Changed 6 years ago by philip

That patch looks good and it works for me on FreeBSD. I don't have a Linux relay nearby at the moment, but probably someone else can test before me! Thanks.

comment:5 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

Okay. I have merged this into the master branch (which will be 0.2.5) so it can see some testing, and I'm marking this ticket for possible backport to 0.2.4.

comment:6 Changed 6 years ago by arma

How do we know whether it's gotten testing? I guess nobody has complained therefore it's perfect?

I could see a good argument for leaving the fix in 0.2.5, and not 0.2.4, and saying "use 0.2.5" to the two bsd people who are affected.

Or if you are convinced that the patch is fine, and want these two bsd people to be happier, I guess you can backport it.

comment:7 Changed 6 years ago by nickm

I say we let it cook for a while. It's not urgent in 0.2.4 fwict

comment:8 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

No visible explosions in 0.2.5; backporting.

Note: See TracTickets for help on using tickets.