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=WQ2SETbK; 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 301905A004C for ; Wed, 06 Nov 2024 11:06:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1730887585; 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=ppAACq8a7c1oWO6ol10bs7J07cWLGsP0Jdsml6hcoXs=; b=WQ2SETbKebU+II6YnkjbQwa3ycy7hW9R5E4gQq1chjNznPEoW60Hd/C9LG9iRcpxrJLtTh sHMY8vqYywuGE5KRx8mOPw0igeoK2isqPI9SRq3g1NlXLxWgGKP2QWVeL77z9/+5jrpi4H WrPrpIDvdGyOzOAolAUrwqL3WSi9diI= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-633-SWLAum-_MVS6lgmMsrJDYw-1; Wed, 06 Nov 2024 05:06:23 -0500 X-MC-Unique: SWLAum-_MVS6lgmMsrJDYw-1 X-Mimecast-MFC-AGG-ID: SWLAum-_MVS6lgmMsrJDYw Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-4315d98a873so38215155e9.1 for ; Wed, 06 Nov 2024 02:06:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730887582; x=1731492382; 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=ppAACq8a7c1oWO6ol10bs7J07cWLGsP0Jdsml6hcoXs=; b=Sqjt7W5DLOKjz+VsIZJQ8XVr4tNyd3/nvp4+4PINZ5bSEMRW95gjABhdRzfHHtCoI4 46fOXgGZpXowInWTeXtgENLGdnmRUBswTQmA5K3DrqrLHFvhA4L8d6Whdq7ceGhQz8qo YI34AxPPtQGWPprIM27M3mYuGYPxAx5sI5d6J04ah9ivCHaDfE4JtuZX3h7pFC6iRMSu LE6Kt7NfFLZuVIWhNdFpwCnBMQ7GHw12BZjuZA65FaqfBFgVPeJ1JjASGZL6wytiYoM4 3OpVgYByF2br3uPGJeUuHK0NAfQTqpx4MemccssXJaUAguWuY7bFL2bzZSvN/vVZTSz8 oPJQ== X-Gm-Message-State: AOJu0Ywra2WMkI1csTvfe7+RhcKMlsFYHtnsodsiVvMEd00HV7MhN7jZ MrYR1M05UVnZ73ic/bDyyaR+9c5apf3LEScFVz1fcMY+JCeWSKRq/h8aOJRkIqFG1NiHB7KXA// 3u9+pI9zThReqaUrJt9enNKzoGhmH4GbdWvCYIaJ1EBihQ5A8WQ== X-Received: by 2002:a05:600c:1f82:b0:430:57f2:bae2 with SMTP id 5b1f17b1804b1-431bb9d14afmr221154865e9.23.1730887582166; Wed, 06 Nov 2024 02:06:22 -0800 (PST) X-Google-Smtp-Source: AGHT+IHUzxbRrhCxyk30/mIgMYSNoriCUsPcWA2vMWR3aV6VBX68V/ucbjSTGLrRadlXyzljJ+L8Mw== X-Received: by 2002:a05:600c:1f82:b0:430:57f2:bae2 with SMTP id 5b1f17b1804b1-431bb9d14afmr221154505e9.23.1730887581577; Wed, 06 Nov 2024 02:06:21 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-432aa74bab3sm16519595e9.43.2024.11.06.02.06.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Nov 2024 02:06:20 -0800 (PST) Date: Wed, 6 Nov 2024 11:06:18 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 1/1] iov: iov tail helpers Message-ID: <20241106110618.333e3c8e@elisabeth> In-Reply-To: References: <20241105023222.698658-1-david@gibson.dropbear.id.au> <20241105023222.698658-2-david@gibson.dropbear.id.au> <20241106015631.53041587@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-MFC-PROC-ID: wcZourFB0GfNQUo9RPh6b0aCz74Mm9F0_BgMWHMaPQo_1730887582 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: AAFA77UKIH4NZSCHXMZN2XRNI5YDBFVQ X-Message-ID-Hash: AAFA77UKIH4NZSCHXMZN2XRNI5YDBFVQ 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: 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, 6 Nov 2024 13:38:38 +1100 David Gibson wrote: > On Wed, Nov 06, 2024 at 01:56:31AM +0100, Stefano Brivio wrote: > > On Tue, 5 Nov 2024 13:32:22 +1100 > > David Gibson wrote: > > > > > In the vhost-user code we have a number of places where we need to locate > > > a particular header within the guest-supplied IO vector. We need to work > > > out which buffer the header is in, and verify that it's contiguous and > > > aligned as we need. At the moment this is open-coded, but introduce a > > > helper to make this more straightforward. > > > > > > We add a new datatype 'struct iov_tail' representing an IO vector from > > > which we've logically consumed some number of headers. The IOV_PULL_HEADER > > > macro consumes a new header from the vector, returning a pointer and > > > updating the iov_tail. > > > > The interfaces look usable and straightforward to me. I find some names > > and comments a bit obscure, though. > > Yeah, I don't love the names either. > > > First off, I would intuitively say that the "tail" is always at the > > end, and if we already consumed something, that's always at the "head". > > Right.. which is true in a sense. The idea is you'd set one of these > up, to cover a whole (say) frame, then pull bits off the front as you > need it. So, the iov_tail does represent the "tail", as in the > unprocessed bit of the frame at each point... > > > If we call the whole abstraction "tail", we risk ending up talking about > > the tail of the tail, and the head of the tail. Consider this part from > > the cover letter: > > .. but, yeah, heads of tails and tails of tails gets confusing. > Unless we rewrite in LISP, I guess. > > > > "iov tail", that is an iov from which you've > > > already consumed (in some sense) some data from the beginning. > > > > ...in other words, that's an IO vector called tail, and we > > already consumed some data from its head. > > > > What about (iov-based) "batch"? > > I don't think "batch" is really any better, it's just unclear on a > different set of axes. > > Would "iov remainder" be any better? Now that I read your reply, the "tail" name becomes clearer as it's clear that we just use it for left-overs. I don't think "remainder" makes it much clearer, so I would rather stick to "tail" and try to explain the purpose of struct iov_tail a bit more specifically (see below). > > > Signed-off-by: David Gibson > > > --- > > > iov.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > iov.h | 24 +++++++++++++++++ > > > 2 files changed, 107 insertions(+) > > > > > > diff --git a/iov.c b/iov.c > > > index 3f9e229..3d384ae 100644 > > > --- a/iov.c > > > +++ b/iov.c > > > @@ -156,3 +156,86 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt) > > > > > > return len; > > > } > > > + > > > +/** > > > + * iov_tail_shorten() - Remove any buffers from an IOV tail that are wholly consumed > > > > "Remove" is a bit difficult to interpret (does it deallocate? Throw > > data away^), I would rather say that we... detach (?) those buffers from > > the batch/tail. > > Yeah, I'm not sure how to express this either. In a sense this > operation is a logical no-op: it shouldn't change the results of any > future operation, but it might make them slightly faster. Oh, this wasn't clear to me. I guess that "reduce", "minimise", or "prune" would convey this better. > > This operation itself raises a question though: if the batch already > > carries the information that some buffers were completely consumed, > > should it ever be in a state where we want to drop these buffers from > > it? > > > > That is, it sounds like we have some other operation that allows it to > > be in an inconsistent state. > > Sort of, yes, but I think this is the right design choice. It means > we can trivially construct one of these things with an arbitrary > offset, and we only do the work of stepping through the buffers when > we actually have to. We can discard bytes simply by adding to the > offset, without having to look at the actual buffers until later. > > Basically if we want to ensure that the representation is always > "minimal", in the sense that the offset lies within the first buffer, > then a bunch more operations need to do work to maintain that. Plus, > this approach is naturally robust: if we somehow get an iov_tail in > non-minimal form, peek/pull will just handle it with no extra logic. > > > > + * @tail: IO vector tail (modified) > > > + * > > > + * Return: true if the tail still contains any bytes, otherwise false > > > + */ > > > +bool iov_tail_shorten(struct iov_tail *tail) > > > +{ > > > + size_t i; > > > + > > > + i = iov_skip_bytes(tail->iov, tail->cnt, tail->off, &tail->off); > > > + tail->iov += i; > > > + tail->cnt -= i; > > > + > > > + return !!tail->cnt; > > > +} > > > + > > > +/** > > > + * iov_tail_size - Calculate the total size of an IO vector tail > > > + * @tail: IO vector tail > > > + * > > > + * Returns: The total size in bytes. > > > + */ > > > +/* cppcheck-suppress unusedFunction */ > > > +size_t iov_tail_size(struct iov_tail *tail) > > > +{ > > > + iov_tail_shorten(tail); > > > + return iov_size(tail->iov, tail->cnt) - tail->off; > > > +} > > > + > > > +/** > > > + * iov_peek_header_() - Get pointer to header from an IOV tail > > > > I think that this needs to be more generic than "header", because yes, > > we're using it for headers, but that word doesn't really help in this > > context. > > Hm. Well, we are using it for headers, and we're also always pulling > from the "head" of whatever we have left. > > > What about "aligned block", or just "block"? > > Maybe... but it really does have to be from the start of the current > tail. "Head block"? Or forget about it, "header" is actually fine. > > > > > + * @tail: IO vector tail to get header from > > > + * @len: Length of header to remove in bytes > > > > to remove, in bytes > > > > > + * @align: Required alignment of header in bytes > > > > Judging from this comment alone, it's not clear if 0 or 1 should be > > used to get freely aligned blocks. > > > > > + * > > > + * @tail may be modified, but will be semantically equivalent. > > > + * > > > + * Returns: Pointer to the removed header, NULL if it overruns the IO > > > + * vector, is not contiguous or is misaligned. > > > + */ > > > +void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) > > > +{ > > > + char *p; > > > + > > > + if (!iov_tail_shorten(tail)) > > > + return NULL; /* Nothing left */ > > > + > > > + if (tail->off + len < tail->off) > > > + return NULL; /* Overflow */ > > > + > > > + if (tail->off + len > tail->iov[0].iov_len) > > > + return NULL; /* Not contiguous */ > > > > I'm not sure if this observation is useful in some cases, but this > > doesn't necessarily mean that the header/block is not contiguous: if > > tail->iov[0].iov_base + tail->iov[0].iov_len == tail->iov[1].iov_base, > > it actually is. > > Hm, true. Not sure if it's worth the tests handle that case though. I guess it's worth it if there's any chance we'll ever want to support split headers. > > > + p = (char *)tail->iov[0].iov_base + tail->off; > > > + if ((uintptr_t)p % align) > > > + return NULL; /* not aligned */ > > > + > > > + return p; > > > +} > > > +/** > > > + * iov_pull_header_() - Remove a header from an IOV tail > > > > I know that "pulling" is widely used, but it's sometimes ambiguous (I > > guess we already had a discussion about that in the past). What about > > "remove", or "drop"? > > "remove" might work. I don't like "drop" because that implies to me > it's just gone, rather than returned. > > > > + * @tail: IO vector tail to remove header from (modified) > > > + * @len: Length of header to remove in bytes > > > + * @align: Required alignment of header in bytes > > > + * > > > + * @tail is updated so that it no longer includes the extracted header > > > + * > > > + * Returns: Pointer to the removed header, NULL if it overruns the IO > > > + * vector, is not contiguous or is misaligned. > > > + */ > > > +/* cppcheck-suppress unusedFunction */ > > > +void *iov_pull_header_(struct iov_tail *tail, size_t len, size_t align) > > > +{ > > > + char *p = iov_peek_header_(tail, len, align); > > > + > > > + if (!p) > > > + return NULL; > > > + > > > + tail->off = tail->off + len; > > > > This could just be += I guess. > > True. > > > > + return p; > > > +} > > > diff --git a/iov.h b/iov.h > > > index a9e1722..a2f449c 100644 > > > --- a/iov.h > > > +++ b/iov.h > > > @@ -28,4 +28,28 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt, > > > size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, > > > size_t offset, void *buf, size_t bytes); > > > size_t iov_size(const struct iovec *iov, size_t iov_cnt); > > > + > > > +/** > > > + * struct iov_tail - Represents the fail portion of an IO vector ...so, it's not just a tail portion, it's a tail portion in a rather specific and constrained context, and once that's revealed these helpers are easier (for me) to understand. What about: * struct iov_tail - Remaining, not consumed portion (tail) of IO vector ? > > s/fail/tail/ > > > > > + * @iov: IO vector > > > + * @cnt: Number of entries in @iov > > > + * @off: Current offset in @iov > > > + */ > > > +struct iov_tail { > > > + const struct iovec *iov; > > > + size_t cnt, off; > > > +}; > > > + > > > +#define IOV_TAIL(iov_, cnt_, off_) \ > > > + (struct iov_tail){ .iov = (iov_), .cnt = (cnt_), .off = (off_) } > > > + > > > +bool iov_tail_shorten(struct iov_tail *tail); > > > +size_t iov_tail_size(struct iov_tail *tail); > > > +void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align); > > > +#define IOV_PEEK_HEADER(tail_, ty_) \ > > > + ((ty_ *)(iov_peek_header_((tail_), sizeof(ty_), __alignof__(ty_)))) > > > > I guess 'x' would be as clear as 'ty_' (actually, I'm failing to guess > > what it stands for). > > "type" Oh. That's kind of obscure. I guess it would be better to actually spell that out and define this on multiple lines. > > > +void *iov_pull_header_(struct iov_tail *tail, size_t len, size_t align); > > > +#define IOV_PULL_HEADER(tail_, ty_) \ > > > + ((ty_ *)(iov_pull_header_((tail_), sizeof(ty_), __alignof__(ty_)))) > > > + > > > #endif /* IOVEC_H */ > > > > I would have expected some functions to add data or build those > > tails... or we don't need them for some reason? > > The IOV_TAIL() macro builds one. Adding data doesn't make sense; the > idea is this is a view into part of an existing IO vector, which > doesn't require copying the struct iovecs of that original vector. I see now, I didn't pay much attention to that. Perhaps some one-liner function-like comments around IOV_TAIL(), IOV_PEEK_HEADER() and IOV_PULL_HEADER() would help. -- Stefano