From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none 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=QFwZkoFN; dkim-atps=neutral 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 0A4AC5A004C for ; Tue, 29 Oct 2024 11:32:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1730197953; 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=QES4FfOtSeH2I9AA0KxYrk11AmTqKZZZaZJhxuIIFDY=; b=QFwZkoFNHruCHmv+64sKaMJv6BvmQr2EVkrB+T5MjE/vjzq2v6p8jTRe8zboxC6OkL66Ke +6ZURjn7atPI95Gwos9bxw8Qs9GpUyqNwEo6sbXhl0sljBwSRI0z/7AE67U2OvDzcP0ex3 y8jDS6kDQiUHa5KbNIyLxtF4FxNYcmo= Received: from mail-lf1-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-439-xv8OKI87OnarzknD4JRq6w-1; Tue, 29 Oct 2024 06:32:31 -0400 X-MC-Unique: xv8OKI87OnarzknD4JRq6w-1 Received: by mail-lf1-f72.google.com with SMTP id 2adb3069b0e04-53b1eef7359so4226883e87.3 for ; Tue, 29 Oct 2024 03:32:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730197950; x=1730802750; 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=QES4FfOtSeH2I9AA0KxYrk11AmTqKZZZaZJhxuIIFDY=; b=d8iZUPhvGoYnEdBT3Ju53dzkkMWI4HFKC6wJK4XP26B6P6GA0REaTrGIy+ekGFITHT 9b+F8PaGZucMOwj2qmQGNk5Q5B9pv2zwdax4Ca1fDgG56KXWFQt7FA9108hKChi3Lztp 1ztyZI27EafS00OpKtGttVZxLsO2KhhqWRxbSux3P6nruFZ1o6QYlx0eOVkgx5bCvWIr xRdJuKu4SreO2KuSNXSaC6cIs92aAyRR07Jj+qtGISF3yDJM3l7MCuLy9Nvl/THL3CJZ OrE/1WPHDejJFShd3f3ol5oVqe5fNrYvOxgXOXWaXiFXS1DDVznJQ+OXXShxRQG5XvI4 Fq8Q== X-Forwarded-Encrypted: i=1; AJvYcCU5yF8RHztTvxpQjpGh27XKR2ZPl8KziOdcUUPgH4ltJzRMab4CxQ23KRsDlNm1buLz42xn0lsUsxs=@passt.top X-Gm-Message-State: AOJu0YyCLCpThQ3Dsw7In+5lZetpRj0B04E/WAkA0uQPOHarFjo/9GNN z98SiVG4WbacdH9Jc2hhilMmIvzJoqDlIWfBPK3gijahHuU7sHRCjtsStRagvCyyY47x9rjO45P tFkay/x29lH22H0QBWfrj5zGek7TxVCUyuUh47FCog7CvQGEqDA== X-Received: by 2002:a05:6512:39c4:b0:535:6892:3be3 with SMTP id 2adb3069b0e04-53b34c5f770mr5290224e87.41.1730197949895; Tue, 29 Oct 2024 03:32:29 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG2yH1uwf77HBCs4aW6BRylzUSiYe8M1jhaNUJErS7PFOo1L8VlPaPQw79DQ/KtXeAPWqqjeg== X-Received: by 2002:a05:6512:39c4:b0:535:6892:3be3 with SMTP id 2adb3069b0e04-53b34c5f770mr5290213e87.41.1730197949405; Tue, 29 Oct 2024 03:32:29 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4319360c0besm139586705e9.41.2024.10.29.03.32.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Oct 2024 03:32:28 -0700 (PDT) Date: Tue, 29 Oct 2024 11:32:27 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 1/7] tcp: Pass TCP header and payload separately to tcp_update_check_tcp[46]() Message-ID: <20241029113227.33c76246@elisabeth> In-Reply-To: References: <20241028094050.1609090-1-david@gibson.dropbear.id.au> <20241028094050.1609090-2-david@gibson.dropbear.id.au> <20241028194254.71df8d2f@elisabeth> <20241029100954.7eb6b8a9@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: A73ZMSFZ7K7PFFZE3IPZQE6VGNN2AKKI X-Message-ID-Hash: A73ZMSFZ7K7PFFZE3IPZQE6VGNN2AKKI 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: Laurent Vivier , 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 Tue, 29 Oct 2024 20:26:25 +1100 David Gibson wrote: > On Tue, Oct 29, 2024 at 10:09:54AM +0100, Stefano Brivio wrote: > > On Tue, 29 Oct 2024 15:07:56 +1100 > > David Gibson wrote: > > > > > On Tue, Oct 29, 2024 at 02:02:25PM +1100, David Gibson wrote: > > > > On Mon, Oct 28, 2024 at 07:42:54PM +0100, Stefano Brivio wrote: > > > > > On Mon, 28 Oct 2024 20:40:44 +1100 > > > > > David Gibson wrote: > > > > > > > > > > > Currently these expects both the TCP header and payload in a single IOV, > > > > > > and goes to some trouble to locate the checksum field within it. In the > > > > > > current caller we've already know where the TCP header is, so we might as > > > > > > well just pass it in. This will need to work a bit differently for > > > > > > vhost-user, but that code already needs to locate the TCP header for other > > > > > > reasons, so again we can just pass it in. > > > > > > > > > > We couldn't do this, and also what you're now doing in 5/7, because > > > > > with vhost-user the TCP header is not aligned, so we can't pass it > > > > > around as a pointer, see: > > > > > > > > > > > > > > > https://archives.passt.top/passt-dev/ZeUpxEY-sn64NLE5@zatzit/ > > > > > > > > > > and following. That one is about IP headers, but the same applies to > > > > > TCP and UDP headers. > > > > > > > > Hrm. I'm aware it theoretically need not be aligned, but I thought it > > > > was in practice.. and that we were already relying on that. > > > > > > > > In fact, I'm pretty sure the second part is true, although more subtly > > > > than here. v8 of the vhost-user patches calls tcp_fill_headers[46]() > > > > with the bp parameter set to the offset of the TCP header. If > > > > creating a tcphdr * there is a problem, then creating a tcp_payload_t > > > > * can't be any better. > > > > Ah, okay, I missed that. Still, I think we should ask gcc for an > > opinion (with the vhost-user series on top of this series), because > > those build-time pointer alignment checks are pretty reliable. > > I'm not exactly sure what you're suggesting with this. I don't think > the compiler will catch it in this case, because we're constructing > the (possibly) misaligned pointer as a (void *), then implicitly > casting it by passing it to a (tcp_payload_t *) argument. (void *) is > explicitly allowed to be cast to any pointer type, so I think the > compiler will take this as asserting we know what we're doing. More > fool it. Oh, hm, right. In the original case we were discussing in that thread it was coming from an offset in a static struct, but if it's not the case anymore, then we should check ourselves I guess (possibly with the function + macro you mention below?). > > > > > Of course the current solution is not elegant and it would be nice to > > > > > find another way to deal with it, but we couldn't come up with anything > > > > > better back then. > > > > > > > > > > The rest of the series looks good to me, but I'm afraid that without > > > > > this one and 5/7 the other changes will be a bit more complicated to > > > > > implement (if at all possible). > > > > > > > > Definitely. I have so ideas for approaches more robust to > > > > misalignment, but they're substantially more complicated. I was > > > > hoping we could avoid it at least for now. > > > > > > I had a closer look at that earlier message now. I believe at the > > > time I was aiming for fully robust handling of misaligned user > > > buffers. AIUI, we've given up on that for the time being: instead > > > we'll just *test* for suitable alignment and we can do the hard work > > > of handling it if it ever arises in practice. > > > > Right, and we can use the compiler to test for suitable alignment. > > I do see allowing the compiler to check this in more cases as an > advantage of using explicitly typed pointers where we can. > > Btw, I didn't find a use for it just yet, but I also have a draft > patch which adds a function+macro that extracts a typed pointer from a > given offset into a IO vector, verifying that it's contiguous and > properly aligned. -- Stefano