* Re: [PATCH] Fix queue stats for sockets with SO_REUSEPORT
2023-02-24 19:42 8% ` Dale Hamel
@ 2023-02-25 0:36 7% ` Eric Wong
0 siblings, 0 replies; 5+ results
From: Eric Wong @ 2023-02-25 0:36 UTC (permalink / raw)
To: Dale Hamel; +Cc: raindrops-public
Dale Hamel <dale.hamel@shopify.com> wrote:
> > I'm honestly shocked that these features get used;
> > I never convinced the org I worked for to use them :x
>
> So far we haven't actually found a production use case
> for SO_REUSEPORT with Unicorn. We thought we had some
> promising early results - but it turned out to be this
> instrumentation error in raindrops.
Oh, I meant the socket instrumentation part of raindrops.
I figured atomic counters were the only thing that ever got used.
Anyways I think SO_REUSEPORT was too risky due to the
possibility of connection drops, but then again I haven't used
more than 4 real cores.
> At least by fixing this, efforts in the future will have the correct
> metrics to base tests off of.
Cool.
> > Anything else? Could tag a release in a bit...
>
> No, that's it! My apologies for sending as two patches,
> I didn't notice the bug in my test until I had already
> submitted, naturally.
No worries. In the future, you can also ask me to do a squash
in the diffstat area or signature area, or use [SQUASH] instead of
[PATCH] in the subject prefix.
> Thanks very much for the quick review!
np. Easy for me as long as I never leave a terminal :>
^ permalink raw reply [relevance 7%]
* [ANN] raindrops 0.20.1 - real-time stats for preforking Rack servers
@ 2023-02-25 0:23 6% Eric Wong
0 siblings, 0 replies; 5+ results
From: Eric Wong @ 2023-02-25 0:23 UTC (permalink / raw)
To: ruby-talk, raindrops-public; +Cc: Dale Hamel
raindrops is a real-time stats toolkit to show statistics for Rack HTTP
servers. It is designed for preforking servers such as unicorn, but
should support any Rack HTTP server on platforms supporting POSIX shared
memory. It may also be used as a generic scoreboard for sharing atomic
counters across multiple processes.
* https://yhbt.net/raindrops/
* public mail box (no subscribers, no HTML mail): raindrops-public@yhbt.net
* mail archives: https://yhbt.net/raindrops-public/
http://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/raindrops-public
imaps://yhbt.net/inbox.comp.lang.ruby.raindrops.0
nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.lang.ruby.raindrops
imap://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.lang.ruby.raindrops.0
(.onion URLs require Tor)
* git clone https://yhbt.net/raindrops.git
* torsocks git clone http://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/raindrops.git
* https://yhbt.net/raindrops/NEWS.atom.xml
Changes:
raindrops 0.20.1
Stats for SO_REUSEPORT sockets are now handled properly.
Thanks to Dale Hamel for the patches.
Dale Hamel (2):
Fix queue stats for sockets with SO_REUSEPORT
Fix off by one error in test
--
-_-
^ permalink raw reply [relevance 6%]
* Re: [PATCH] Fix queue stats for sockets with SO_REUSEPORT
2023-02-24 19:09 14% ` Eric Wong
@ 2023-02-24 19:42 8% ` Dale Hamel
2023-02-25 0:36 7% ` Eric Wong
0 siblings, 1 reply; 5+ results
From: Dale Hamel @ 2023-02-24 19:42 UTC (permalink / raw)
To: Eric Wong; +Cc: raindrops-public
> I'm honestly shocked that these features get used;
> I never convinced the org I worked for to use them :x
So far we haven't actually found a production use case
for SO_REUSEPORT with Unicorn. We thought we had some
promising early results - but it turned out to be this
instrumentation error in raindrops.
At least by fixing this, efforts in the future will have the correct
metrics to base tests off of.
> Anything else? Could tag a release in a bit...
No, that's it! My apologies for sending as two patches,
I didn't notice the bug in my test until I had already
submitted, naturally.
Thanks very much for the quick review!
^ permalink raw reply [relevance 8%]
* Re: [PATCH] Fix queue stats for sockets with SO_REUSEPORT
2023-02-24 18:35 5% [PATCH] Fix queue stats for sockets with SO_REUSEPORT Dale Hamel
@ 2023-02-24 19:09 14% ` Eric Wong
2023-02-24 19:42 8% ` Dale Hamel
0 siblings, 1 reply; 5+ results
From: Eric Wong @ 2023-02-24 19:09 UTC (permalink / raw)
To: Dale Hamel; +Cc: raindrops-public
Thanks, pushed to raindrops.git:
10d5fb25b631 Fix off by one error in test
a5902f243cd2 Fix queue stats for sockets with SO_REUSEPORT
I'm honestly shocked that these features get used;
I never convinced the org I worked for to use them :x
Anything else? Could tag a release in a bit...
^ permalink raw reply [relevance 14%]
* [PATCH] Fix queue stats for sockets with SO_REUSEPORT
@ 2023-02-24 18:35 5% Dale Hamel
2023-02-24 19:09 14% ` Eric Wong
0 siblings, 1 reply; 5+ results
From: Dale Hamel @ 2023-02-24 18:35 UTC (permalink / raw)
To: raindrops-public; +Cc: Dale Hamel
SO_REUSEPORT was introduced in 2013, see https://lwn.net/Articles/542629/.
However, the current logic for calculating socket queue backlog stats
is from 2011, predating the advent of SO_REUSEPORT.
The current strategy was thus written before the notion that the mapping of
INET_ADDR:socket could potentially be 1:N instead of always 1:1.
This causes raindrops to provide invalid socket backlog values when
SO_REUSEPORT is used, which Unicorn supports since 2013. The current behaviour
will set the queue metric to the queue depth of the last inet_diag_msg
it processes matching the INET_ADDR. In practice, this will result in
the backlog being off by a factor N, assuming relatively even
distribution of requests to sockets, which the kernel should guarantee
through consistent hashing.
The fix here is to accumulate the socket queue depth as we iterate
over the the socket diagnostics, rather than reset it each time.
I have a provided a test, but it only checks the queues, not the
accept metrics, as those are not affected by this bug, and it is
not possible to know which of the listeners will be dispatched the
request by the kernel, and thus which should call accept.
---
ext/raindrops/linux_inet_diag.c | 4 +-
test/test_linux_reuseport_tcp_listen_stats.rb | 51 +++++++++++++++++++
2 files changed, 53 insertions(+), 2 deletions(-)
create mode 100644 test/test_linux_reuseport_tcp_listen_stats.rb
diff --git a/ext/raindrops/linux_inet_diag.c b/ext/raindrops/linux_inet_diag.c
index cabd427..2a2360c 100644
--- a/ext/raindrops/linux_inet_diag.c
+++ b/ext/raindrops/linux_inet_diag.c
@@ -306,7 +306,7 @@ static void table_set_queued(st_table *table, struct inet_diag_msg *r)
{
struct listen_stats *stats = stats_for(table, r);
stats->listener_p = 1;
- stats->queued = r->idiag_rqueue;
+ stats->queued += r->idiag_rqueue;
}
/* inner loop of inet_diag, called for every socket returned by netlink */
@@ -328,7 +328,7 @@ static inline void r_acc(struct nogvl_args *args, struct inet_diag_msg *r)
if (args->table)
table_set_queued(args->table, r);
else
- args->stats.queued = r->idiag_rqueue;
+ args->stats.queued += r->idiag_rqueue;
}
/*
* we wont get anything else because of the idiag_states filter
diff --git a/test/test_linux_reuseport_tcp_listen_stats.rb b/test/test_linux_reuseport_tcp_listen_stats.rb
new file mode 100644
index 0000000..c977c43
--- /dev/null
+++ b/test/test_linux_reuseport_tcp_listen_stats.rb
@@ -0,0 +1,51 @@
+# -*- encoding: binary -*-
+require "./test/rack_unicorn"
+require 'test/unit'
+require 'socket'
+require 'raindrops'
+$stderr.sync = $stdout.sync = true
+
+class TestLinuxReuseportTcpListenStats < Test::Unit::TestCase
+ include Raindrops::Linux
+ include Unicorn::SocketHelper
+ TEST_ADDR = ENV['UNICORN_TEST_ADDR'] || '127.0.0.1'
+ DEFAULT_BACKLOG = 10
+
+ def setup
+ @socks = []
+ end
+
+ def teardown
+ @socks.each { |io| io.closed? or io.close }
+ end
+
+ def new_socket_server(**kwargs)
+ s = new_tcp_server TEST_ADDR, kwargs[:port] || 0, kwargs
+ s.listen(kwargs[:backlog] || DEFAULT_BACKLOG)
+ @socks << s
+ [ s, s.addr[1] ]
+ end
+
+ def new_client(port)
+ s = TCPSocket.new("127.0.0.1", port)
+ @socks << s
+ s
+ end
+
+ def test_reuseport_queue_stats
+ listeners = 10
+ _, port = new_socket_server(reuseport: true)
+ addr = "#{TEST_ADDR}:#{port}"
+ listeners.times do
+ new_socket_server(reuseport: true, port: port)
+ end
+
+ listeners.times do |i|
+ all = Raindrops::Linux.tcp_listener_stats
+ assert_equal [0, i], all[addr].to_a
+ new_client(port)
+ all = Raindrops::Linux.tcp_listener_stats
+ assert_equal [0, i+1], all[addr].to_a
+ end
+ end
+end if RUBY_PLATFORM =~ /linux/
--
2.34.1
^ permalink raw reply related [relevance 5%]
Results 1-5 of 5 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2023-02-24 18:35 5% [PATCH] Fix queue stats for sockets with SO_REUSEPORT Dale Hamel
2023-02-24 19:09 14% ` Eric Wong
2023-02-24 19:42 8% ` Dale Hamel
2023-02-25 0:36 7% ` Eric Wong
2023-02-25 0:23 6% [ANN] raindrops 0.20.1 - real-time stats for preforking Rack servers Eric Wong
Code repositories for project(s) associated with this public inbox
https://yhbt.net/raindrops.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).