From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-3.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from mail-il1-x133.google.com (mail-il1-x133.google.com [IPv6:2607:f8b0:4864:20::133]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id B7AE41F62B for ; Fri, 24 Feb 2023 18:36:00 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=pass (1024-bit key; unprotected) header.d=shopify.com header.i=@shopify.com header.a=rsa-sha256 header.s=google header.b=CWMRwNAJ; dkim-atps=neutral Received: by mail-il1-x133.google.com with SMTP id b12so242844ils.8 for ; Fri, 24 Feb 2023 10:36:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shopify.com; s=google; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=0m/DjLrKXbCcE7fBxqWuB76C+WtNdGpaQqxmeMEuZCU=; b=CWMRwNAJddA0dz5zLGoQcSAA8fBYavTiUUoFlXT2ZuB1YuogmEaiIz2T2VosH4egGm r9oVcGzhtU0BMH2taUtD9RgIGnRUb+li/7UuDnFjxEqEi/GyFhiDVsst0P2DxFpcH57k HkH7Lh0zuI49Amon7FVFVUvyXtHnyLvIi9YTA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=0m/DjLrKXbCcE7fBxqWuB76C+WtNdGpaQqxmeMEuZCU=; b=kiuXoqPU3Kvx22u44hrjZcT/ElgANARks0k1nUfPOPLRCDY0hVYH4eWyBF+cg57ThS BZJ0QL7sVqjzQamWKi416ieYoLqYMPVsWNzjBmdlESS0+s5frUHGDNhW68pkRMjVtBi2 852urVaOeTSTOHYFyuM5FxEicA5Mvx+TaxyszSD1fGzfoVMR8OGfaaR9RILbdx42l2/I 19RtQBShBKNGR/MnexnO+/0Xb7n1iCp8JpcV+zaEqgZqcGnOPiqqljMxMO2hEzn/eEg4 PMxt2UjShvwk5l1MTSwefWXoMO3F4T1iOLYN31+Gp2haSmxdk/RJgj7PbCNHG46Mr27K rvew== X-Gm-Message-State: AO0yUKVWo92xe8LoWOGKqvjV7ytVfQxTa/NAl7L2e4eD0LEXRCBr8zrB CU4QFO4qYxfdHIBpq2mMWuTAbBIn8wzax/0tvBHWKj5A50pMJiYuCLKDNs3B+xfdVkxvfwdpwfk i8dAag8EiXBz+RTjHpNurlcYPBIMgmEsdQahUENjbXynOFX9mgLBw2uIM7yoFLaXVNqy8XEssSR 1QaKgu X-Google-Smtp-Source: AK7set/3LzaRBZp18D75p8X+/ulvbw5ef0NWo3Xf7NMv7Hpo5VbE4GuQyLgEsvDi0Sj48FNeW1qkZQ== X-Received: by 2002:a05:6e02:1b02:b0:315:3581:bbf with SMTP id i2-20020a056e021b0200b0031535810bbfmr13378326ilv.14.1677263758289; Fri, 24 Feb 2023 10:35:58 -0800 (PST) Received: from MacBook-Pro.localdomain (198-84-220-18.cpe.teksavvy.com. [198.84.220.18]) by smtp.gmail.com with ESMTPSA id p10-20020a92d48a000000b0030c27c9eea4sm3903935ilg.33.2023.02.24.10.35.57 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Fri, 24 Feb 2023 10:35:57 -0800 (PST) From: Dale Hamel To: raindrops-public@yhbt.net Cc: Dale Hamel Subject: [PATCH] Fix queue stats for sockets with SO_REUSEPORT Date: Fri, 24 Feb 2023 13:35:44 -0500 Message-Id: <20230224183544.51837-1-dale.hamel@shopify.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: 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