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=K08Pu3SD; 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 E9DB35A061A for ; Wed, 06 Nov 2024 01:56:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1730854599; 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=EAr0A32qw90RQhOGjj5X/fiXYheKjOWJXADPreHp2fk=; b=K08Pu3SDeDL9Xyb7RRZI/YtOIRKJlzJkJ1JTftGLWuh3HpSYIB1zwvjobu7qC+7A3VWgLl iDTJebiEXsAS5kZn7qKMihWEuq4Tevfl2ppOW5ErTbyqcvlvWqwuO5/2e9t2oYLQxxdqku Ja59brmub8lNXgTeuQaKshdiUfUpUy4= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-365-BWu7hjebONa6MUtN4yZ5QA-1; Tue, 05 Nov 2024 19:56:38 -0500 X-MC-Unique: BWu7hjebONa6MUtN4yZ5QA-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-4315b7b0c16so41859705e9.1 for ; Tue, 05 Nov 2024 16:56:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730854597; x=1731459397; 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=EAr0A32qw90RQhOGjj5X/fiXYheKjOWJXADPreHp2fk=; b=O9it/7h5wfKlDInQZU276zz2N3JNHhWw8f+QZYNISUlrHgSdUBmS0dwhvuDxPAUfft v/CmtmsNQD1Lib7cz+ys2mLsQntSbuxXtrC0oPLUaghNbuyVqAYjEkJHrFOcGbkAxLr6 kO577+PII5XEPW712LiQqBPy2kxHbA6+Bz+5EmCA0be5T448EnUIWIuwZ96l7csSRSOf 3bmFQ9/F9UNSXp5uwjdbc1icIzaZgdXRKaRMuqtsphTzhxvwfRiJEUt5bPcfwlRWSJrp rIz56kKJDYa/Ho6eFSgcjogePGhwlgB1KV4RldZPn0sIVO5BVQWrGUVJ5/JkPJb4Jwcn zprg== X-Gm-Message-State: AOJu0YxjA8Gx8FpDerVqtQSBzebFkNrLKUKMZ9krW0RTBh8AQZYFQhzZ UtPd+VHY5/5di3AtXYeYWngPlfgrKXaEKmK/AngOqbAITcQHxYEHUkigOFszz8AX6AfkNXj6JPT oNs57f/CpPYGwAN7NvnsVsCMjyaUU9EC+TvI+MRMhzXS9fgif+Fy8peBIHA== X-Received: by 2002:a05:600c:1390:b0:431:5d4f:73a3 with SMTP id 5b1f17b1804b1-4319acb226amr326453525e9.18.1730854596738; Tue, 05 Nov 2024 16:56:36 -0800 (PST) X-Google-Smtp-Source: AGHT+IGKUFXev5nVKMYYhcSgGhvjXtKNYduTbqeMCU4sQ7zAfPuni6KEa6XsMxxyq6t1sN8U4eLxPQ== X-Received: by 2002:a05:600c:1390:b0:431:5d4f:73a3 with SMTP id 5b1f17b1804b1-4319acb226amr326453315e9.18.1730854596272; Tue, 05 Nov 2024 16:56:36 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-432aa6b2a3bsm3524315e9.11.2024.11.05.16.56.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Nov 2024 16:56:34 -0800 (PST) Date: Wed, 6 Nov 2024 01:56:31 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 1/1] iov: iov tail helpers Message-ID: <20241106015631.53041587@elisabeth> In-Reply-To: <20241105023222.698658-2-david@gibson.dropbear.id.au> References: <20241105023222.698658-1-david@gibson.dropbear.id.au> <20241105023222.698658-2-david@gibson.dropbear.id.au> 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: MEJ4EXMXI5E3UQK3P7WLW2I2UE6EB6VS X-Message-ID-Hash: MEJ4EXMXI5E3UQK3P7WLW2I2UE6EB6VS 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 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. 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 > --- > 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