From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 4B6685A0271 for ; Sun, 3 Mar 2024 14:50:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709473832; 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=X9GlExg5Z0kQydPBVzLntIsiJW0jDs+YqNpm5DJy7MU=; b=KuZ05icsj7xM4bqKACs9SrUf9tAT4nv4yQ76+frEfDSa4am10w8DkmbNbtdOGGwZcd22EH mvmk/egyt0VRpev8Fwi4l++dP1JDVG6vB0kzedQ4XefPxMWEjRGjLxQ8ED2YJzdCc+5XwY oNpwmxYfJKS7mKhS9KO1Yz2K/16h6Rk= Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-339-aRAKInWDP7ODtNNWMq5lYQ-1; Sun, 03 Mar 2024 08:50:30 -0500 X-MC-Unique: aRAKInWDP7ODtNNWMq5lYQ-1 Received: by mail-qk1-f197.google.com with SMTP id af79cd13be357-787ce91a4e0so843067085a.0 for ; Sun, 03 Mar 2024 05:50:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709473829; x=1710078629; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=X9GlExg5Z0kQydPBVzLntIsiJW0jDs+YqNpm5DJy7MU=; b=rfcaRJh3Ehc50VahCdkR7Mb7C12vZNtuSSvGkIJQnS1ibywzLXPJvrHCS1+qeXmaQT yJBtBL0AuKkySWJHwLc00VkaAZgOChS1leDBLpuRgUuyhcEaOjD5pcE+v61UKf0Vh3Kf RwiP/DbRJRUOrq92joYRTwGXsBBV7shdo7oxqE7ekISoEcn6VSJJl/5SfqURdla9qSnv rXsEVVtsuJWM8m/xaAwObjX7ptfInNH05MQvHqWfzxPrkxq2+qr91HnpI9LttGKxFj+o o2tEJipLkmAo90EDZCgg8tFWiM0BzFKE2sWp34DiCVhsP6ByV2GNDQbLGVm6agbIydEN eq6g== X-Gm-Message-State: AOJu0YxTW8LpMHAddaC9J1PF46+sQFqeX5cehWbgUDlZBE24oP6z9h0w ttO9xQMGAteaL9m1OCckLA3F6YHPXb7cDBlkBs11BDIqIE+OqoiH71aR09IjBILZsZKBnj4Rqrv Vq5KocLud1dW7oZa7YMb8EdRaTeaEKsdqir07pwFleAN1elI2XIbipe/HOg== X-Received: by 2002:a05:620a:44c1:b0:787:f11f:49f7 with SMTP id y1-20020a05620a44c100b00787f11f49f7mr16524309qkp.26.1709473829681; Sun, 03 Mar 2024 05:50:29 -0800 (PST) X-Google-Smtp-Source: AGHT+IGriPTk0soBu1f6RF9Sm5baNr/l5bT5meGaC2XkioJwYlt16d/0RDTLnNYfrSqSBm0YLfTbXw== X-Received: by 2002:a05:620a:44c1:b0:787:f11f:49f7 with SMTP id y1-20020a05620a44c100b00787f11f49f7mr16524274qkp.26.1709473829185; Sun, 03 Mar 2024 05:50:29 -0800 (PST) Received: from [192.168.100.30] ([82.142.8.70]) by smtp.gmail.com with ESMTPSA id fu32-20020a05622a5da000b0042ee44418e3sm835352qtb.94.2024.03.03.05.50.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 03 Mar 2024 05:50:28 -0800 (PST) Message-ID: <29633956-4e1c-404d-bd15-86ad74fdd39a@redhat.com> Date: Sun, 3 Mar 2024 14:50:26 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers() To: David Gibson References: <20240229165955.829476-1-lvivier@redhat.com> <20240229165955.829476-9-lvivier@redhat.com> From: Laurent Vivier In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Message-ID-Hash: 5TGKAXW2AE7DF2M7TPX32DXH4N27J4WH X-Message-ID-Hash: 5TGKAXW2AE7DF2M7TPX32DXH4N27J4WH X-MailFrom: lvivier@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: 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 3/1/24 00:54, David Gibson wrote: > On Thu, Feb 29, 2024 at 05:59:55PM +0100, Laurent Vivier wrote: >> Replace the macro SET_TCP_HEADER_COMMON_V4_V6() by a new function >> tcp_fill_header(). >> >> Move IPv4 and IPv6 code from tcp_l2_buf_fill_headers() to >> tcp_fill_ipv4_header() and tcp_fill_ipv6_header() >> >> Signed-off-by: Laurent Vivier >> --- >> >> Notes: >> v4: >> - group all the ip6g initialisations together and >> remove flow_lbl preset to 0 >> - add ASSERT(a4) >> >> v3: >> - add to sub-series part 1 >> >> v2: >> - extract header filling functions from >> "tcp: extract buffer management from tcp_send_flag()" >> - rename them tcp_fill_flag_header()/tcp_fill_ipv4_header(), >> tcp_fill_ipv6_header() >> - use upside-down Christmas tree arguments order >> - replace (void *) by (struct tcphdr *) >> >> tcp.c | 154 +++++++++++++++++++++++++++++++++++++++------------------- >> 1 file changed, 104 insertions(+), 50 deletions(-) >> >> diff --git a/tcp.c b/tcp.c >> index 5b2fdf662a6c..ced22534a103 100644 >> --- a/tcp.c >> +++ b/tcp.c >> @@ -1326,6 +1326,108 @@ void tcp_defer_handler(struct ctx *c) >> tcp_l2_data_buf_flush(c); >> } >> >> +/** >> + * tcp_fill_header() - Fill the TCP header fields for a given TCP segment. >> + * >> + * @th: Pointer to the TCP header structure >> + * @conn: Pointer to the TCP connection structure >> + * @seq: Sequence number >> + */ >> +static void tcp_fill_header(struct tcphdr *th, >> + const struct tcp_tap_conn *conn, uint32_t seq) >> +{ >> + th->source = htons(conn->fport); >> + th->dest = htons(conn->eport); >> + th->seq = htonl(seq); >> + th->ack_seq = htonl(conn->seq_ack_to_tap); >> + if (conn->events & ESTABLISHED) { >> + th->window = htons(conn->wnd_to_tap); >> + } else { >> + unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap; >> + >> + th->window = htons(MIN(wnd, USHRT_MAX)); >> + } >> +} >> + >> +/** >> + * tcp_fill_ipv4_header() - Fill 802.3, IPv4, TCP headers in pre-cooked buffers > > I don't love the name, since it does also fill the TCP header. Maybe > 'tcp_fill_headers4()'? > >> + * @c: Execution context >> + * @conn: Connection pointer >> + * @iph: Pointer to IPv4 header, immediately followed by a TCP header > > Again, really don't love accessing beyond a given pointer's type. > >> + * @plen: Payload length (including TCP header options) >> + * @check: Checksum, if already known >> + * @seq: Sequence number for this segment >> + * >> + * Return: IP frame length including L2 headers, host order > > AFAICT the return value does *not* include the L2 headers.. > >> + */ >> +static size_t tcp_fill_ipv4_header(const struct ctx *c, >> + const struct tcp_tap_conn *conn, >> + struct iphdr *iph, size_t plen, >> + const uint16_t *check, uint32_t seq) >> +{ >> + size_t ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr); >> + const struct in_addr *a4 = inany_v4(&conn->faddr); >> + struct tcphdr *th = (struct tcphdr *)(iph + 1); >> + >> + ASSERT(a4); >> + >> + iph->tot_len = htons(ip_len); >> + iph->saddr = a4->s_addr; >> + iph->daddr = c->ip4.addr_seen.s_addr; >> + >> + iph->check = check ? *check : >> + csum_ip4_header(iph->tot_len, IPPROTO_TCP, >> + iph->saddr, iph->daddr); >> + >> + >> + tcp_fill_header(th, conn, seq); >> + >> + tcp_update_check_tcp4(iph); > > It's a bit ugly that tcp_fill_header() fills the TCP header, but *not* > the checksum. Could we handle this by passing the pseudo-header psum > into tcp_fill_header()? Then the logic for that in > tcp_update_check_tcp4() would become part of this function. The problem with that is we must also pass the payload (that is after the TCP header) and the payload length. So we need to add two parameters to tcp_fill_header(); psum and payload_length (guessing also the payload is following "th"). Moreover in the vhost-user part tcp_update_check_tcp4() is now called conditionally, because the checksum is computed in the vhost-code as the payload is stripped along several iovecs. I'm going to send my v5 without updating this part. If you really think it should be done differently please give me more details (considering also the vhost-user part). Thanks, Laurent