From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SazudnV5; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 427D35A061B for ; Wed, 05 Nov 2025 08:01:04 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1762326063; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=41cXF9TX9zQagKRQ1GC+MJdqDnQDJjvvs8FIhaO4yNA=; b=SazudnV5LWxCAya+h12xslOetWblVVhBvTJFa+uFR8ADHcrrnsSowynrclfrncEge52MOm G3MJa0AUj2DH2+sshQ6Tp7weoJeevGVv0acICuScwbIy+wERH+03M8NgXcEuF2AvsynKUu m1boAccUJU6ErD+OnO6MrIUcrAwsRjU= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-26-X4I5FgAXNLS2B0sdgOjvZQ-1; Wed, 05 Nov 2025 02:01:02 -0500 X-MC-Unique: X4I5FgAXNLS2B0sdgOjvZQ-1 X-Mimecast-MFC-AGG-ID: X4I5FgAXNLS2B0sdgOjvZQ_1762326061 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-429c5da68e5so474672f8f.1 for ; Tue, 04 Nov 2025 23:01:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1762326061; x=1762930861; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=41cXF9TX9zQagKRQ1GC+MJdqDnQDJjvvs8FIhaO4yNA=; b=FgMsnn04tts/FJoUJf/SEkABgE3S4PnnP6VVHWmQThujBIQ51vVVLSvZCQUyT6i4fK 7AeBvuyCEwoMyBTPwyrkW2tdwCLixIoPtiQZfNgs5GhCr1uWUkS0xUeU+6EHLyrnbLDT O9Yw4n4Y7tc4pXzUUKqaAPDWbCwoC9d2ZMS+0oTSVUSlaQ5J6buwKPIAzkcuOF5W06dn h4kAJ6yKE42pEvB2bOmobaPamGFFIQxeNTDDRT0MLMjGEk8NymMT++bK3ovzAVlgRRzB fpHo8YlehyR6k5VfqCWAiF+ZfFyETZ9QmQsx4qkXesTrsT0h1hSVEtmM/fNvlDA0QHC0 POxQ== X-Forwarded-Encrypted: i=1; AJvYcCW6SAMUkkuLFr2Q8HMrBUGOxBEBlCnOBAA8DNTnnD0B7ca45Ot6Y6h+if7avEuI03/K6zRuN5pnpPI=@passt.top X-Gm-Message-State: AOJu0YyHUmrH/8tNxz/qhJt27lZaFrMCr5MufAa2mCs3rSOkMkX0qOJa ZKjv+3wzm3iUVauQEraeCHOMO0tsmD4Gb3X4nXEKtjhkCFsAZHm5S/+FKp1AYprLcQj1SpP1t3j zcbqBsL4kD9WwZYTndjLVzrpJYHaFsrfQU+HQ1SWlWTzaysHZgPdDEQ== X-Gm-Gg: ASbGncvssJ0A42++3HyvwKhM9g7CTxsk/hO9vkPiDvv0/z1RL85JDYiPMV+OHODKju5 eeicybC8I5Sz3Xy5K6nFDeBA4A9BJlEVUq2/51afDC6JezOZfgPWyGARm2nniuxdie2EVE1uZVR LR9iBHT2jn3fZyXUejm+Ncv58vooadW5PV2MN0gtk4KG3F5JzURwiTDAbBtncXMTlHuBnMAoPtw JwaWkDCXIdHUqch401OGVokVz+DiUUUrtQFuGTC0eAZ8qstG7QWLr8ZesspeYBon8hANjHcfsdh W9MeVmarhDs0y/jovfLrQHXa7A9PXPlOK69cyda1kWiQUYbMUX3rRTB1Q0lb5xYD7S+35sRH07Z SEhDIauOjOWLjky2jLRwbd+8zWf8= X-Received: by 2002:a05:6000:2287:b0:429:c6ba:d94c with SMTP id ffacd0b85a97d-429dbcdff66mr5778193f8f.10.1762326060281; Tue, 04 Nov 2025 23:01:00 -0800 (PST) X-Google-Smtp-Source: AGHT+IFGj3922klPRbxzFdc6PLr+Kbpcg8cqfySBVUFTX+QUVPUoRX/XlbbY+6Mba1PXQEpJ/TZn/w== X-Received: by 2002:a05:6000:2287:b0:429:c6ba:d94c with SMTP id ffacd0b85a97d-429dbcdff66mr5778138f8f.10.1762326059490; Tue, 04 Nov 2025 23:00:59 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-429dc1f98d9sm8588873f8f.32.2025.11.04.23.00.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Nov 2025 23:00:58 -0800 (PST) Date: Wed, 5 Nov 2025 08:00:57 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v7 3/5] tcp: Resend SYN for inbound connections Message-ID: <20251105080057.3c4a61b9@elisabeth> In-Reply-To: References: <20251031054242.7334-1-yuhuang@redhat.com> <20251031054242.7334-4-yuhuang@redhat.com> <20251104054233.1dec4eb6@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: u250Y6nnPpVf0UytRTjIQ4VMWkx2TI_WTS1cfhbcJJw_1762326061 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 7YUL3IHT76JMI227NBXVISXI4FRPB2XF X-Message-ID-Hash: 7YUL3IHT76JMI227NBXVISXI4FRPB2XF X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Yumei Huang , passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, 5 Nov 2025 15:24:44 +1100 David Gibson wrote: > On Tue, Nov 04, 2025 at 05:42:33AM +0100, Stefano Brivio wrote: > > On Fri, 31 Oct 2025 13:42:40 +0800 > > Yumei Huang wrote: > [snip] > > > +/** > > > + * tcp_get_rto_params() - Get host kernel RTO parameters > > > + * @c: Execution context > > > +*/ > > > +void tcp_get_rto_params(struct ctx *c) > > > +{ > > > + intmax_t tcp_syn_retries, syn_linear_timeouts; > > > + > > > + tcp_syn_retries = read_file_integer( > > > + TCP_SYN_RETRIES, TCP_SYN_RETRIES_DEFAULT); > > > + syn_linear_timeouts = read_file_integer( > > > + TCP_SYN_LINEAR_TIMEOUTS, TCP_SYN_LINEAR_TIMEOUTS_DEFAULT); > > > > I think this is a bit hard to read. Now: > > > > tcp_syn_retries = read_file_integer(TCP_SYN_RETRIES, > > TCP_SYN_RETRIES_DEFAULT); > > syn_linear_timeouts = read_file_integer(TCP_SYN_LINEAR_TIMEOUTS, > > TCP_SYN_LINEAR_TIMEOUTS_DEFAULT); > > > > would be a bit closer to the coding style we're adopting, but I wonder: > > > > - does read_file_integer() really need to have "integer" in the name? > > > In a language where integers are called 'int', perhaps > > read_file_int() is clear enough? > > I think the idea is that read_file_integer() can be used for any > (signed) integer type (with range checking performed after the call). > read_file_int() might suggest it reads exactly an 'int', not anything > bigger or smaller. Oh, I see. It could be read_file_num() then, even if it's slightly less accurate, or it can even remain read_file_integer(). If something like this: v = read_file_integer(SYN_RETRIES, SYN_RETRIES_DEFAULT); c->tcp.tcp_syn_retries = MIN(v, UINT8_MAX); v = read_file_integer(SYN_LINEAR_TIMEOUTS, SYN_LINEAR_TIMEOUTS_DEFAULT); c->tcp.syn_linear_timeouts = MIN(v, UINT8_MAX); fits 80 columns, I'm taking it as a sign that the function name isn't exceedingly long. No particular preference from me. > > - the constants are defined here, in tcp.c, so they obviously refer to > > TCP, so maybe SYN_LINEAR_TIMEOUTS is clear enough > > > > - you don't really need to store both values in two different > > variables, one is enough as you're assigning them right away. And: > > > > v = read_file_int(SYN_RETRIES, SYN_RETRIES_DEFAULT); > > c->tcp.tcp_syn_retries = MIN(v, UINT8_MAX); > > > > v = read_file_int(SYN_LINEAR_TIMEOUTS, SYN_LINEAR_TIMEOUTS_DEFAULT); > > c->tcp.syn_linear_timeouts = MIN(v, UINT8_MAX); > > > > is four lines instead of six, and more readable if you ask me. > > > > > + > > > + c->tcp.tcp_syn_retries = MIN(tcp_syn_retries, UINT8_MAX); > > > + c->tcp.syn_linear_timeouts = MIN(syn_linear_timeouts, UINT8_MAX); > > > + > > > + debug("Read sysctl values tcp_syn_retries: %"PRIu8", linear_timeouts: %"PRIu8, > > > + c->tcp.tcp_syn_retries, c->tcp.syn_linear_timeouts); > > > +} > > > + > > > /** > > > * tcp_init() - Get initial sequence, hash secret, initialise per-socket data > > > * @c: Execution context > > > @@ -2776,6 +2815,8 @@ int tcp_init(struct ctx *c) > > > { > > > ASSERT(!c->no_tcp); > > > > > > + tcp_get_rto_params(c); > > > + > > > tcp_sock_iov_init(c); > > > > > > memset(init_sock_pool4, 0xff, sizeof(init_sock_pool4)); > > > diff --git a/tcp.h b/tcp.h > > > index 234a803..befedde 100644 > > > --- a/tcp.h > > > +++ b/tcp.h > > > @@ -59,12 +59,16 @@ union tcp_listen_epoll_ref { > > > * @fwd_out: Port forwarding configuration for outbound packets > > > * @timer_run: Timestamp of most recent timer run > > > * @pipe_size: Size of pipes for spliced connections > > > + * @tcp_syn_retries: SYN retries using exponential backoff timeout > > > + * @syn_linear_timeouts: SYN retries before using exponential backoff timeout > > > */ > > > struct tcp_ctx { > > > struct fwd_ports fwd_in; > > > struct fwd_ports fwd_out; > > > struct timespec timer_run; > > > size_t pipe_size; > > > + uint8_t tcp_syn_retries; > > > > Why 'tcp' again? > > > > > + uint8_t syn_linear_timeouts; > > > }; > > > > > > #endif /* TCP_H */ -- Stefano