* [PATCH 0/1] RFC: IOV tail helpers
@ 2024-11-05 2:32 David Gibson
2024-11-05 2:32 ` [PATCH 1/1] iov: iov " David Gibson
0 siblings, 1 reply; 6+ messages in thread
From: David Gibson @ 2024-11-05 2:32 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
As mentioned here's my draft patch adding a helper to pull headers off
an IOV, checking for contiguity, alignment and so forth. It's based
around the idea of an "iov tail", that is an iov from which you've
already consumed (in some sense) some data from the beginning. It
uses an explicit offset to track this, so we don't have to copy an
entire iovec array in order to trim a bit off it. But, it's built so
that if you pull enough off the front to remove some entire buffers,
it won't repeatedly have to step through them working out later
offsets.
It may be somewhat overengineered, let me know what you think of the
interface.
David Gibson (1):
iov: iov tail helpers
iov.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
iov.h | 24 +++++++++++++++++
2 files changed, 107 insertions(+)
--
2.47.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] iov: iov tail helpers
2024-11-05 2:32 [PATCH 0/1] RFC: IOV tail helpers David Gibson
@ 2024-11-05 2:32 ` David Gibson
2024-11-06 0:56 ` Stefano Brivio
0 siblings, 1 reply; 6+ messages in thread
From: David Gibson @ 2024-11-05 2:32 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
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
+ * @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
+ * @tail: IO vector tail to get header from
+ * @len: Length of header to remove in bytes
+ * @align: Required alignment of header in bytes
+ *
+ * @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 */
+
+ 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
+ * @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;
+ 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
+ * @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_))))
+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 */
--
@@ -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
+ * @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_))))
+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 */
--
2.47.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iov: iov tail helpers
2024-11-05 2:32 ` [PATCH 1/1] iov: iov " David Gibson
@ 2024-11-06 0:56 ` Stefano Brivio
2024-11-06 2:38 ` David Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Brivio @ 2024-11-06 0:56 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Tue, 5 Nov 2024 13:32:22 +1100
David Gibson <david@gibson.dropbear.id.au> 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.
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".
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:
> "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"?
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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.
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.
> + * @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.
What about "aligned block", or just "block"?
> + * @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.
> +
> + 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"?
> + * @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.
> + 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
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).
> +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?
--
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iov: iov tail helpers
2024-11-06 0:56 ` Stefano Brivio
@ 2024-11-06 2:38 ` David Gibson
2024-11-06 10:06 ` Stefano Brivio
0 siblings, 1 reply; 6+ messages in thread
From: David Gibson @ 2024-11-06 2:38 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 9733 bytes --]
On Wed, Nov 06, 2024 at 01:56:31AM +0100, Stefano Brivio wrote:
> On Tue, 5 Nov 2024 13:32:22 +1100
> David Gibson <david@gibson.dropbear.id.au> 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?
>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > 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.
> 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.
>
> > + * @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.
> > +
> > + 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
>
> 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"
> > +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.
--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iov: iov tail helpers
2024-11-06 2:38 ` David Gibson
@ 2024-11-06 10:06 ` Stefano Brivio
2024-11-08 4:18 ` David Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Brivio @ 2024-11-06 10:06 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Wed, 6 Nov 2024 13:38:38 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iov: iov tail helpers
2024-11-06 10:06 ` Stefano Brivio
@ 2024-11-08 4:18 ` David Gibson
0 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2024-11-08 4:18 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 12994 bytes --]
On Wed, Nov 06, 2024 at 11:06:18AM +0100, Stefano Brivio wrote:
> On Wed, 6 Nov 2024 13:38:38 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > > ---
> > > > 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.
Good idea. I've gone with "prune".
> > > 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.
Eh, barely, even then. We'd still need to have a fall back path for
when the header is *really* not contiguous. So it's a tiny
optimization for a case that will only happen if we're improbably
lucky about where the buffers end up.
Incidentally it would be quite easy to extend these helpers to handle
the split header case. Add a parameter with a "spare" buffer of the
necessary size/type. If the header is contiguous we just return it as
now, otherwise we linearize it into the spare buffer and return a
pointer to that. Not optimal, but might be worth it for the
simplicity.
> > > > + 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.
I'm going with "remove" for now.
> > > > + * @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
>
> ?
Ok. I've added a "theory of operation" comment, as well as revising
a bunch of other comments, which I hope will help.
> > > 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.
I've actually given them full function-style comments.
--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-08 4:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-05 2:32 [PATCH 0/1] RFC: IOV tail helpers David Gibson
2024-11-05 2:32 ` [PATCH 1/1] iov: iov " David Gibson
2024-11-06 0:56 ` Stefano Brivio
2024-11-06 2:38 ` David Gibson
2024-11-06 10:06 ` Stefano Brivio
2024-11-08 4:18 ` David Gibson
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).